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

FIX Get JS tests passing again #1330

Merged
merged 1 commit into from
Feb 13, 2023

Conversation

GuySartorelli
Copy link
Member

@GuySartorelli GuySartorelli commented Feb 9, 2023

Using FIX prefix because it affects src files and therefore dist files.

Parent issue

@@ -0,0 +1,6 @@
{
Copy link
Member Author

Choose a reason for hiding this comment

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

For non-admin modeles it seems that this config needs to be in babel.config.json rather than in package.json/.babelrc - https://babeljs.io/docs/en/configuration#whats-your-use-case

Comment on lines -27 to -35
item = ReactTestUtils.renderIntoDocument(
<AssetDropzone {...props} />
);
item = new AssetDropzone();
});

it('should set this.dropzone to null', () => {
item.dropzone = 1;
item.constructor(props);

Copy link
Member Author

Choose a reason for hiding this comment

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

Can no longer call constructor() directly on react components, need to use the new keyword

/* global jest, jasmine, describe, it, expect, beforeEach */
/* global jest, describe, it, expect, beforeEach */
Copy link
Member Author

Choose a reason for hiding this comment

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

jasmine isn't used or installed

.mockReturnValueOnce({ confirm: Promise.resolve(), callback: callbackMockFn });
.mockReturnValueOnce({ confirm: () => Promise.resolve(), callback: callbackMockFn });
Copy link
Member Author

@GuySartorelli GuySartorelli Feb 9, 2023

Choose a reason for hiding this comment

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

This wasn't actually hitting the bit of code it's meant to be testing, because Promise.resolve() returns an object, and the handleChangeValue() method in the BulkActions component has a typeof option.confirm === 'function' condition.

return bulkActions.handleChangeValue(event).then(() => {
expect(callbackMockFn).toBeCalled();
expect(callbackMockFn).not.toBeCalled();
Copy link
Member Author

Choose a reason for hiding this comment

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

This test was literally not testing the right thing before. I only caught this because the test started failing with a UnhandledPromiseRejection because the rejected promise wasn't being caught.

Comment on lines -52 to +54
(url, resolve, reject) => { reject(); }
(url, resolve, reject) => { reject(4); }
Copy link
Member Author

Choose a reason for hiding this comment

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

Added to help debug an issue where one of the promise rejections wasn't being caught. Left in so it's easier to find in the future.
The output of the error includes whatever is passed in here, so if it said the issue was with a reject that had 4 passed in we can just search for reject(4) and there it is.

jasmine.objectContaining({
expect.objectContaining({
Copy link
Member Author

Choose a reason for hiding this comment

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

Jasmine isn't installed - expect.objectContaining() does the same thing that used to.

Comment on lines +32 to +40
"testEnvironment": "jsdom",
"roots": [
"client/src"
],
"moduleNameMapper": {
"^react-dom/client$": "react-dom-16",
"^react-dom((/.*)?)$": "react-dom-16$1",
"^react((/.*)?)$": "react-16$1"
},
Copy link
Member Author

Choose a reason for hiding this comment

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

This and the inclusion of react16 in dev deps reflects how I got tests working in admin in silverstripe/silverstripe-admin#1389
see silverstripe/silverstripe-admin#1419

Comment on lines +63 to +64
"enzyme": "^3.11.0",
"enzyme-adapter-react-16": "^1.15.6",
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 tests use enzyme. This should have already been here.

Comment on lines -103 to -108
"babel": {
"presets": [
"@babel/preset-env",
"@babel/preset-react"
]
}
Copy link
Member Author

Choose a reason for hiding this comment

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

We're using the babel.config.json file instead.

@emteknetnz emteknetnz merged commit f800799 into silverstripe:2.0 Feb 13, 2023
@emteknetnz emteknetnz deleted the pulls/2/js-tests branch February 13, 2023 02:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants