Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

tests(rust): fix flaky transport tests #2726

Merged
merged 4 commits into from
May 16, 2022
Merged

tests(rust): fix flaky transport tests #2726

merged 4 commits into from
May 16, 2022

Conversation

adrianbenavides
Copy link
Member

Transport tests that were using a random port to create the "listener" sides were failing when the selected port was already used by another process. These changes add a loop to retry binding a transport until it succeeds.

It also adds a small change to the test macro so that we don't have to explicitly stop the context at the end of every test.

Fixes #2720

@adrianbenavides adrianbenavides requested a review from a team as a code owner May 9, 2022 09:43
@adrianbenavides adrianbenavides force-pushed the adrian/tests-fix-transport-flaky-tests branch from 23a7535 to 51a548b Compare May 9, 2022 09:44
@twittner
Copy link
Contributor

twittner commented May 9, 2022

Can we not have the operating system assign us a port number by using 0?

@adrianbenavides
Copy link
Member Author

Can we not have the operating system assign us a port number by using 0?

We should be able to by modifying the listen and create_outlet methods on the transports crates to return the assigned address. That would definitely be an improvement. Maybe we could leave this for a separate PR? Just to ensure that the tests are stable now without changing any production code.

Thanks for the tip @twittner!

@adrianbenavides adrianbenavides force-pushed the adrian/tests-fix-transport-flaky-tests branch 2 times, most recently from de092fe to c960373 Compare May 9, 2022 10:24
@adrianbenavides
Copy link
Member Author

adrianbenavides commented May 9, 2022 •
edited
Loading

I've added the changes suggested by @twittner to remove the "random port" logic altogether.

@SanjoDeundiak
Copy link
Member

How do we manage to collide picking random port from 10000 to 65535?
P.S. Can we reduce the scope of this PR to fixing tests only? Would be glad to discuss macro update in separate PR

@adrianbenavides adrianbenavides force-pushed the adrian/tests-fix-transport-flaky-tests branch 2 times, most recently from e7de132 to 1dc5665 Compare May 11, 2022 05:46
@adrianbenavides
Copy link
Member Author

adrianbenavides commented May 11, 2022 •
edited
Loading

How do we manage to collide picking random port from 10000 to 65535?

In my case, I had some os processes running on ports 19455, 42849 and 50586. I don't know if a similar case could happen in CI, but knowing what ports are available is out of our control I guess, so it should be best if we can assign a port automatically by the os. Also if tests were to be run in parallel, chances would definitely be there, although this fix won't solve the problem toralf outlined here: #2730.

P.S. Can we reduce the scope of this PR to fixing tests only? Would be glad to discuss macro update in separate PR

Sure, done!

twittner
twittner previously approved these changes May 11, 2022
View reviewed changes
Copy link
Contributor

@twittner twittner left a comment

Choose a reason for hiding this comment

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

👍

implementations/rust/ockam/ockam_transport_tcp/tests/send_receive.rs Outdated Show resolved Hide resolved
implementations/rust/ockam/ockam_transport_tcp/tests/send_receive.rs Show resolved Hide resolved
spacekookie
spacekookie previously approved these changes May 11, 2022
View reviewed changes
SanjoDeundiak
SanjoDeundiak previously requested changes May 11, 2022
View reviewed changes
Copy link
Member

@SanjoDeundiak SanjoDeundiak left a comment

Choose a reason for hiding this comment

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

Looks great, one fix is required though

implementations/rust/ockam/ockam_transport_tcp/src/transport.rs Outdated Show resolved Hide resolved
@adrianbenavides adrianbenavides dismissed stale reviews from spacekookie and twittner via e4c823e May 11, 2022 13:44
twittner
twittner previously approved these changes May 11, 2022
View reviewed changes
@mrinalwadhwa mrinalwadhwa self-requested a review May 15, 2022 23:39
mrinalwadhwa
mrinalwadhwa requested changes May 15, 2022
View reviewed changes
Copy link
Member

@mrinalwadhwa mrinalwadhwa left a comment

Choose a reason for hiding this comment

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

@adrianbenavides It looks like commitlint ignores commits of the form:

Revert "test(rust): fix flaky transport tests"

That was surprising to me. We should add some overriding lints tests for it.

Since in out workflow commits get rebased and moved around, its better to avoid referring to older commits and instead just describe the change that is in a patch/commit.

@adrianbenavides
test(rust): fix flaky transport tests
00082e5 
Transport tests that were using a random port to create the "listener" sides were
failing when the selected port was already used by another process. These changes
add a loop to retry binding a transport until it succeeds.

It also adds a small change to the `test` macro so that we don't have to explicitly
stop the context at the end of every test.
@adrianbenavides
feat(rust): return socket address when starting a transport listener
b5d0812 
To reduce boilerplate in tests to get a random port to bind to,
now the router's `bind` method return the socket address that was used.

This can be useful when binding to port 0.
@adrianbenavides adrianbenavides force-pushed the adrian/tests-fix-transport-flaky-tests branch from e4c823e to c85ea3b Compare May 16, 2022 08:08
@adrianbenavides adrianbenavides force-pushed the adrian/tests-fix-transport-flaky-tests branch from c85ea3b to 089d48c Compare May 16, 2022 08:44
@adrianbenavides
Copy link
Member Author

adrianbenavides commented May 16, 2022 •
edited
Loading

@adrianbenavides It looks like commitlint ignores commits of the form:

Revert "test(rust): fix flaky transport tests"

That was surprising to me. We should add some overriding lints tests for it.

Since in out workflow commits get rebased and moved around, its better to avoid referring to older commits and instead just describe the change that is in a patch/commit.

Yeah, I didn't think about changing the auto-generated message after the revert. I've changed it now!

@mrinalwadhwa mrinalwadhwa requested review from SanjoDeundiak and removed request for SanjoDeundiak May 16, 2022 13:46
@mrinalwadhwa mrinalwadhwa dismissed SanjoDeundiak’s stale review May 16, 2022 13:52

try to see what is the best way one approver can grant an okay after changes were requested by another approver.

@mergify mergify bot merged commit ac2308d into develop May 16, 2022
@mrinalwadhwa mrinalwadhwa deleted the adrian/tests-fix-transport-flaky-tests branch March 9, 2023 07:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrinalwadhwa mrinalwadhwa mrinalwadhwa approved these changes

@twittner twittner twittner left review comments

@spacekookie spacekookie spacekookie left review comments

@SanjoDeundiak SanjoDeundiak Awaiting requested review from SanjoDeundiak

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Failing test in websocket transport
5 participants
@adrianbenavides @twittner @SanjoDeundiak @mrinalwadhwa @spacekookie