Throw error when running vitest with no afterEach global and no VTL_SKIP_AUTO_CLEANUP

[Maxim-Mazurok]

This partially addresses #296 and will make more sense after docs are updated

[afontcu]

Looking good! I'd love to have some tests that prove that the behavior works as expected

[Maxim-Mazurok]

Thank you!
I've added a few tests that should be enough.
The best approach would be to have some tests that are run with jest and some tests that are run with vitest, but I'm afraid that would require too much disruption in a library I'm barely familiar with...

[lmiller1990]

@afontcu good to merge this up?

[afontcu]

Hey! sorry for the delay.

Taking a look back at this, I'm not 100% sure about throwing an error if people use Vite without globals. What if they prefer not to use them? Vite offers that option for a reason, and I don't think Testing Lib should be enforcing a specific config (that is not actually mandatory to function). Maybe adding docs is good enough?

Is there a way to enable auto-cleanup in Vite without requiring user config, as we do in Jest?

[Maxim-Mazurok]

What if they prefer not to use them?

In that case, they have an option to explicitly acknowledge that they're missing out on the auto-cleanup by setting VTL_SKIP_AUTO_CLEANUP env variable.

Maybe adding docs is good enough?

I would argue that it isn't, because people migrating from Jest to Vitest would expect it to work the same as it was working before and might not notice that.

Is there a way to enable auto-cleanup in Vite without requiring user config, as we do in Jest?

I don't think so, because the current Jest approach relies on global hooks being exposed.

[afontcu]

Hi folks, I know this issue has been here for a few months, I was away for a while but now I'm back.

So, I wanted to:

• Double check if the fix is still valid nowadays. If so, I'll go ahead and merge it.
• Discuss if this should be labeled as a major release as it includes a breaking change – I'd love to avoid that, but seems like some people might have been relying on previous behavior (even if not expected) and adding a throw there might break their current suites. wdyt?

[Maxim-Mazurok]

• Double check if the fix is still valid nowadays. If so, I'll go ahead and merge it.

I haven't checked, but doubt any of this changed so issue is likely to be still valid.

• Discuss if this should be labeled as a major release as it includes a breaking change – I'd love to avoid that, but seems like some people might have been relying on previous behavior (even if not expected) and adding a throw there might break their current suites. wdyt?

I can see how this can be a breaking change, yeah. Most quality-focused teams would probably like to be notified about this issue tho. Maybe by deprecating previous versions. Maybe it can be made a warning in this major, and then an error in the next major. Up to you.

[github-actions]

🎉 This PR is included in version 8.0.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[afontcu]

Released a new major to include this PR as is – thanks for working on this, @Maxim-Mazurok!

[oskarols]

@Maxim-Mazurok Hi, shouldn't the condition introduced here also contain a check for whether process.env.VTL_SKIP_AUTO_CLEANUP is not set? I might be misunderstanding something here but it seems to me there is no way to suppress this error message in Vitest (if one does not use globals).

[Maxim-Mazurok]

@Maxim-Mazurok Hi, shouldn't the condition introduced here also contain a check for whether process.env.VTL_SKIP_AUTO_CLEANUP is not set? I might be misunderstanding something here but it seems to me there is no way to suppress this error message in Vitest (if one does not use globals).

Yeah, that's a good point, looks like you do have to enable globals in order to skip cleanup.

I don't see why would anyone want to skip it tho. Having tests that affect each other is pretty bad, imagine seeing errors when running all tests, and then not seeing errors when trying to debug failing test, that would be a headscratcher. So the main value of my PR was to alert developers that expected auto-cleanup doesn't work without globals.

One could add an option to skip cleanup without globals, probably would be good to add a test similar to the ones added in this PR, and it should be a pretty straightforward task. I don't really want to do it as I don't see value in that, but I guess it won't hurt.

[ITenthusiasm]

@afontcu @Maxim-Mazurok Sorry to pop-in after the fact. I wasn't aware of this PR until I saw the major version bump in @testing-library/vue. I'm actually with Adrià's earlier comment on this one. 😅

What if they prefer not to use them? Vite offers that option for a reason, and I don't think Testing Lib should be enforcing a specific config.

I agree. Granted, some people coming to Vitest are migrating from Jest, yes. But Vitest (at least currently) seems to have its own philosophy. Part of that is seen in how Promises returned from mocked functions behave differently from how they do in Jest. (This actually provides a better DX in the end.) Another way in which that's seen is by the fact that -- at least for now -- globals are intentionally not enabled by default; the developer has to enable them if they want to. Explicit imports for the testing functions have their own benefits, and I'm coming to like them.

I am quite content to use the cleanup function manually. And sometimes it makes more sense to do so. For instance, if VTL is only used in certain parts of a testing file. Throwing an error in response to a developer config is too aggressive of a change, in my opinion. A warning would be much more friendly (though potentially too invasive). And I think the most friendly approach would be a simple update to the VTL docs -- which was suggested earlier.

When I was testing the Svelte integration of my Form Observer package, I found STL's Documentation perfectly sufficient to clarify what I needed. I think devs should expect that there will be minor nuances as they change between frameworks and/or testing tools. It comes with the territory of migration.

In that case, they have an option to explicitly acknowledge that they're missing out on the auto-cleanup by setting VTL_SKIP_AUTO_CLEANUP env variable.

This is not the intention of the *_SKIP_AUTO_CLEANUP env variable, and it's not how other tools in the TL family behave. The intent of the variable is to opt out of auto cleanup if the developer wishes to do so for whatever reason. But my understanding is that the intent is not to save the developer from unexpectedly-thrown errors. Those shouldn't be thrown to begin with.

[Maxim-Mazurok]

IMO most developers using VTL will expect auto-cleanup to work out of the box, so the goal was to alert them when auto-cleanup (which is enabled by default) can't happen (due to lack of globals).

For those rare cases when people don't care about auto-cleanup - it's fair to ask them to disable it explicitly IMO. VTL_SKIP_AUTO_CLEANUP should've allowed you to disable auto-cleanup so that globals aren't needed anymore. I failed to deliver that feature in this PR, so I think it's best to create a separate PR/issue to solve this, should be an easy fix, just change conditions a bit.

I think it makes sense, if auto-cleanup is enabled - it needs globals. If there are no globals - it can't work, so error should occur. Because you expect system to use auto-cleanup but it can't.

Hope this helps, cheers!

[ITenthusiasm]

I understand your line of reasoning. But my concern is that this breaking change:

1. Doesn't provide the friendliest developer experience (particularly because of the thrown errors that prevent tests from running).
2. Behaves differently from the other Testing Library Tools.
3. Is out of sync with how Vitest behaves by default. (Jest uses globals by default, so expecting globals in Jest envs makes sense. But Vitest doesn't use globals by default, so this change is a little unusual.)

Vitest already provides documentation regarding the Testing Library Globals. On the other side, well-documented integrations like STL also document this topic. This seems to be the approach that others are going with, and it worked for me when I migrated from Jest to Vitest.

Personally, my vote would be for this change to be reverted. It seems to already be an inconvenience for other devs (like @oskarols) without providing significant benefit. (The end user isn't necessarily saved from having to go to the docs with this new breaking change.) For that change, I appeal to @afontcu. However, I'd understand if my request is rejected.

[afontcu]

Addressed in 1bbeeb4