test: add test on restart and dev command #34
Conversation
Your org has enabled the Graphite merge queue for merging into mainAdd the label “graphite/merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. You must have a Graphite account in order to use the merge queue. Sign up using this link. |
WalkthroughThe recent updates introduce a new Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
Current dependencies on/for this PR: This stack of pull requests is managed by Graphite. |
zulkhair
force-pushed
the
daim/world-532-add_test_on_restart_and_dev_command
branch
from
33a011f
to
94866b1
Compare
zulkhair
force-pushed
the
daim/world-757-add_hot_reloading_on_dev_mode
branch
from
04487c0
to
94b6df6
Compare
zulkhair
force-pushed
the
daim/world-532-add_test_on_restart_and_dev_command
branch
2 times, most recently
from
a29ee6c
to
938db8d
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #34 +/- ##
=======================================
Coverage 47.97% 47.97%
=======================================
Files 8 8
Lines 469 469
=======================================
Hits 225 225
Misses 229 229
Partials 15 15 ☔ View full report in Codecov by Sentry. |
zulkhair
force-pushed
the
daim/world-757-add_hot_reloading_on_dev_mode
branch
2 times, most recently
from
c95b668
to
66b2b6f
Compare
zulkhair
force-pushed
the
daim/world-532-add_test_on_restart_and_dev_command
branch
from
938db8d
to
641c402
Compare
zulkhair
force-pushed
the
daim/world-757-add_hot_reloading_on_dev_mode
branch
from
66b2b6f
to
bcb810b
Compare
zulkhair
force-pushed
the
daim/world-532-add_test_on_restart_and_dev_command
branch
from
641c402
to
b627701
Compare
zulkhair
force-pushed
the
daim/world-532-add_test_on_restart_and_dev_command
branch
from
b627701
to
e3d993c
Compare
zulkhair
force-pushed
the
daim/world-757-add_hot_reloading_on_dev_mode
branch
from
bcb810b
to
125ed24
Compare
zulkhair
changed the base branch from
daim/world-757-add_hot_reloading_on_dev_mode
to
main
zulkhair
force-pushed
the
daim/world-532-add_test_on_restart_and_dev_command
branch
from
e3d993c
to
4e86b62
Compare
Merge activity
|
Closes: WORLD-532 ## Overview Add test for `restart` and `dev` command ## Brief Changelog - Add Detach mode for restart - Modify older test, adding restart before stop and purge - Add new test for dev ## Testing and Verifying - Running the unit test suite
zulkhair
force-pushed
the
daim/world-532-add_test_on_restart_and_dev_command
branch
from
4e86b62
to
4a0ac1e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (2)
- cmd/world/cardinal/restart.go (2 hunks)
- cmd/world/root/root_test.go (3 hunks)
Additional comments: 3
cmd/world/cardinal/restart.go (1)
- 16-16: Ensure the
flagDetachvariable is defined and correctly scoped within the package or globally if intended for reuse.cmd/world/root/root_test.go (2)
- 12-12: The import of
pkg.world.dev/world-cli/cmd/world/cardinalis added. Verify its usage within this test file to ensure it's necessary, as direct code references to thecardinalpackage are not visible in the provided code segments.- 18-38: The
healthResponsestruct andgetHealthCheckfunction are well-defined for checking the health of the application. Ensure the health check endpoint (http://127.0.0.1:3333/health) matches the application's actual health check URL and is not hardcoded for testing environments.
| if replaceBoolWithFlag(cmd, flagDetach, &cfg.Detach); err != nil { | ||
| return err |
There was a problem hiding this comment.
The error handling for replaceBoolWithFlag does not capture the specific error from the function call due to shadowing err from the outer scope. Consider assigning the error to a new variable or directly returning it without assignment.
- if replaceBoolWithFlag(cmd, flagDetach, &cfg.Detach); err != nil {
+ if err := replaceBoolWithFlag(cmd, flagDetach, &cfg.Detach); err != nil {Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if replaceBoolWithFlag(cmd, flagDetach, &cfg.Detach); err != nil { | |
| return err | |
| if err := replaceBoolWithFlag(cmd, flagDetach, &cfg.Detach); err != nil { | |
| return err |
| IsGameLoopRunning bool | ||
| } | ||
| err = json.NewDecoder(resp.Body).Decode(&healthResponse) | ||
| assert.Assert(t, resp.IsServerRunning) | ||
| assert.Assert(t, resp.IsGameLoopRunning) | ||
|
|
||
| // Restart cardinal | ||
| rootCmd.SetArgs([]string{"cardinal", "restart", "--detach"}) | ||
| err = rootCmd.Execute() | ||
| assert.NilError(t, err) | ||
|
|
||
| // Check cardinal health after restart | ||
| resp, err = getHealthCheck() | ||
| assert.NilError(t, err) | ||
| assert.Assert(t, healthResponse.IsServerRunning) | ||
| assert.Assert(t, healthResponse.IsGameLoopRunning) | ||
| assert.Equal(t, resp.StatusCode, 200) | ||
| assert.Assert(t, resp.IsServerRunning) | ||
| assert.Assert(t, resp.IsGameLoopRunning) | ||
|
|
||
| // Stop cardinal | ||
| rootCmd.SetArgs([]string{"cardinal", "stop"}) | ||
| err = rootCmd.Execute() | ||
| assert.NilError(t, err) | ||
|
|
||
| // Check cardinal health | ||
| _, err = http.Get("http://127.0.0.1:3333/health") | ||
| // Check cardinal health (expected error) | ||
| _, err = getHealthCheck() | ||
| assert.Error(t, err, | ||
| "Get \"http://127.0.0.1:3333/health\": dial tcp 127.0.0.1:3333: connect: connection refused") | ||
| } | ||
|
|
||
| // Purge cardinal | ||
| rootCmd.SetArgs([]string{"cardinal", "purge"}) | ||
| err = rootCmd.Execute() | ||
| func TestDev(t *testing.T) { | ||
| // Create Cardinal | ||
| gameDir, err := os.MkdirTemp("", "game-template") | ||
| assert.NilError(t, err) | ||
|
|
||
| // Remove dir | ||
| defer func() { | ||
| err = os.RemoveAll(gameDir) | ||
| assert.NilError(t, err) | ||
| }() | ||
|
|
||
| // Change dir | ||
| err = os.Chdir(gameDir) | ||
| assert.NilError(t, err) | ||
|
|
||
| //set tea ouput to variable | ||
| teaOut := &bytes.Buffer{} | ||
| createCmd := getCreateCmd(teaOut) | ||
| createCmd.SetArgs([]string{gameDir}) | ||
|
|
||
| err = createCmd.Execute() | ||
| assert.NilError(t, err) | ||
|
|
||
| // Start cardinal dev | ||
| rootCmd.SetArgs([]string{"cardinal", "dev"}) | ||
| go func() { | ||
| err := rootCmd.Execute() | ||
| assert.NilError(t, err) | ||
| }() | ||
|
|
||
| // Check cardinal health for 300 second, waiting to download dependencies and building the apps | ||
| isServerRunning := false | ||
| isGameLoopRunning := false | ||
| timeout := time.Now().Add(300 * time.Second) | ||
| for !(isServerRunning && isGameLoopRunning) && time.Now().Before(timeout) { | ||
| resp, err := getHealthCheck() | ||
| if err != nil { | ||
| time.Sleep(1 * time.Second) | ||
| continue | ||
| } | ||
| assert.Equal(t, resp.StatusCode, 200) | ||
| isServerRunning = resp.IsServerRunning | ||
| isGameLoopRunning = resp.IsGameLoopRunning | ||
| } | ||
| assert.Assert(t, isServerRunning) | ||
| assert.Assert(t, isGameLoopRunning) | ||
|
|
||
| // Shutdown the program | ||
| close(cardinal.StopChan) | ||
|
|
||
| // Check cardinal health (expected error), trying for 10 times | ||
| count := 0 | ||
| for count < 10 { | ||
| _, err = getHealthCheck() | ||
| if err != nil { | ||
| break | ||
| } | ||
| time.Sleep(1 * time.Second) | ||
| count++ | ||
| } | ||
|
|
||
| assert.Error(t, err, | ||
| "Get \"http://127.0.0.1:3333/health\": dial tcp 127.0.0.1:3333: connect: connection refused") | ||
| } |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [125-188]
The test TestCreateStartStopRestartPurge correctly follows the lifecycle of creating, starting, restarting, and purging the application. However, ensure that the health check assertions (assert.Equal(t, resp.StatusCode, 200)) are robust enough to handle transient states or delays in service availability, especially after restart operations.
Consider adding retries with backoff for health checks to handle transient failures.
| func TestDev(t *testing.T) { | ||
| // Create Cardinal | ||
| gameDir, err := os.MkdirTemp("", "game-template") | ||
| assert.NilError(t, err) | ||
|
|
||
| // Remove dir | ||
| defer func() { | ||
| err = os.RemoveAll(gameDir) | ||
| assert.NilError(t, err) | ||
| }() | ||
|
|
||
| // Change dir | ||
| err = os.Chdir(gameDir) | ||
| assert.NilError(t, err) | ||
|
|
||
| //set tea ouput to variable | ||
| teaOut := &bytes.Buffer{} | ||
| createCmd := getCreateCmd(teaOut) | ||
| createCmd.SetArgs([]string{gameDir}) | ||
|
|
||
| err = createCmd.Execute() | ||
| assert.NilError(t, err) | ||
|
|
||
| // Start cardinal dev | ||
| rootCmd.SetArgs([]string{"cardinal", "dev"}) | ||
| go func() { | ||
| err := rootCmd.Execute() | ||
| assert.NilError(t, err) | ||
| }() | ||
|
|
||
| // Check cardinal health for 300 second, waiting to download dependencies and building the apps | ||
| isServerRunning := false | ||
| isGameLoopRunning := false | ||
| timeout := time.Now().Add(300 * time.Second) | ||
| for !(isServerRunning && isGameLoopRunning) && time.Now().Before(timeout) { | ||
| resp, err := getHealthCheck() | ||
| if err != nil { | ||
| time.Sleep(1 * time.Second) | ||
| continue | ||
| } | ||
| assert.Equal(t, resp.StatusCode, 200) | ||
| isServerRunning = resp.IsServerRunning | ||
| isGameLoopRunning = resp.IsGameLoopRunning | ||
| } | ||
| assert.Assert(t, isServerRunning) | ||
| assert.Assert(t, isGameLoopRunning) | ||
|
|
||
| // Shutdown the program | ||
| close(cardinal.StopChan) | ||
|
|
||
| // Check cardinal health (expected error), trying for 10 times | ||
| count := 0 | ||
| for count < 10 { | ||
| _, err = getHealthCheck() | ||
| if err != nil { | ||
| break | ||
| } | ||
| time.Sleep(1 * time.Second) | ||
| count++ | ||
| } | ||
|
|
||
| assert.Error(t, err, | ||
| "Get \"http://127.0.0.1:3333/health\": dial tcp 127.0.0.1:3333: connect: connection refused") |
There was a problem hiding this comment.
The TestDev function introduces a test for the development mode, including asynchronous execution and health checks. Ensure that the asynchronous execution pattern (go func() { ... }()) is correctly synchronized with the test's main goroutine to prevent premature test termination or flaky test results.
Implement a more reliable synchronization mechanism, such as using sync.WaitGroup or channels, to ensure the test waits for the asynchronous operation to complete before proceeding with assertions.
Closes: WORLD-532
Overview
Add test for
restartanddevcommandBrief Changelog
Testing and Verifying
Summary by CodeRabbit
--detachflag for therestartcommand to enable running in detached mode.