AydarZaynutdinov commented on a change in pull request #16891:
URL: https://github.com/apache/beam/pull/16891#discussion_r810050012



##########
File path: playground/backend/internal/code_processing/code_processing_test.go
##########
@@ -99,11 +98,12 @@ func Test_Process(t *testing.T) {
        if err != nil {
                panic(err)
        }
-       sdkEnv, err := environment.ConfigureBeamEnvs(appEnvs.WorkingDir())
+       sdkJavaEnv, err := environment.ConfigureBeamEnvs(appEnvs.WorkingDir())

Review comment:
       Can we add tests cases for all supported SDKs?

##########
File path: playground/backend/internal/code_processing/code_processing_test.go
##########
@@ -948,11 +967,17 @@ func Test_runStep(t *testing.T) {
        if err != nil {
                panic(err)
        }
-       sdkEnv, err := environment.ConfigureBeamEnvs(appEnvs.WorkingDir())
+       sdkJavaEnv, err := environment.ConfigureBeamEnvs(appEnvs.WorkingDir())

Review comment:
       I also see that we don't have test cases for the `compileStep()` method:
   - case with `Python SDK`/`SCIO SDK` or `Go SDK` unit tests -> cache should 
contain status `Status_STATUS_EXECUTING` and empty `CompileOutput`, 
`RunOutput`, `RunError`, `Logs`, `Graph` 
   - case when `reconcileBackgroundTask()` method returns `err` (expired 
context for example) -> cache should contain the same status like it was before.

##########
File path: playground/backend/internal/code_processing/code_processing_test.go
##########
@@ -948,11 +967,17 @@ func Test_runStep(t *testing.T) {
        if err != nil {
                panic(err)
        }
-       sdkEnv, err := environment.ConfigureBeamEnvs(appEnvs.WorkingDir())
+       sdkJavaEnv, err := environment.ConfigureBeamEnvs(appEnvs.WorkingDir())

Review comment:
       `processSetupError()` method:
   - test case with an error during `utils.SetToCache()` -> return `err` 
   
   `GetLastIndex()` method:
   - test case with an error during converging value to `float64` -> return 
`err`
   
   `readGraphFile()` method:
   - test case with reading an existed graph file -> cache should contain 
`Graph`
   - test case with `ctx.Done()` and exists graph file -> cache should contain 
`Graph`
   
   `processErrorWithSavingOutput` method:
   - test case with an error during `utils.SetToCache` method (maybe use 
redis-mock) -> return `err`
   
   `processRunError` method:
   - ditto
   
   `processCompileSuccess` method:
   - tests cases with an error during `utils.SetToCache()` for each subkeys 
(`CompileOutput`/`RunOutput`/`RunError`/`Logs`/`Graph`/`Status`) (maybe use 
redis-mock) -> return `err` 
   
   
   

##########
File path: playground/backend/internal/code_processing/code_processing_test.go
##########
@@ -948,11 +967,17 @@ func Test_runStep(t *testing.T) {
        if err != nil {
                panic(err)
        }
-       sdkEnv, err := environment.ConfigureBeamEnvs(appEnvs.WorkingDir())
+       sdkJavaEnv, err := environment.ConfigureBeamEnvs(appEnvs.WorkingDir())

Review comment:
       `validateStep()` method:
   
   - test case with an error during `builder.Preparer()` -> cache should 
contain status `Status_STATUS_ERROR`

##########
File path: playground/backend/internal/code_processing/code_processing_test.go
##########
@@ -948,11 +967,17 @@ func Test_runStep(t *testing.T) {
        if err != nil {
                panic(err)
        }
-       sdkEnv, err := environment.ConfigureBeamEnvs(appEnvs.WorkingDir())
+       sdkJavaEnv, err := environment.ConfigureBeamEnvs(appEnvs.WorkingDir())

Review comment:
       Could we add test cases with `Go SDK` (unit tests and examples) that 
contain errors? Our tests don't cover these cases. 

##########
File path: playground/backend/internal/code_processing/code_processing_test.go
##########
@@ -948,11 +967,17 @@ func Test_runStep(t *testing.T) {
        if err != nil {
                panic(err)
        }
-       sdkEnv, err := environment.ConfigureBeamEnvs(appEnvs.WorkingDir())
+       sdkJavaEnv, err := environment.ConfigureBeamEnvs(appEnvs.WorkingDir())

Review comment:
       `prepareStep()` method:
   - test case with an error during `builder.Preparer()` -> cache should 
contain status `Status_STATUS_ERROR`
   - case when `reconcileBackgroundTask()` method returns err (expired context 
for example) -> cache should contain the same status like it was before.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


Reply via email to