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
Prepare DOM globals in test setup #3946
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
process.env.NODE_ENV = process.env.NODE_ENV || 'test'; | ||
|
||
module.exports = { | ||
require: ['@babel/register', 'spec/javascripts/spec_helper.js'], | ||
require: ['@babel/register'], | ||
file: 'spec/javascripts/spec_helper.js', | ||
extension: ['js', 'jsx'], | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,20 @@ | ||
const chai = require('chai'); | ||
const dirtyChai = require('dirty-chai'); | ||
const { JSDOM } = require('jsdom'); | ||
|
||
chai.use(dirtyChai); | ||
global.expect = chai.expect; | ||
|
||
// Emulate a DOM, since many modules will assume the presence of these globals | ||
// exist as a side effect of their import (focus-trap, classList.js, etc). | ||
const dom = new JSDOM(); | ||
global.window = dom.window; | ||
global.navigator = window.navigator; | ||
global.document = window.document; | ||
global.self = window; | ||
|
||
beforeEach(() => { | ||
while (document.body.firstChild) { | ||
document.body.removeChild(document.body.firstChild); | ||
} | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -39,13 +39,23 @@ const INITIAL_HTML = ` | |
</p> | ||
`; | ||
|
||
let originalWindow; | ||
let originalDocument; | ||
|
||
export const setupDocumentCaptureTestDOM = () => { | ||
originalWindow = global.window; | ||
originalDocument = global.document; | ||
Comment on lines
+46
to
+47
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Small error here: Since This was encountered and fixed in #3952 (ff6eac0519a61956f12081f5fff19bc259db61e2). |
||
|
||
// While there's already DOM globals, the current implementation assumes a | ||
// cleanly initialized DOM in every test case. Ideally all event handlers | ||
// attached to the DOM in the course of a test case would be cleaned up, and | ||
// the same DOM could be reused for every test. | ||
const dom = new JSDOM(INITIAL_HTML); | ||
global.window = dom.window; | ||
global.document = global.window.document; | ||
}; | ||
|
||
export const teardownDocumentCaptureTestDOM = () => { | ||
global.window = undefined; | ||
global.document = undefined; | ||
global.window = originalWindow; | ||
global.document = originalDocument; | ||
}; |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.js
tabbable
, viafocus-trap
react-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:
useDOM
) which restores the DOM globalsI 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good enough reason, thanks for explaining!
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.
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