Prepare DOM globals in test setup #3946
Conversation
**Why**: Many third-party dependencies assume the presence of DOM globals as a side-effect of their import, and will cause tests to fail if not available.
|
|
||
| describe('document-capture/components/document-capture', () => { | ||
| useDOM(); |
There was a problem hiding this comment.
Do you think it would be worth splitting up the test suite for React things vs legacy things? I like how explicit these were
There was a problem hiding this comment.
The primary challenge this pull request seeks to address is that many dependencies will reach to assumed DOM globals as a side effect of merely being brought into scope (imported).
Examples:
classList.jstabbable, viafocus-trapreact-testing-library(a new discovery)
So if the DOM setup is delayed until this point in the test lifecycle, it's too late, since those have already been evaluated.
This is true for both legacy code and for the new React code, at least if it brings in those dependencies (like proposed with Accordion usage in #3938).
Putting the DOM setup in spec_helper.js helps ensure those globals are assigned in time for that initial import.
It's true that this means all tests will run with a DOM available, which isn't always necessary, and could potentially encourage (or at least not discourage) best practices with tight coupling to the DOM.
Because we'll need the DOM for the React tests, and the DOM will need to be set up early, I'm not sure it's something we could easily split, at least not in a way where we have some expressiveness like this within the test itself to declare it as needing the DOM.
One possible option could be to invert the behavior:
- Always set up the DOM globals in the test setup so they're available at the time that dependencies are imported
- Unset those DOM globals in global test lifecycle methods
- Allow an explicit opt-in (like this
useDOM) which restores the DOM globals
I could explore this if it seems like a valuable addition.
That said, my experience is that the DOM will be needed more often than not, and it may not be worth the effort or the overhead in marking each of these tests explicitly.
There was a problem hiding this comment.
That's a good enough reason, thanks for explaining!
There was a problem hiding this comment.
As a general pattern though, I do think it could be nice to adopt this sort of convention of lifecycle-based opt-in helpers ("hooks").
Related: WordPress/gutenberg#22804
| originalWindow = global.window; | ||
| originalDocument = global.document; |
There was a problem hiding this comment.
Small error here: Since setupDocumentCaptureTestDOM runs in beforeEach (before each test case) and teardownDocumentCaptureTestDOM runs in after (after all have finished), originalWindow assignment here might actually be assigning from a previous test case within the same Acuant test suite, thus overriding the true "original".
This was encountered and fixed in #3952 (ff6eac0519a61956f12081f5fff19bc259db61e2).
Extracted (cherry-picked) from: #3938
Why: Many third-party dependencies assume the presence of DOM globals as a side-effect of their import, and will cause tests to fail if not available.