[Test] Add Task service api test structure and for DeployTask #6823
Conversation
Signed-off-by: machichima <[email protected]>
Signed-off-by: machichima <[email protected]>
Signed-off-by: machichima <[email protected]>
Signed-off-by: machichima <[email protected]>
Signed-off-by: machichima <[email protected]>
machichima
changed the title
| func getEnvOrDefault(key, defaultValue string) string { | ||
| if value, exists := os.LookupEnv(key); exists { | ||
| return value | ||
| } | ||
| return defaultValue | ||
| } |
There was a problem hiding this comment.
Hmm I think this function was never used right 🤔
| go func() { | ||
| log.Printf("Test server starting on %s", endpoint) | ||
| if err := testServer.ListenAndServe(); err != nil && err != http.ErrServerClosed { | ||
| log.Printf("Server error: %v", err) | ||
| } | ||
| }() | ||
|
|
||
| // Wait for server to be ready | ||
| if !waitForServer(endpoint, 10*time.Second) { | ||
| log.Fatal("Test server failed to start") | ||
| } |
There was a problem hiding this comment.
If the server fails to bind to port 8091, besides logging, the code still keeps on running for 10 seconds. Do you think it's a good idea to stop early whenever the server fails to bind in the goroutine?
|
|
||
| ### Run API tests | ||
|
|
||
| Pleaes ensure the service is started by following [Quick Start with SQLite](#quick-start-with-sqlite) section |
| for _, table := range tables { | ||
| if err := testDB.Exec(fmt.Sprintf("DELETE FROM %s", table)).Error; err != nil { | ||
| t.Logf("Warning: Failed to cleanup table %s: %v", table, err) | ||
| } | ||
| } |
There was a problem hiding this comment.
I think it'll be better to change it to t.Fatalf() to stop the function as soon as the deletion of database was errored, so we wouldn't have used the broken database down the way. What do you think about this?
|
|
||
| // Delete all records from all tables | ||
| // Order matters due to foreign key constraints (if any) | ||
| tables := []string{"tasks", "task_specs", "actions"} |
There was a problem hiding this comment.
I acknowledge we only got these three tables right now. But what do you feel about sending query for dynamic table check?
| ) | ||
|
|
||
| const ( | ||
| testPort = 8091 // Different port to avoid conflicts with main service |
There was a problem hiding this comment.
This is just pure out of curiosity. Will people have their port 8091 being used❓
| // Teardown: Stop server | ||
| shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) | ||
| defer cancel() | ||
| if err := testServer.Shutdown(shutdownCtx); err != nil { | ||
| log.Printf("Test server shutdown error: %v", err) | ||
| } | ||
| log.Println("Test server stopped") | ||
|
|
||
| // Cleanup: Remove test database file | ||
| if err := os.Remove(testDBFile); err != nil && !os.IsNotExist(err) { | ||
| log.Printf("Warning: Failed to remove test database: %v", err) | ||
| } |
There was a problem hiding this comment.
Here cleanup only runs if we reach the end of TestMain. If there’s a panic or early exit during setup, the server and DB file might be left around. Would it make sense to move the cleanup into the deferred block so it always runs?
3 participants
Tracking issue
Closes #6818
Closes #6819
Why are the changes needed?
Add api test for task service.
What changes were proposed in this pull request?
DeployTaskHow was this patch tested?
Labels
Please add one or more of the following labels to categorize your PR:
This is important to improve the readability of release notes.
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Docs link