test: refactor container_stop_linux.go to use Tigron#4863
Open
opjt wants to merge 1 commit intocontainerd:mainfrom
Open
test: refactor container_stop_linux.go to use Tigron#4863opjt wants to merge 1 commit intocontainerd:mainfrom
opjt wants to merge 1 commit intocontainerd:mainfrom
Conversation
46 tasks
haytok
reviewed
Apr 26, 2026
opjt
force-pushed
the
issues_4613_container_stop_linux_test
branch
from
9519677 to
358c028
Compare
Contributor
Author
|
@haytok thanks for your review! Could you take another look? |
haytok
reviewed
Apr 28, 2026
Comment on lines
-197
to
-200
| elapsed := time.Since(start) | ||
| testCase.Setup = func(data test.Data, helpers test.Helpers) { | ||
| // Start a container that sleeps forever | ||
| helpers.Ensure("run", "-d", "--name", data.Identifier(), testutil.CommonImage, "sleep", nerdtest.Infinity) | ||
| } | ||
|
|
||
| // The container should get the SIGKILL before the 10s default timeout | ||
| assert.Assert(t, elapsed < 10*time.Second, "Container did not respect --timeout flag") |
Contributor
Author
There was a problem hiding this comment.
replaced cmd.WithTimeout(10 * time.Second)
Comment on lines
-179
to
-182
| elapsed := time.Since(start) | ||
| testCase.Expected = test.Expects(expect.ExitCodeSuccess, nil, nil) | ||
|
|
||
| // The container should be stopped almost immediately, well before the 5-second timeout | ||
| assert.Assert(t, elapsed < 5*time.Second, "Container wasn't stopped immediately with SIGKILL") |
Contributor
Author
There was a problem hiding this comment.
The elapsed time assertion was replaced with cmd.WithTimeout(5s), which is more direct — if the stop command doesn't complete within 5 seconds, the test fails immediately
| return nil | ||
| } | ||
|
|
||
| assert.NilError(t, check(5)) |
Contributor
Author
There was a problem hiding this comment.
Added it back in testCase.Setup
Comment on lines
+96
to
+97
| helpers.Fail("exec", data.Labels().Get("containerName"), "ps") | ||
| assert.Assert(t, httpCheck(data, 1) != nil, "expected HTTP to fail after stop") |
Member
There was a problem hiding this comment.
Shouldn't this be split into a separate subtest?
Contributor
Author
There was a problem hiding this comment.
I think keeping them in the same subtest is fine as a verification that stop was successful.
Signed-off-by: Park jungtae <jtpark1957@gmail.com>
opjt
force-pushed
the
issues_4613_container_stop_linux_test
branch
from
358c028 to
c55d91b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
testutil.NewBaseto usenerdtest.Setup(Tigron) #4613