fix #14320 (tasyncawait.nim is recently very flaky) + avoid hardcoding service ports everywhere + flakyAssert #14327
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dom96
reviewed
May 12, 2020
timotheecour
force-pushed
the
pr_fix_14320_tasyncawait_flaky
branch
from
1e85a5a to
e26cfde
Compare
timotheecour
changed the title
|
Is this still a draft? |
ya i wanna see which tests fail first (eg: i'm re-enabling tests/async/tasyncssl.nim after those changes to see if it works). But feedback welcome |
timotheecour
force-pushed
the
pr_fix_14320_tasyncawait_flaky
branch
from
e5c99ad to
3481f10
Compare
timotheecour
changed the title
EchoPouet
pushed a commit
to EchoPouet/Nim
that referenced
this pull request
Jun 13, 2020
…ardcoding service ports everywhere + flakyAssert (nim-lang#14327) * hotfix nim-lang#14320 tasyncawait.nim is recently very flaky * fix nim-lang#14327 * add flakyAssert
This was referenced Jan 12, 2021
Merged
This was referenced Feb 26, 2021
1 task
This was referenced Apr 17, 2021
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Address already in useerrorsIIUC the reason tasyncawait starting giving
Address already in useerrors since 5/6/2020, 3:44:15 AM is that some unrelated test was added around that time and it surfaced a pre-existing time-window bug where 2 processes (testament runs tests in parallel) try to bind to same port; a lot of tests were using the same port10335, which gaveAddress already in useerror.This PR uses OS to get a port everywhere, and also refactors to remove some code duplication. Unfortunately a lot of tests use the "copy paste" approach, which, while quick and easy for the person who copy pastes, makes maintenance/evolving/bug fixing/increasing generality/etc hard. Eg, any bug fix or code evolution involves N times the amount of work after code is copy pasted N times. This is what happened in this PR.
Copy pasting (even test files) is almost always the wrong thing to do and should be rejected in code review in favor of refactoring as needed.
flakyAssert
this PR also adds
flakyAssert, a replacement forassertfor tests that are known to fail, see doc comments in PR + rationale. This sill show FLAKY_SUCCESS (with notifySuccess = true and on success) or FLAKY_FAILURE, along with context where it came from, and makes it easy to find in source code where a flake happens, so that it can eventually be fixed, and without disabling entire tests altogether. Testament may swallow the FLAKY_SUCCESS/FLAKY_FAILURE if the entire test succeeds, as usual, but in future work we can refine the logic (in just 1 place) to also log those messages to some file that can be shown in summary along with existing summary.