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

Bump Jest version to v27 #4499

Merged
merged 11 commits into from
Jan 21, 2022
Merged

Bump Jest version to v27 #4499

merged 11 commits into from
Jan 21, 2022

Conversation

fluiddot
Copy link
Contributor

@fluiddot fluiddot commented Jan 21, 2022

Related to WordPress/gutenberg#38133.

This PR bumps the Jest package version to v27 to match the same version in Gutenberg, which was recently updated in WordPress/gutenberg#33287.

Additionally, it updates the unit tests located in GB-mobile in order to prevent failures with the update.

To test:

Automated tests

  1. Check that all PR checks pass, especially the Android, iOS device tests, and unit tests.

Manual tests

  1. Run command npm run test:e2e:android:local.
  2. Observe that Android device tests pass.
  3. Run command npm run test:e2e:ios:local.
  4. Observe that iOS device tests pass.
  5. Run command npm run test.
  6. Observe that unit tests pass.

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered if this change warrants user-facing release notes more info and have added them to RELEASE-NOTES.txt if necessary.

@fluiddot fluiddot added the dependencies Pull requests that update a dependency file label Jan 21, 2022
@fluiddot fluiddot added this to the 1.71.0 (19.2) milestone Jan 21, 2022
@fluiddot fluiddot self-assigned this Jan 21, 2022
Comment on lines -41 to +42
"jest": "^24.9.0",
"jest-junit": "^6.3.0",
"jest": "^27.4.5",
"jest-junit": "^13.0.0",
Copy link
Contributor Author

@fluiddot fluiddot Jan 21, 2022

Choose a reason for hiding this comment

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

"babel-jest": "^24.9.0",
"babel-jest": "^27.4.5",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This version matches the value from Gutenberg's package.json file:
https://github.com/WordPress/gutenberg/blob/5a84b27bcead1528ca98bcade9c6d5259c70de7b/package.json#L151

@fluiddot fluiddot marked this pull request as draft January 21, 2022 10:13
@fluiddot fluiddot removed the request for review from dcalhoun January 21, 2022 10:13
@fluiddot
Copy link
Contributor Author

Changing the PR to draft as it requires further modifications to fix the unit tests 🔧 .

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Jan 21, 2022

Wanna run full suite of Android and iOS UI tests? Click here and 'Approve' CI job!

