Skip to content

Conversation

@robmry
Copy link
Contributor

@robmry robmry commented Feb 14, 2024

- What I did

While looking at #47318, I wondered why the change hadn't caused test TestDockerNetworkMacvlan/OverlapParent to fail...

The macvlan/ipvlan tests haven't been running since a hiccup in #45652.

- How I did it

Call the test functions.

- How to verify it

Tests now run.

- Description for the changelog

Fixed integration tests for ipvlan/macvlan.

@robmry robmry self-assigned this Feb 14, 2024
@robmry robmry added area/networking Networking kind/bugfix PR's that fix bugs labels Feb 14, 2024
@thaJeztah
Copy link
Member

Ugh. Nice catch 😓 😅

Wondering if (as a follow-up) we should remove the extra indirect of the closure, and make the signature of those subtests less error-prone, e.g. something like;

func testIpvlanSubinterface(t *testing.T, ctx context.Context, client dclient.APIClient) {

Copy link
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

@robmry
Copy link
Contributor Author

robmry commented Feb 14, 2024

Wondering if (as a follow-up) we should remove the extra indirect of the closure, and make the signature of those subtests less error-prone, e.g. something like;

Sure - can do ... I'll add another commit to this PR, it might kick the flaky test into working.

@thaJeztah
Copy link
Member

Yeah, I like the minimal changes if we want to backport this, but for "master" at least (I think) changing the signature may make it less error prone (that's just at a quick glance though, so take pinches of salt if you don't agree!)

Copy link
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, thanks!

for others; the diff is mostly whitespace, and much easier to review with whitespace changes removed; add ?w=1 to the "changes" URL for that; https://github.com/moby/moby/pull/47382/files?w=1

The problem was accidentally introduced in:
  e8dc902

Signed-off-by: Rob Murray <rob.murray@docker.com>
Signed-off-by: Rob Murray <rob.murray@docker.com>
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.

3 participants