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

chore: make tests compatible with Jest 24 #15779

Merged
merged 5 commits into from
Aug 14, 2019
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -521,7 +521,6 @@ const tests = {
// Valid because the ref is captured.
code: `
function useMyThing(myRef) {
const myRef = useRef();
useEffect(() => {
const handleMove = () => {};
const node = myRef.current;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, {
useHook2 = () => { useState(); };
({useHook: () => { useState(); }});
({useHook() { useState(); }});
const {useHook = () => { useState(); }} = {};
const {useHook3 = () => { useState(); }} = {};
({useHook = () => { useState(); }} = {});
`,
`
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -651,7 +651,7 @@ describe('ReactDOMServerIntegration', () => {
});
});

describe('component hierarchies', async function() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

describe must be void (in reality it must be synchronous) in Jest. async here was always an error, but it throws with jest 24.

https://jestjs.io/docs/en/troubleshooting#defining-tests

describe('component hierarchies', function() {
itRenders('single child hierarchies of components', async render => {
const Component = props => <div>{props.children}</div>;
let e = await render(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,7 @@ describe('ChangeEventPlugin', () => {
ReactFeatureFlags = require('shared/ReactFeatureFlags');
// TODO pull this into helper method, reduce repetition.
// mock the browser APIs which are used in schedule:
// - requestAnimationFrame should pass the DOMHighResTimeStamp argument
// - calling 'window.postMessage' should actually fire postmessage handlers
global.requestAnimationFrame = function(cb) {
return setTimeout(() => {
cb(Date.now());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the raf from jsdom passes in the timestamp. is this safe to remove?

});
};
const originalAddEventListener = global.addEventListener;
let postMessageCallback;
global.addEventListener = function(eventName, callback, useCapture) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -543,3 +543,4 @@ describe 'ReactCoffeeScriptClass', ->
node = ReactDOM.findDOMNode(instance)
expect(node).toBe container.firstChild
undefined
undefined
Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as the async describe - the return value must be void.

7 changes: 7 additions & 0 deletions scripts/jest/config.base.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,4 +21,11 @@ module.exports = {
collectCoverageFrom: ['packages/**/*.js'],
timers: 'fake',
snapshotSerializers: [require.resolve('jest-snapshot-serializer-raw')],
// Jest changed from `about:blank` to `http://localhost` default in 24.5 (https://github.com/facebook/jest/pull/6792)
// in order to address https://github.com/facebook/jest/issues/6766. If one uses `about:blank` in JSDOM@11.12 or
// newer, it fails with `SecurityError: localStorage is not available for opaque origins`. However, some of React's
// tests depend on `about:blank` being the domain (for e.g. `url` in `img` tags). So we set `about:blank` here to
// keep the current behavior and make sure to keep the version of JSDOM to version lower than 11.12. This will have
// to be addressed properly when Jest 25 is released, as it will come with a newer version of JSDOM.
testURL: 'about:blank',
Copy link
Contributor Author

@SimenB SimenB Aug 7, 2019

Choose a reason for hiding this comment

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

Note that doing this breaks in JSDOM@>=11.12 with SecurityError: localStorage is not available for opaque origins which is why the default has changed in Jest. Can try to do something for this when Jest 25 is released, which will force a newer version of JSDOM.

For now, I've manually made sure the entry in the lockfile is ~11.11.0, which does not have this issue

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a short TODO here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ergh, expect it wasn't particularly short 😅

};
2 changes: 1 addition & 1 deletion scripts/jest/preprocessor.js
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ module.exports = {
}
: {}
)
).code;
Copy link
Contributor Author

@SimenB SimenB Aug 7, 2019

Choose a reason for hiding this comment

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

removing this ensures you return source maps to Jest, for better errors and debugging.

);
}
return src;
},
Expand Down
4 changes: 0 additions & 4 deletions scripts/jest/setupEnvironment.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,6 @@ global.__PROFILE__ = NODE_ENV === 'development';
global.__UMD__ = false;

if (typeof window !== 'undefined') {
global.requestAnimationFrame = function(callback) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

jsdom comes with raf

setTimeout(callback);
};

global.requestIdleCallback = function(callback) {
return setTimeout(() => {
callback({
Expand Down
Loading