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

test: reset cucumber timeouts to practical limits #3511

Conversation

hansieodendaal
Copy link
Contributor

Description

Re-applied reasonable time outs to cucumber tests. It is not worthwhile
to have too stringent time outs as it makes the tests flaky on a slower
system and especially on CI.

Motivation and Context

Tests were flaky on slow and fast computers due to stringent time outs.

How Has This Been Tested?

Cucumber on slow and fast computers

Re-applied reasonable time outs to cucumber tests. It is not worthwhile
to have too stringent time outs as it makes the tests flaky on a slower
system and especially on CI.
@stringhandler
Copy link
Collaborator

Thanks for this, but it's going to be a nightmare if we keep going back and forward on these timeouts.

I suggest we do this:

  1. create constants 'SLOW','FAST' and set the timeouts in the steps to use them
  2. on CI, we set 'SLOW' = 60 seconds, FAST to 10 seconds using a world parameter, but by default they are set to 20 seconds, 1 second
  3. Remove all custom timeouts and only use SLOW, MEDIUM, FAST,

Alternatively we can set it to 5 * seconds() and use a speed factor in the world parameters

@@ -300,7 +300,7 @@ Given(

Given(
"I have {int} base nodes connected to all seed nodes",
{ timeout: 20 * 1000 },
{ timeout: 120 * 1000 },
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is too big a jump

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, that one is not needed to be so high

@@ -2903,7 +2910,7 @@ Then(

Then(
/all wallets detect all transactions as Mined_Confirmed/,
{ timeout: 40 * 1000 },
{ timeout: 1200 * 1000 },
Copy link
Collaborator

Choose a reason for hiding this comment

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

20 minutes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For stress tests

@stringhandler stringhandler added the P-do_not_merge Process - Not ready for merging label Oct 29, 2021
@hansieodendaal
Copy link
Contributor Author

Thanks for this, but it's going to be a nightmare if we keep going back and forward on these timeouts.

I suggest we do this:

  1. create constants 'SLOW','FAST' and set the timeouts in the steps to use them
  2. on CI, we set 'SLOW' = 60 seconds, FAST to 10 seconds using a world parameter, but by default they are set to 20 seconds, 1 second
  3. Remove all custom timeouts and only use SLOW, MEDIUM, FAST,

Alternatively we can set it to 5 * seconds() and use a speed factor in the world parameters

Let me think a bit about this and come up with a better solution than what we currently have. We have various value systems at play here; we do not want to wait a long time for a failing test, but we also do not want a test that will eventually succeed time out prematurely. This also behaves differently on slow and fast computers; the latter can many times be fixed with clever hold/synchronization points.

@stringhandler
Copy link
Collaborator

The CI passed using the previous values on #3494 and passed on 8 out of the 9 PR's following that (Github does not show circle ci results once it's merged, but I'm using the green and red ticks. They could have also failed on other checks. I'm excluding PRs to validator-node branch)

Given that 88% of the PR's passed, I would expect a far smaller adjustment to the timeouts

@stringhandler
Copy link
Collaborator

I think let's take this approach. As the tests fail, let's increase one or two steps at a time, as I have done in fad8520

@hansieodendaal
Copy link
Contributor Author

Ok, will make small PRs for the failing tests.

aviator-app bot pushed a commit that referenced this pull request Nov 8, 2021
Description
---
- Re-applied reasonable time outs to cucumber tests, using dynamic time-outs in test steps where possible.
- Fixed a broken test.
- Fixed some flaky tests.
- Removed some dead code.
- Removed duplicate test.

Note: Replaces #3511.

Motivation and Context
---
- Tests were flaky on slow and fast computers due to stringent time outs..
- Some test logic was flawed.
- Some flaky tests could be improved.

How Has This Been Tested?
---
Cucumber on slow and fast computers
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-do_not_merge Process - Not ready for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants