✨ Support for beforeEach/afterEach in @fast-check/vitest - #6587
✨ Support for beforeEach/afterEach in @fast-check/vitest#6587rushelex wants to merge 17 commits into
beforeEach/afterEach in @fast-check/vitest#6587Conversation
- Add explicit undefined checks for test and suite - Replace truthy checks with explicit undefined comparisons - Use nullish coalescing operator (??) instead of logical OR - Simplify afterEach hook reversal logic - Add whitespace for better code readability - Add comment explaining undefined test/suite cases
- Replace while(current) with while(true) and explicit termination - Add type annotation for current variable - Add comment explaining suite chain traversal logic - Clarify that loop terminates at File node with filepath
- Collect cleanup functions returned by beforeEach hooks - Execute cleanups before afterEach hooks between runs - Run remaining cleanups after final property iteration - Match vitest's cleanup-before-afterEach execution order
🦋 Changeset detectedLatest commit: 7e308e8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
dubzzz
left a comment
dubzzz
left a comment
There was a problem hiding this comment.
Overall the PR looks good to me. I just did a quick pass on it. Starting with vitest one but comments would probably be the same in the other one. Let's wait for this one to be validated and merged before replicating the same changes on the other so that I don't make you lose time with fixing both at the same time.
| // Run remaining cleanup functions from the last manual beforeEach call. | ||
| // Vitest supports returning a cleanup function from beforeEach; the first | ||
| // iteration's cleanup is handled by vitest's own afterEach lifecycle, but | ||
| // cleanups from iterations 2..N are captured and invoked by us. | ||
| for (let i = pendingCleanups.length - 1; i >= 0; i--) { | ||
| await pendingCleanups[i](); | ||
| } | ||
| }, |
There was a problem hiding this comment.
So if I get it well, clean-ups of the first beforeEach will be executed at the end by vitest. This manual clean-up is cleaning the last run within fast-check (if there was one not being the first).
There was a problem hiding this comment.
Said differently we might have:
- beforeEach#1 done by vitest
- beforeEach#2 done by beforeEach of fast-check
- cleanUp of beforeEach#2 done by beforeEach of fast-check
- beforeEach#3 done by beforeEach of fast-check
- cleanUp of beforeEach#3 done by the manual clean-up loop after assert
- cleanUp of beforeEach#1 done by vitest
There was a problem hiding this comment.
Yes, you're right. Cleanup#1 is held by vitest in a local variable of runTest and runs after vitest's own afterEach — we cannot intercept it.
Co-authored-by: Nicolas DUBIEN <github@dubien.org>
| }); | ||
|
|
||
| it.concurrent( | ||
| `should never call beforeEach/afterEach twice per run in ${runnerName}.prop`, |
There was a problem hiding this comment.
Actually the test does not really does that. This one mostly checks that we don't have a too large diff between number of beforeEach runs vs number of afterEach runs
| }, | ||
| ); | ||
|
|
||
| it.concurrent( |
There was a problem hiding this comment.
IMO no real need to check for shrinking vs no-shrinking. Having the no-shrinking cover is good enough for this feature. So worth dropping this test case
| }, | ||
| ); | ||
|
|
||
| it.concurrent( |
There was a problem hiding this comment.
I'd rather group the two last tests into one doing something like:
Have an array being empty.
In beforeEach, push "beforeEach('runId')" in the array.
In run, push "predicate('runId')" in the array.
In clean-up of beforeEach, push "clean-up('runId')" in the array.
In afterEach, push "afterEach('runId')" in the array.
Maybe limit yourself to 3 runs and in afterAll expect an hardcoded array. Such test would have the benefit to capture all remaining things including relative ordering of calls and also the thing around clean-up of 1st run not being called exactly when it should be.
There was a problem hiding this comment.
Regarding my comment https://github.com/dubzzz/fast-check/pull/6587/changes#r2819152307, if we address it (maybe in another PR) it would be great to have a test with the same idea as the one described above but in the context of a failure. This time no way to hardcode the output but we could in the afterAll test its shape.
There was a problem hiding this comment.
@claude could you try to take that one? one a PR doing the fix I'm asking
| // fc's beforeEach hook above. Cleanup#1 is held by vitest in a local | ||
| // variable of runTest and runs after vitest's own afterEach — we cannot | ||
| // intercept it. | ||
| for (let i = pendingCleanups.length - 1; i >= 0; i--) { |
There was a problem hiding this comment.
With this trick, I have the feeling that we might not support correctly things such as:
beforeEach(() => {
console.log("before");
});
test("abc", () => {
console.log("test");
throw new Error("a");
});
afterEach(() => {
console.log("after");
});In such context vitest prints the three logs. In our case if fc.assert fails, I feel that we might not execute clean-ups properly...
There was a problem hiding this comment.
Not blocking at all for the first iteration on this feature
There was a problem hiding this comment.
@claude could you try to take that one? one a PR doing the fix I'm asking
Co-authored-by: Nicolas DUBIEN <github@dubien.org>
Co-authored-by: Nicolas DUBIEN <github@dubien.org>
Co-authored-by: Nicolas DUBIEN <github@dubien.org>
Co-authored-by: Nicolas DUBIEN <github@dubien.org>
Co-authored-by: Nicolas DUBIEN <github@dubien.org>
dubzzz
changed the title
beforeEach/afterEach in @fast-check/vitestbeforeEach/afterEach in @fast-check/vitest
|
Let's go for it! Thanks for the contribution. I'm gonna address on separate PRs the remaining points. |
@fast-check/ava
fast-check
@fast-check/jest
@fast-check/packaged
@fast-check/poisoning
@fast-check/vitest
@fast-check/worker
commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6587 +/- ##
==========================================
- Coverage 94.72% 94.72% -0.01%
==========================================
Files 209 209
Lines 5670 5666 -4
Branches 1502 1500 -2
==========================================
- Hits 5371 5367 -4
Misses 286 286
Partials 13 13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@rushelex I had to recreate the PR to deal with last few blockers and totally unlock it. The new PR is accessible at #6695. No worry you'll still be marked as author on the finally merged PR and still be added as a contributor to the repository. My plan is to address the few last TODOs: tests and bugs in term of errors before merging it. |
## Description <!-- Describe your change and explain what this PR is trying to solve --> Original PR with full description available at #6587. Fixes #3942 <!-- Add any additional context here --> ## Checklist — _Don't delete this checklist and make sure you do the following before opening the PR_ - [x] I have a full understanding of every line in this PR — whether the code was hand-written, AI-generated, copied from external sources or produced by any other tool - [x] I flagged the impact of my change (minor / patch / major) either by running `pnpm run bump` or by following the instructions from the changeset bot - [x] I kept this PR focused on a single concern and did not bundle unrelated changes - [x] I followed the [gitmoji](https://gitmoji.dev/) specification for the name of the PR, including the package scope (e.g. `🐛(vitest) Something...`) when the change targets a package other than `fast-check` - [x] I added relevant tests and they would have failed without my PR (when applicable) <!-- PRs not checking all the boxes may take longer before being reviewed --> <!-- More about contributing at https://github.com/dubzzz/fast-check/blob/main/CONTRIBUTING.md --> --------- Co-authored-by: Aleksey Shelementev <rushelex@gmail.com>
|
Closing as already merged |
2 participants
Description
This PR closes issue #3942.
This PR related to Jest PR #6586.
Currently, when using
it.prop/test.propin@fast-check/vitest,beforeEachandafterEachhooks are only called once per test — not once per property run. This means that if a property executes 100 runs inside a singleit.prop, hooks likebeforeEach(commonly used for setup/teardown, mocking, DB transactions, etc.) only fire once instead of 100 times.This PR makes
beforeEach/afterEachfire for each property run, not just once per test. For example, with 1 regularit()+ 1 regularit()+ 1it.prop()withnumRuns: 10,beforeEach/afterEachwill now fire 12 times total instead of 3.Strategy:
beforeEachbefore the test (covers 1st run)afterEach→beforeEachviapropertyInstance.beforeEach()hookafterEachafter the test (covers last run)beforeEach+ NafterEachfor N property runs@fast-check/vitest: Uses vitest internals (getCurrentSuite(),getHooks()) to collect hooks from the suite chain. Collects cleanup functions returned bybeforeEachhooks and executes them beforeafterEachhooks between runs, matching vitest's cleanup-before-afterEach execution order.Checklist — Don't delete this checklist and make sure you do the following before opening the PR
pnpm run bumpand flag the impacts properlyAdvanced