registerGutenberg( {
beforeInitCallback: () => {
// We have to lazy import the setup code to prevent executing any code located
// at global scope before the editor is initialized, like translations retrieval.
require( './setup' ).default();
export default function registerGutenbergMobile() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to expose the register function mainly for testing purposes, so we can initialize Gutenberg Mobile on each test without having to reimport the module.

@fluiddot fluiddot marked this pull request as ready for review January 21, 2022 12:38
@fluiddot
Copy link
Contributor Author

I noticed that Android device tests are failing with the following error:

Error: EACCES: permission denied, open '/home/circleci/project/junit.xml'

I'll take a deeper look but I have the gut feeling that after the Jest upgrade, we might need to reconfigure how/where CircleCI fetches the test results.

@@ -26,7 +26,6 @@ module.exports = {
clearMocks: true,
preset: './gutenberg/node_modules/react-native/jest-preset.js',
setupFiles: [ '<rootDir>/' + configPath + '/setup.js' ],
testEnvironment: 'jsdom',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now we use node environment which is now the default value (reference).

Comment on lines +35 to +47
const setupLocaleLogs = [
[ 'locale', defaultLocale, getGutenbergTranslation( defaultLocale ) ],
[
'jetpack - locale',
defaultLocale,
getJetpackTranslation( defaultLocale ),
],
[
'layout-grid - locale',
defaultLocale,
getLayoutGridTranslation( defaultLocale ),
],
];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These logs correspond to the setup locale logs triggered from here.

Copy link
Member

Choose a reason for hiding this comment

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

Not for this PR, but what do you think about refactoring all of our intentional logs to reside behind a ENABLE_LOGGING environment flag? E.g. ENABLE_LOGGING=true npm run start:quick.

Whether to add a log message is often left to each contributor to decide what subject reaches the threshold of deserving a log message. Ultimately, the project will likely arrive at a point where there are so many logs that it becomes difficult to parse important warnings or errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAIK, we don't display too many informational logs, apart from the locale/translations ones, but in any case, I like your idea about providing an option for enabling/disabling logs. As you mentioned, it would help us on identifying important messages like warnings and errors 👍.

Comment on lines -34 to +56
jest.isolateModules( () => {
// Import entry point to initialize the editor
require( '../index' );
} );
return render( <EditorComponent { ...props } /> );
registerGutenbergMobile();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

After running several tests, I noticed that now using isolateModules when rendering the editor causes failures. As far as I investigated, it duplicates the Redux stores, in my case I verified it was creating two different stores for @wordpress/blocks. This leads to a scenario where blocks are registered into the second store but the editor complains about the lack of blocks (most likely because it's accessing the first one).

For this reason, I decided to stop using the workaround of isolating modules for registering Gutenberg on each test, and instead export the register function and call it here.

Comment on lines -50 to -63
// Jetpack blocks are registered when importing the editor extension module.
// Since we need to register the blocks multiple times for testing,
// it's required to isolate modules for re-importing the editor extension multiple times.
const registerJetpackBlocksIsolated = ( props ) => {
jest.isolateModules( () => {
registerJetpackBlocks( props );
} );
};
// Similarly to Jetpack blocks, Jetpack embed variations also require to isolate modules.
const registerJetpackEmbedVariationsIsolated = ( props ) => {
jest.isolateModules( () => {
registerJetpackEmbedVariations( props );
} );
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the same reason as outlined in this comment, I removed the use of isolateModules. In this case, the main purpose of using this function was to assure that Jetpack/Layout-grid blocks are registered on each test, as they are registered upon module importation. However, I realized after removing that function that it's no longer required to isolate the modules, as blocks are registered on every test.

@fluiddot
Copy link
Contributor Author

I noticed that Android device tests are failing with the following error:

Error: EACCES: permission denied, open '/home/circleci/project/junit.xml'

I'll take a deeper look but I have the gut feeling that after the Jest upgrade, we might need to reconfigure how/where CircleCI fetches the test results.

I found out that the environment variable used for specifying the output file of Jest-unit (i.e. JEST_JUNIT_OUTPUT), is not listed in the documentation of the package (reference). The package has been upgraded from version 6.3.0 to 13.0.0, and I noticed that on version 8.0.0 it was renamed (reference). I'll update the CircleCI configuration to use JEST_JUNIT_OUTPUT_FILE instead.

Copy link
Member

@dcalhoun dcalhoun left a comment

Choose a reason for hiding this comment

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

🚀 LGTM. Thank you for putting this work together. 👏🏻

I attempted to verify the e2e tests locally, but ran in to a fair amount of flakiness and unrelated build errors, unfortunately. Provided the CI tests pass, I think we are good to go. 👍🏻

Comment on lines +35 to +47
const setupLocaleLogs = [
[ 'locale', defaultLocale, getGutenbergTranslation( defaultLocale ) ],
[
'jetpack - locale',
defaultLocale,
getJetpackTranslation( defaultLocale ),
],
[
'layout-grid - locale',
defaultLocale,
getLayoutGridTranslation( defaultLocale ),
],
];
Copy link
Member

Choose a reason for hiding this comment

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

Not for this PR, but what do you think about refactoring all of our intentional logs to reside behind a ENABLE_LOGGING environment flag? E.g. ENABLE_LOGGING=true npm run start:quick.

Whether to add a log message is often left to each contributor to decide what subject reaches the threshold of deserving a log message. Ultimately, the project will likely arrive at a point where there are so many logs that it becomes difficult to parse important warnings or errors.

@fluiddot
Copy link
Contributor Author

I confirm that all device tests are 🟢 after running the optional ones:

Screenshot 2022-01-21 at 17 31 53

@fluiddot fluiddot merged commit 6169e94 into develop Jan 21, 2022
@fluiddot fluiddot deleted the update/bump-jest-version-v27 branch January 21, 2022 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants