Skip to content

integration-cli: Enable TestGetContainersAttachWebsocket for Windows#43866

Merged
thaJeztah merged 1 commit intomoby:masterfrom
olljanat:win-enable-attach-websocket
Jul 26, 2022
Merged

integration-cli: Enable TestGetContainersAttachWebsocket for Windows#43866
thaJeztah merged 1 commit intomoby:masterfrom
olljanat:win-enable-attach-websocket

Conversation

@olljanat
Copy link
Copy Markdown
Contributor

@olljanat olljanat commented Jul 25, 2022 •
edited
Loading

- What I did
I have been trying to figure out why TestExecWithCloseStdin test hangs forever in Windows + containerd combination and as part of it find out that there is other good attach tests which are not currently enabled on Windows. So this PR enables TestGetContainersAttachWebsocket

- How I did it
Disabled TTY because is.DeepEqual looks to be comparing hex decimals and which does not work even without containerd on Windows because of different encoding/etc

For record error before disabling TTY was (or that looks to be from my test where message was "hello+r

=== RUN   TestDockerAPISuite/TestGetContainersAttachWebsocket
    docker_api_attach_test.go:73: assertion failed: 
        --- actual
        +++ expected
          []uint8{
        -       0x1b, 0x5b, 0x32, 0x4a, 0x1b, // -|.[2J.|
        +       0x68, 0x65, 0x6c, 0x6c, 0x6f, // +|hello|
          }
        : Websocket didn't return the expected data

For the record. I did also investigate TestPostContainersAttach but there even non-TTY tests looks to be problematic because those tests contains line breaks which does not end up to actual.

- How to verify it
Should be good if CI passes on all combinations.

- Description for the changelog

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

cpuguy83
cpuguy83 approved these changes Jul 25, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

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

LGTM assuming everything is happy.

@corhere
Copy link
Copy Markdown
Contributor

corhere commented Jul 25, 2022

The "different encoding" is actually ANSI/VT terminal commands embedded in the output. The sequence 0x1b, 0x5b, 0x32, 0x4a is the "clear entire screen" command. We have termtest.StripANSICommands to help with comparing TTY-enabled output.

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Jul 26, 2022

I think it's ok for the scope of this test to remove the TTY

But (as a follow-up) we should consider testing both with and without a TTY attached. wsContainersAttach calls out to ContainerAttach

moby/api/server/router/container/container_routes.go

Line 756 in cf8b057

err = s.backend.ContainerAttach(containerName, attachConfig)

which contains logic based on wether the container has a TTY or not;

moby/daemon/attach.go

Lines 53 to 54 in cf8b057

multiplexed := !ctr.Config.Tty && c.MuxStreams
inStream, outStream, errStream, err := c.GetStreams(multiplexed)

🤔 So, I guess that's covered in TestPostContainersAttach, which also is disabled on Windows (perhaps for the exact reason mentioned above?);

moby/integration-cli/docker_api_attach_test.go

Lines 98 to 99 in a61f7ab

func (s *DockerAPISuite) TestPostContainersAttach(c *testing.T) {
testRequires(c, DaemonIsLinux)

When fixing that, we should also consider rewriting that test to use subtests, as it looks like it's testing quite some variations.

@thaJeztah thaJeztah added this to the v-next milestone Jul 26, 2022
thaJeztah
thaJeztah approved these changes Jul 26, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

(but we should look at fixing some of these in follow-ups; see my comment above)

@thaJeztah thaJeztah mentioned this pull request Jul 26, 2022
Open
@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Jul 26, 2022

CI is happy; let me get this one in; I created #43868 for tracking the comment above

@thaJeztah thaJeztah merged commit 2bfc7ae into moby:master Jul 26, 2022
crazy-max pushed a commit to crazy-max/moby that referenced this pull request Sep 29, 2022
@thaJeztah @crazy-max
Merge pull request moby#43866 from olljanat/win-enable-attach-websocket
922cf68 
integration-cli: Enable TestGetContainersAttachWebsocket for Windows
Signed-off-by: CrazyMax <[email protected]>
@olljanat olljanat mentioned this pull request Jan 19, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cpuguy83 cpuguy83 cpuguy83 approved these changes

@thaJeztah thaJeztah thaJeztah approved these changes

Assignees

No one assigned

Labels

area/testing platform/windows status/2-code-review

Projects

None yet

Milestone

24.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@olljanat @corhere @thaJeztah @cpuguy83