feat: implement repeats
#5011
feat: implement repeats
#5011
Conversation
Refs: Re-run a test multiple times mochajs#2332
|
|
|
Can you add the current iteration number to the context? Reproducible tests require reproducible seeds. const rng = xorshift.random(this.currentIteration + 1) |
JoshuaKGoldberg
added
the
semver-major
There was a problem hiding this comment.
🔥 great PR, thanks for sending @mmomtchev! The test coverage is thorough and the code changes are all clear & logical. Makes sense to me.
I think the only change request on my end is adding a new EVENT_TEST_REPEAT. Would love to hear your thoughts on that.
Note that per #5027 we're trying to ease into big changes. Since this one touches so much we're going to have to wait a while to get through more patch & minor level changes first.
| repeatedTest.currentRepeat(test.currentRepeat() + 1); | ||
| tests.unshift(repeatedTest); | ||
|
|
||
| self.emit(constants.EVENT_TEST_RETRY, test, null); |
There was a problem hiding this comment.
[Refactor] 🤔 I think a "retry" is different from a "repeat" as designed. I think we'd want a constants.EVENT_TEST_REPEAT so hooks can differentiate. Thoughts?
There was a problem hiding this comment.
I agree (especially since EVENT_TEST_RETRY is supposed to have a mandatory Error argument) - but I wonder whether there should be a special event. You will get the EVENT_TEST_BEGIN / EVENT_TEST_END / EVENT_TEST_PASS chain anyway. Should there be another event after these when a test is to be repeated? Should it come in place of the EVENT_TEST_PASS?
There was a problem hiding this comment.
Hmm yeah that's a good point. I think it'd be reasonable to leave out the event for now. We can always add it in if folks ask. Taking one out is much harder.
Will also want to hear from @mochajs/maintenance-crew on this (and the PR in general, per the review note).
There was a problem hiding this comment.
Oh, and per #5011: if there's no event then I think it'd be especially valuable to make sure test contexts have the current repeat count.
|
|
||
| This feature does re-run a passed test and its corresponding `beforeEach/afterEach` hooks, but not `before/after` hooks. | ||
|
|
||
| If using both `repeat` and `retries`, the test will be run `repeat` times tolerating up to `retries` failures in total. |
There was a problem hiding this comment.
[Praise] Very good line to have. The distinction between the repeat and retries could be tricky for folks to understand. I like having this clear explanation!
JoshuaKGoldberg
added
the
status: waiting for author
JoshuaKGoldberg
added
status: needs review
Refs: Re-run a test multiple times / fixes #2332
Requirements
Description of the Change
Implements a
this.repeats- a very useful feature judging by the number of thumbs up on the issue #2332Alternate Designs
This design follows very closely the already existing
retriesfeatureWhy should this be in core?
It is a core feature that is difficult to implement in a plugin
Benefits
It is a useful feature that allows:
Possible Drawbacks
This design follows very closely the tried and tested
retriesfeature so the risks should be minimalApplicable issues
#2332