Skip to content

container/integration: TestResize: add more test-cases, and add TestExecResize#48665

Merged
thaJeztah merged 2 commits intomoby:masterfrom
thaJeztah:integration_resize
Oct 16, 2024
Merged

container/integration: TestResize: add more test-cases, and add TestExecResize#48665
thaJeztah merged 2 commits intomoby:masterfrom
thaJeztah:integration_resize

Conversation

@thaJeztah
Copy link
Copy Markdown
Member

- How to verify it

make TEST_FILTER='Test(ExecResize|Resize)' TEST_SKIP_INTEGRATION_CLI=1 DOCKER_GRAPHDRIVER=vfs test-integration
=== RUN   TestExecResize
=== RUN   TestExecResize/success
=== RUN   TestExecResize/invalid_size
=== RUN   TestExecResize/invalid_size/unset_height
=== RUN   TestExecResize/invalid_size/unset_width
=== RUN   TestExecResize/invalid_size/empty_height
=== RUN   TestExecResize/invalid_size/empty_width
=== RUN   TestExecResize/invalid_size/non-numeric_height
=== RUN   TestExecResize/invalid_size/non-numeric_width
=== RUN   TestExecResize/unknown_execID
=== RUN   TestExecResize/invalid_state
--- PASS: TestExecResize (0.24s)
    --- PASS: TestExecResize/success (0.00s)
    --- PASS: TestExecResize/invalid_size (0.00s)
        --- PASS: TestExecResize/invalid_size/unset_height (0.00s)
        --- PASS: TestExecResize/invalid_size/unset_width (0.00s)
        --- PASS: TestExecResize/invalid_size/empty_height (0.00s)
        --- PASS: TestExecResize/invalid_size/empty_width (0.00s)
        --- PASS: TestExecResize/invalid_size/non-numeric_height (0.00s)
        --- PASS: TestExecResize/invalid_size/non-numeric_width (0.00s)
    --- PASS: TestExecResize/unknown_execID (0.00s)
    --- PASS: TestExecResize/invalid_state (0.06s)
=== RUN   TestResize
=== RUN   TestResize/success
=== RUN   TestResize/invalid_size
=== RUN   TestResize/invalid_size/unset_height
=== RUN   TestResize/invalid_size/unset_width
=== RUN   TestResize/invalid_size/empty_height
=== RUN   TestResize/invalid_size/empty_width
=== RUN   TestResize/invalid_size/non-numeric_height
=== RUN   TestResize/invalid_size/non-numeric_width
=== RUN   TestResize/invalid_state
--- PASS: TestResize (0.39s)
    --- PASS: TestResize/success (0.18s)
    --- PASS: TestResize/invalid_size (0.19s)
        --- PASS: TestResize/invalid_size/unset_height (0.00s)
        --- PASS: TestResize/invalid_size/unset_width (0.00s)
        --- PASS: TestResize/invalid_size/empty_height (0.00s)
        --- PASS: TestResize/invalid_size/empty_width (0.00s)
        --- PASS: TestResize/invalid_size/non-numeric_height (0.00s)
        --- PASS: TestResize/invalid_size/non-numeric_width (0.00s)
    --- PASS: TestResize/invalid_state (0.01s)
PASS

DONE 21 tests in 1.816s

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

@thaJeztah
Copy link
Copy Markdown
Member Author

One failure on Windows; I saw other exec-tests were skipped on Windows, so maybe it's the same issue;

=== FAIL: github.com/docker/docker/integration/container TestExecResize/success (0.01s)
    exec_test.go:139: assertion failed: error is not nil: Error response from daemon: FailedPrecondition: exec: '772c53f87fc5a86f41cfed8f974f279899f953cc2a4d9363819345d368742115' in task: '2c6aa47aeadb63334a57571f2e4bbe951719d0ac915d4c45c0d29a65f6bf881a' is not a tty: failed precondition

=== FAIL: github.com/docker/docker/integration/container TestExecResize (3.42s)

@thaJeztah
Copy link
Copy Markdown
Member Author

One more difference on Windows; stopping the container results in the Exec to be "not found" on the builtin driver;

=== RUN   TestExecResize/invalid_state
    exec_test.go:234: assertion failed: error is Error response from daemon: No such exec instance: cc728a332d3f594249fb7ee9adb3bb12a59a5d1776f8f6dedc56355364361711 (errdefs.errNotFound), not errdefs.IsConflict
    exec_test.go:235: assertion failed: expected error to contain "is not running", got "Error response from daemon: No such exec instance: cc728a332d3f594249fb7ee9adb3bb12a59a5d1776f8f6dedc56355364361711"
        Error response from daemon: No such exec instance: cc728a332d3f594249fb7ee9adb3bb12a59a5d1776f8f6dedc56355364361711

I can adjust the test; will have to wait for CI to complete to verify if it does the same with containerd as runtime.

Add tests for various (invalid) sizes for resizing.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
Add integration tests for resizing exec's.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah thaJeztah requested a review from vvoland October 15, 2024 16:37
Copy link
Copy Markdown
Contributor

@vvoland vvoland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, left some nits

containertypes "github.com/docker/docker/api/types/container"
"github.com/docker/docker/errdefs"
"github.com/docker/docker/integration/internal/container"
req "github.com/docker/docker/testutil/request"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need the req alias?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, we don't; looks like this was because I copy/pasted bits in this file from the other one.

I'm working on a follow-up; I'll fix it up for both files there

doc: "unset height",
height: valueNotSet,
width: "100",
expErr: `strconv.Atoi: parsing "": invalid syntax`,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

out of scope of this PR, but would be nice to have a better message here 😅

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Working on that exactly!

@thaJeztah thaJeztah merged commit f96c059 into moby:master Oct 16, 2024
@thaJeztah thaJeztah deleted the integration_resize branch October 16, 2024 10:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants