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

test: fix auto-cleanup tests #371

Merged
merged 6 commits into from May 15, 2024
Merged

test: fix auto-cleanup tests #371

merged 6 commits into from May 15, 2024

Conversation

mcous
Copy link
Collaborator

@mcous mcous commented Apr 30, 2024

Overview

Our auto-cleanup tests were implemented as a snapshot test. The snapshot was wrong, so these tests were passing erroneously. This PR fixes the test, removing all snapshot tests from the codebase.

A previous iteration of this PR tried to fix up the pure export, which used to exist to skip auto-cleanup. It was more broken by recent refactors than I realized, so instead I used this PR to abandon the export. A comment was updated accordingly.

Change log

  • Fix auto-cleanup tests, which weren't actually testing the global auto-cleanup due to our vitest setup file
    • This has the happy side effect of removing our last snapshot test
    • This helps unblock adding Jest to the test matrix
  • Remove reference to ./pure export condition

@mcous mcous marked this pull request as ready for review April 30, 2024 04:28
@mcous mcous requested a review from yanick April 30, 2024 04:28
@mcous
Copy link
Collaborator Author

mcous commented May 11, 2024

@yanick did you get a chance to check this PR out? I'd like to get it in so I can move forward on adding Jest to the matrix

@yanick
Copy link
Collaborator

yanick commented May 13, 2024

If we export it explicitly, we might want to give it a more meaningful name. svelte4, or perhaps core? (with an alias for pure as not to break peeps that somehow already use it)

@mcous
Copy link
Collaborator Author

mcous commented May 13, 2024

pure is used across the testing-library ecosystem, so I think it’s the most “conventional.”

I’d like to move away from the svelte5 export when Svelte 5 hits production (it’s currently RC), because it’s been causing folks issues (see the Svelte 5 issue thread). For that reason I’d be opposed to naming the “pure” export svelte4

@yanick
Copy link
Collaborator

yanick commented May 13, 2024

pure is used across the testing-library ecosystem, so I think it’s the most “conventional.”

I’d like to move away from the svelte5 export when Svelte 5 hits production (it’s currently RC), because it’s been causing folks issues (see the Svelte 5 issue thread). For that reason I’d be opposed to naming the “pure” export svelte4

Just to echo the other issue thread, my preference is to have the default import points to the latest supported svelte version, and have the possibility to load the specific versions via svelte-testing-library/svelte4 and svelte-testing-library/svelte5.

@mcous
Copy link
Collaborator Author

mcous commented May 14, 2024

Just to echo the other issue thread, my preference is to have the default import points to the latest supported svelte version, and have the possibility to load the specific versions via svelte-testing-library/svelte4 and svelte-testing-library/svelte5.

Should we move this particular discussion to #284 and maybe include some actual users of the library? I don't want the larger strategy discussion to derail the rather small changes of this PR and #374, where we've been talking about it thus far

If we export [pure] explicitly, we might want to give it a more meaningful name.

Since dropping Svelte 4 support from the main export would be a breaking change, for now would you be ok with moving this PR forward as is - using the pure export? From there, I think we can make more informed - and safer - changes

The benefit in the short term would simply be to align with our existing docs and the pattern established by react-testing-library and preact-testing-library - which this library was originally based on. Having used both react-testing-library and preact-testing-library before working in Svelte, I think "pure" is meaningful enough in the testing-library ecosystem to keep it for now. Changing it feels a bit like change for change's sake

No longer applicable for this PR; removed the export

@mcous
Copy link
Collaborator Author

mcous commented May 14, 2024

Shoot, I missed that #328 removed the testing-library re-exports from pure.js. Those would have to be added back in to get pure.js back to its original intended usage as a side-effect-free import path

Figured it would be easier to abandon pure entirely, this PR now concentrates on fixing the auto-cleanup tests

@mcous mcous changed the title fix: ensure pure module is exported test: fix auto-cleanup tests May 14, 2024
@mcous
Copy link
Collaborator Author

mcous commented May 15, 2024

@yanick since this has been up for a couple weeks - and I've pared it down to tests only - I'm going to merge it to move forward on adding Jest to the CI matrix unless you have any additional thoughts

@mcous mcous merged commit ac3248d into main May 15, 2024
16 checks passed
@mcous mcous deleted the fix-auto-cleanup-export branch May 15, 2024 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants