Skip to content

Conversation

@kanongil
Copy link
Contributor

@kanongil kanongil commented Jan 6, 2021

Any test configured with a retry more than 1 will erroneously re-run the test logic until all tries are used.

The issue is fixed with this PR.

@kanongil kanongil changed the title Fix retries of passing tests Fix retrying of passing tests Jan 6, 2021
Copy link
Member

@devinivy devinivy left a comment

Choose a reason for hiding this comment

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

Well, that was a funny bug! Thanks for catching and fixing it.

@devinivy devinivy added the bug Bug or defect label Jan 6, 2021
@devinivy devinivy added this to the 24.1.1 milestone Jan 6, 2021
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

😄

@kanongil
Copy link
Contributor Author

kanongil commented Jan 6, 2021

Funny, yeah - This is a class of bug that tests likely won't catch. The code appears to run as expected, but is just a bit slow. At least it was simple enough to write a failing test.

Though in this case, it was not only slow. Given that the test code would be run after the teardown in finish(), I expect that it could be the cause of some weird issues. In this case I discovered it since all the catbox policy tests were reporting 2 tries, when I would only ever expect 1, and it in fact used 5.

@devinivy devinivy merged commit 47cf441 into hapijs:master Jan 6, 2021
@lloydbenson
Copy link
Contributor

This was a great find. Thanks @kanongil.

@kanongil
Copy link
Contributor Author

Can you do a patch release?

@devinivy
Copy link
Member

@kanongil consider it done 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Bug or defect

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants