Skip to content

libnet: de-flake TestEndpointStore and TestNetworkStore#49764

Merged
vvoland merged 3 commits intomoby:masterfrom
akerouanton:fix-TestNetworkStore-TestEndpointStore
Apr 7, 2025
Merged

libnet: de-flake TestEndpointStore and TestNetworkStore#49764
vvoland merged 3 commits intomoby:masterfrom
akerouanton:fix-TestNetworkStore-TestEndpointStore

Conversation

@akerouanton
Copy link
Member

Related to:

- What I did

- How to verify it

Verified both tests with -test.count 10000:

$ TESTDIRS='./libnetwork' TESTFLAGS='-test.run TestEndpointStore -test.count 10000' hack/test/unit
$ TESTDIRS='./libnetwork' TESTFLAGS='-test.run TestNetworkStore -test.count 10000' hack/test/unit

@akerouanton akerouanton added this to the 28.1.0 milestone Apr 7, 2025
@akerouanton akerouanton self-assigned this Apr 7, 2025
Comment on lines +43 to +44
assert.Check(t, found[0] == ep1, "got: %s; expected: %s", found[0].id, ep1.id)
assert.Check(t, found[1] == ep2, "got: %s; expected: %s", found[1].id, ep2.id)
Copy link
Contributor

Choose a reason for hiding this comment

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

using is.Equal saves you from writing the message :)

Suggested change
assert.Check(t, found[0] == ep1, "got: %s; expected: %s", found[0].id, ep1.id)
assert.Check(t, found[1] == ep2, "got: %s; expected: %s", found[1].id, ep2.id)
assert.Check(t, is.Equal(found[0], ep1))
assert.Check(t, is.Equal(found[1], ep2))

Copy link
Member Author

Choose a reason for hiding this comment

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

is.Equal is probably more idiomatic but unfortunately it's going to dump the whole struct which isn't particularly readable. So I'll switch to it, but keep the custom message.

I could compare the endpoint ID instead, but one of the goal of this test is to make sure findEndpoints returns the original values (and not copy). I'll add a comment to clarify that.

Copy link
Contributor

Choose a reason for hiding this comment

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

one of the goal of this test is to make sure findEndpoints returns the original values (and not copy)

The comment says "and that the returned values are not copies of the original ones" - which seems like the opposite at first glance.

So, is the intention to check the pointers are the same (there's a single Endpoint object), rather than being pointers to different Endpoint objects that have the same values?

I think is.Equal will compare the pointed-to values if the pointers are different. So, it probably needs to go back to the original pointer equality?

(But then, if the pointers are to different objects with the same id value ... the error message will be confusing because the two ids will be the same?)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm talking nonsense, it's not DeepEqual - ignore me!

Copy link
Contributor

Choose a reason for hiding this comment

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

(Although, the printed id values are likely to be the same even if the pointers are different?)

Comment on lines +43 to +44
assert.Check(t, found[0] == ep1, "got: %s; expected: %s", found[0].id, ep1.id)
assert.Check(t, found[1] == ep2, "got: %s; expected: %s", found[1].id, ep2.id)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this would work? ...

Suggested change
assert.Check(t, found[0] == ep1, "got: %s; expected: %s", found[0].id, ep1.id)
assert.Check(t, found[1] == ep2, "got: %s; expected: %s", found[1].id, ep2.id)
assert.Check(t, is.Equal(found[0], ep1))
assert.Check(t, is.Equal(found[1], ep2))

Copy link
Member

Choose a reason for hiding this comment

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

I think this was done to use the custom message to prevent the whole ep being dumped as "not equal".

Possibly a custom message would work in that case (but not sure if is.Equal would still dump the whole ep);

	assert.Check(t, is.Equal(found[0], ep1), "got: %s; expected: %s", found[0].id, ep1.id)

@robmry
Copy link
Contributor

robmry commented Apr 7, 2025

Ohh! Well, at least we all agree!

Signed-off-by: Albin Kerouanton <albinker@gmail.com>
Signed-off-by: Albin Kerouanton <albinker@gmail.com>
@akerouanton akerouanton force-pushed the fix-TestNetworkStore-TestEndpointStore branch from 87be15d to 7c99f24 Compare April 7, 2025 14:22
Signed-off-by: Albin Kerouanton <albinker@gmail.com>
@akerouanton akerouanton force-pushed the fix-TestNetworkStore-TestEndpointStore branch from 7c99f24 to 4eebd2c Compare April 7, 2025 14:25
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

@vvoland vvoland merged commit d7d91b6 into moby:master Apr 7, 2025
148 checks passed
@akerouanton akerouanton deleted the fix-TestNetworkStore-TestEndpointStore branch April 7, 2025 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants