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

feat: implement repeats #5011

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: implement repeats #5011

wants to merge 1 commit into from

Conversation

mmomtchev
Copy link

@mmomtchev mmomtchev commented Sep 13, 2023

Refs: Re-run a test multiple times / fixes #2332

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions.

Description of the Change

Implements a this.repeats - a very useful feature judging by the number of thumbs up on the issue #2332

Alternate Designs

This design follows very closely the already existing retries feature

Why 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:

  • testing of tear-down procedures
  • testing for leaks

Possible Drawbacks

This design follows very closely the tried and tested retries feature so the risks should be minimal

Applicable issues

#2332

Refs: Re-run a test multiple times mochajs#2332
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Sep 13, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: mmomtchev / name: Momtchil Momtchev (8cd9463)

@falsandtru
Copy link

Can you add the current iteration number to the context? Reproducible tests require reproducible seeds.

const rng = xorshift.random(this.currentIteration + 1)

@JoshuaKGoldberg JoshuaKGoldberg added the semver-major implementation requires increase of "major" version number; "breaking changes" label Mar 4, 2024
Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

🔥 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);
Copy link
Member

Choose a reason for hiding this comment

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

[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?

Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Member

Choose a reason for hiding this comment

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

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.
Copy link
Member

Choose a reason for hiding this comment

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

[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!

Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

(I'd hit the wrong button first)

@JoshuaKGoldberg JoshuaKGoldberg added the status: waiting for author waiting on response from OP - more information needed label Mar 4, 2024
@JoshuaKGoldberg JoshuaKGoldberg changed the title implement repeats feat: implement repeats Mar 4, 2024
@JoshuaKGoldberg JoshuaKGoldberg added status: needs review a maintainer should (re-)review this pull request and removed status: waiting for author waiting on response from OP - more information needed labels Aug 6, 2024
@JoshuaKGoldberg JoshuaKGoldberg requested a review from a team August 6, 2024 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-major implementation requires increase of "major" version number; "breaking changes" status: needs review a maintainer should (re-)review this pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🚀 Feature: Re-run a test multiple times: this.repeat
3 participants