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

Prepare DOM globals in test setup #3946

Merged
merged 1 commit into from Jul 20, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion .mocharc.js
@@ -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'],
};
Expand Up @@ -3,11 +3,8 @@ import { render, fireEvent, cleanup } from '@testing-library/react';
import sinon from 'sinon';
import AcuantCapture from '../../../../../app/javascript/app/document-capture/components/acuant-capture';
import { Provider as AcuantContextProvider } from '../../../../../app/javascript/app/document-capture/context/acuant';
import { useDOM } from '../../../support/dom';

describe('document-capture/components/acuant-capture', () => {
useDOM();

afterEach(() => {
// While RTL will perform this automatically, it must to occur prior to
// resetting the global variables, since otherwise the component's effect
Expand Down
@@ -1,11 +1,8 @@
import React from 'react';
import { render } from '@testing-library/react';
import DocumentCapture from '../../../../../app/javascript/app/document-capture/components/document-capture';
import { useDOM } from '../../../support/dom';

describe('document-capture/components/document-capture', () => {
useDOM();
Copy link
Contributor

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

Copy link
Member Author

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:

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:

  1. Always set up the DOM globals in the test setup so they're available at the time that dependencies are imported
  2. Unset those DOM globals in global test lifecycle methods
  3. 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.

Copy link
Contributor

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!

Copy link
Member Author

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


it('renders a heading', () => {
const { getByText } = render(<DocumentCapture />);

Expand Down
3 changes: 0 additions & 3 deletions spec/javascripts/app/document-capture/context/acuant.jsx
Expand Up @@ -3,11 +3,8 @@ import { render } from '@testing-library/react';
import AcuantContext, {
Provider as AcuantContextProvider,
} from '../../../../../app/javascript/app/document-capture/context/acuant';
import { useDOM } from '../../../support/dom';

describe('document-capture/context/acuant', () => {
useDOM();

afterEach(() => {
delete window.AcuantJavascriptWebSdk;
});
Expand Down
3 changes: 0 additions & 3 deletions spec/javascripts/app/document-capture/context/i18n-spec.jsx
@@ -1,11 +1,8 @@
import React, { useContext } from 'react';
import { render } from '@testing-library/react';
import I18nContext from '../../../../../app/javascript/app/document-capture/context/i18n';
import { useDOM } from '../../../support/dom';

describe('document-capture/context/i18n', () => {
useDOM();

const ContextValue = () => JSON.stringify(useContext(I18nContext));

it('defaults to empty object', () => {
Expand Down
3 changes: 0 additions & 3 deletions spec/javascripts/app/document-capture/hooks/use-i18n-spec.jsx
Expand Up @@ -2,11 +2,8 @@ import React from 'react';
import { render } from '@testing-library/react';
import I18nContext from '../../../../../app/javascript/app/document-capture/context/i18n';
import useI18n from '../../../../../app/javascript/app/document-capture/hooks/use-i18n';
import { useDOM } from '../../../support/dom';

describe('document-capture/hooks/use-i18n', () => {
useDOM();

const LocalizedString = ({ stringKey }) => useI18n()(stringKey);

it('returns localized key value', () => {
Expand Down
15 changes: 15 additions & 0 deletions spec/javascripts/spec_helper.js
@@ -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);
}
});
14 changes: 12 additions & 2 deletions spec/javascripts/support/acuant/document_capture_dom.js
Expand Up @@ -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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).


// 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;
};
20 changes: 0 additions & 20 deletions spec/javascripts/support/dom.js

This file was deleted.