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.

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

chore: refactor flaky test with while #1698

Merged
merged 1 commit into from
Apr 25, 2023
Merged

chore: refactor flaky test with while #1698

merged 1 commit into from
Apr 25, 2023

Conversation

alrevuelta
Copy link
Contributor

Aims to fix couple of flaky tests by:

  • Increasing the time we wait before the assert.
  • Running a while loop with a max number of attempts. So instead of waiting a fixed amount of time, we set a maximum number of attempts before it fails. If the tests succeedes before, it shorten execution time.

Comment on lines +129 to +134
var attempts = 10
while (node1.wakuDiscv5.protocol.nodesDiscovered < 1 or
node2.wakuDiscv5.protocol.nodesDiscovered < 1) and
attempts > 0:
await sleepAsync(1.seconds)
attempts -= 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

That is great indeed.
I like the idea of having a more precise wait.
However, I would encapsulate it in a generic proc that waits for a certain condition to be true (20 attempts 0.5s each by default). If the max attempts is exceeded then fail the test in that point showing the condition didn't occur.

The important thing is to make sure the desired condition is met (require). If not, then notify it clearly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup agree, the thing is that it might be tricky to have a generic proc, since in some cases it can be one condition, in others multiple ones (like here). each condition(s) can have a different value etc.

also i prefer an explicit check value1 == value2 instead of a proc just returning true/false and then asserting for that, since when asserting for value1==value2, you get the actual values, which gives more granularity for debugging.

but for sure, once we start doing this for other tests, i think we can come up with something more fancy than this.

Copy link
Contributor

@jm-clius jm-clius left a comment

Choose a reason for hiding this comment

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

LGTM as an improvement.

Copy link
Member

@vpavlin vpavlin left a comment

Choose a reason for hiding this comment

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

lgtm

@alrevuelta alrevuelta merged commit dca0e9b into master Apr 25, 2023
@alrevuelta alrevuelta deleted the fix-flaky-test-3 branch April 25, 2023 07:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants