-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Update: Move some components to single test runner #4132
Changes from all commits
1f89a6b
9149b6c
11d99f2
c202c1e
906c1a5
f8e1788
3ba9697
a10d0f4
3591eec
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not related to this PR, but maybe we should improve the way clean window document in
useFakeDom
helper to avoid manual cleaning?/cc @aduth @blowery
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did we have to do this in the first place? cleanup lets go of window, which holds MutationObserver...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was needed in the original test, but I agree, with
use-fake-dom
performing the cleanup inafter
, this shouldn't be necessary.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that window gets cleaned up and this won't get executed at all; it was semi-conscious decision to clean something up the test created explicitly.
We're kind of used to not cleaning up the tests since they used to live in different processes; I wanted this to be a good example that cleans up after itself.
Another argument might be that the consumer of
use-fake-dom
doesn't have to know which variables we are exposing and cleaning up; and should clean up the globals they've introduced.I'm not feeling wrong strong with either, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, whew, i thought you had to do it for some odd reason.
I'd be wary of leaving stuff like this in there, just because someone coming across it later might think it was strictly necessary and spent a bunch of time and energy trying to figure out why it was done. But maybe not. 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@blowery Oh no. Definitely the reverse effect of what I intended. If that's the effect you had, we should definitely remove that.