Migrate apps unit tests away from wp scripts #79097

noahtallen · 2023-07-07T00:26:33Z

Proposed Changes

This migrates ETK and Odyssey Stats away from WP scripts. There isn't a clear reason to use WP scripts for this any more. Our main config handles everything, and it's even faster because it has some caching. This reduces complexity so that our unit test setup has fewer dependencies and moving pieces.

This should also make it easier to move forward with both #78786 and #78711.

Testing Instructions

All unit tests should pass in C:

Also, try these manually if you like:

# From root directory:
yarn test-apps
cd apps/editing-toolkit
yarn test:js
cd ../odyssey-stats
yarn test:js

I've confirmed that the number of tests being executed is the same as before. (These suites are pretty small :))

matticbot · 2023-07-07T00:26:38Z

This PR modifies the release build for editing-toolkit

To test your changes on WordPress.com, run install-plugin.sh editing-toolkit migrate-apps-unit-tests-away-from-wp-scripts on your sandbox.

To deploy your changes after merging, see the documentation: PCYsg-mMA-p2

github-actions · 2023-07-07T00:26:44Z

Calypso Live (direct link)

	https://calypso.live?image=registry.a8c.com/calypso/app:build-72858

Jetpack Cloud live (direct link)

	https://calypso.live?image=registry.a8c.com/calypso/app:build-72858&env=jetpack

noahtallen · 2023-07-07T00:27:30Z

apps/editing-toolkit/bin/babel-transform.js

We're using the transformer provided by calypso-jest now.

noahtallen · 2023-07-07T00:27:41Z

apps/editing-toolkit/bin/js-unit-setup.js

@@ -1,3 +1 @@
-import '@testing-library/jest-dom/extend-expect';


This is handled by the calypso-jest setup file.

noahtallen · 2023-07-07T00:28:51Z

apps/editing-toolkit/package.json

-		"test:js:help": "npx wp-scripts test-unit-js --config='jest.config.js' --help --colors",
-		"test:js:update-snapshots": "npx wp-scripts test-unit-js -u --config='jest.config.js' --colors",
-		"test:js:watch": "npx wp-scripts test-unit-js --config='jest.config.js' --watch --colors",
+		"test:js": "yarn run -T test-apps apps/editing-toolkit",


-T executes the script from the workspace root. In this case, it's necessary because babel-jest won't work unless it can find a babel config file. (And without that, imports don't work!) That config file is found in the root of the repo. There doesn't seem to be an easy way to configure this, other than running jest from the same directory. Running unit tests from the root package.json seems to solve that.

noahtallen · 2023-07-07T00:29:55Z

apps/editing-toolkit/package.json

@@ -40,10 +40,8 @@
 		"lint:php": "../../vendor/bin/phpcs --standard=../phpcs.xml ./ ",
 		"lint:php:fix": "../../vendor/bin/phpcbf --standard=../phpcs.xml ./ ",
 		"sync:newspack-blocks": "./bin/sync-newspack-blocks.sh",
-		"test:js": "npx wp-scripts test-unit-js --config='jest.config.js' --colors",
-		"test:js:help": "npx wp-scripts test-unit-js --config='jest.config.js' --help --colors",
-		"test:js:update-snapshots": "npx wp-scripts test-unit-js -u --config='jest.config.js' --colors",


I don't think these are useful -- yarn test:js -u, or yarn test:js --help will access these options. --watch would also work, but it's more commonly used.

noahtallen · 2023-07-07T00:31:46Z

test/apps/jest-preset.js

+const path = require( 'path' );
+const base = require( '@automattic/calypso-jest' );
+
+module.exports = {


This copies a lot of things from test/apps/client and test/apps/packages. It doesn't strictly need to be separate, but probably useful to be more granular, as the projects are all deployed to different locations.

noahtallen · 2023-07-07T00:32:04Z

test/apps/jest.config.js

+module.exports = {
+	rootDir: './../../',
+	// run tests for all packages that have a Jest config file
+	projects: [ '<rootDir>/apps/*/jest.config.js' ],


basically same approach as packages.

noahtallen · 2023-07-07T00:32:22Z

yarn.lock

I guess wordpress/scripts was showing up a lot 😆

matticbot · 2023-07-07T00:38:11Z

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

Sections (~948 bytes removed 📉 [gzipped])

name                     parsed_size           gzip_size
site-setup-flow               -621 B  (-0.0%)     -948 B  (-0.2%)
plugins                       -621 B  (-0.0%)     -948 B  (-0.2%)
media                         -621 B  (-0.0%)     -948 B  (-0.2%)
import-hosted-site-flow       -621 B  (-0.0%)     -948 B  (-0.2%)
import-flow                   -621 B  (-0.0%)     -948 B  (-0.1%)
devdocs                       -621 B  (-0.3%)     -948 B  (-1.6%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Async-loaded Components (~948 bytes removed 📉 [gzipped])

name                                               parsed_size           gzip_size
async-load-calypso-post-editor-media-modal              -621 B  (-0.0%)     -948 B  (-0.3%)
async-load-calypso-post-editor-editor-media-modal       -621 B  (-0.0%)     -948 B  (-0.3%)

React components that are loaded lazily, when a certain part of UI is displayed for the first time.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

tyxla

Nice one 👍 Great to see these as part of the general unit test check 💯

🚢 🚀

tyxla · 2023-07-07T10:22:44Z

test/apps/jest-preset.js

+	],
+	setupFiles: [ 'jest-canvas-mock' ],
+	// This includes a lot of globals that don't exist, like fetch, matchMedia, etc.
+	setupFilesAfterEnv: [ require.resolve( '../client/setup-test-framework.js' ) ],


Worth evaluating if we need all those mocks and polyfills or if we can include a more minimal version.

I was looked into this, but we'd need to duplicate at least a few things from that file. From what I could tell, none of it was specific enough to the client setup to justify splitting it out.

I think it'd be good to keep this in mind in the future though. I personally like having this shared as much as possible, since there aren't big differences in the environments! Both the client and ETK run in a browser, after all.

roo2 · 2023-07-10T02:40:12Z

This caused the newspack-blocks sync task to fail to replace the text domain. Details: p1688956741274269/1685923446.140549-slack-C7YPUHBB2

I'll revert for now.

This reverts commit d9acd80.

roo2 · 2023-07-10T02:57:56Z

@noahtallen I also noticed that the "Odyssey Stats" plugin build has its tests tab hidden with this change (screenshot from testing the revert)

This reverts commit d9acd80.

tyxla · 2023-07-10T13:07:30Z

@roo2 I appreciate the quick revert, but I also have a couple of notes and suggestions here about things we can improve in the future.

Given that this functionality seems to be pretty fragile (it breaks quite often when upgrading anything on the frameworks side), there is no alerting that it broke (it could use some unit testing), and it's not very intuitive (replacing a textdomain with the eslint --fix command), I'd suggest considering another solution for it.

In this particular scenario, I'd have suggested that you debug and fix it. It didn't seem like a difficult debugging and since you're familiar with that test workflow I assume it would have been straightforward to resolve it. I know reverting is the fastest way to fix issues, but it's not the most collaborative one.

So, that being said, I'd summarize my suggestions for actions to consider to prevent this from happening again:

Consider using a different, less fragile and more straightforward mechanism for replacing the textdomains.
Consider implementing alerting or a separate check after the eslint --fix job that verifies that the textdomain has been replaced properly. That way if we break it, we'll know and we'll fix it before merging the offending PR.

Let me know what you think!

noahtallen · 2023-07-10T20:00:16Z

Consider using a different, less fragile and more straightforward mechanism for replacing the textdomains.

Unfortunately, this is something put in place 3 years ago (#42583), which I've mostly been maintaining as needed since. (Plus, it was the solution created by the i18n team at the time!) I don't know if we have any better options than lint tools, because they're fully automatic. I don't think it makes sense to create a custom tool for this.

On top of that, I have created tests for this in the past (#44323). However, I'm not sure if that would have caught this issue, since it's in JS files and not PHP. However, that check doesn't seem to exist any more, so it may have been removed at some point as we were iterating on CI configuration.

We should probably revisit the second check, but it's a bit tricky!

Unfortunately the newspack blocks integration is not very nice, but I don't think there's a clearly better solution. (The whole thing exists in the first place because dotcom product wanted the blocks available, but not under the newspack branding -- hence we need to extract them, and add our own wrapper around them, before deploying them ourselves inside of ETK. And since it's a different codebase, it needs a different textdomain already deployed to dotcom.)

@noahtallen I also noticed that the "Odyssey Stats" plugin build has its tests tab hidden with this change (screenshot from testing the revert)

This is expected -- the tests were moved to the main unit test build, so they don't run as part of the odyssey stats build any more

tyxla · 2023-07-11T08:30:49Z

Thanks for the extra context @noahtallen. I think it's not a good situation and we need to at least do one of the suggested improvements above. If we decide to rely on the tricky, fragile, and non-intuitive approach, we just need to add a test to ensure that it always works. Otherwise, it will just keep breaking without us noticing and we need to prevent that.

noahtallen · 2023-07-12T20:40:43Z

Opened #79266 and #79272 for follow-up

noahtallen added 5 commits July 6, 2023 17:19

Create new Apps jest config and use it instead of WP scripts

6eadef9

Remove unit tests from app builds

ad678fc

Create new run scripts

599b03e

Remove unused dependencies

8f008af

Add app unit tests to main unit test CI build

dcb6fbd

noahtallen requested a review from a team July 7, 2023 00:26

noahtallen self-assigned this Jul 7, 2023

matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jul 7, 2023

noahtallen requested review from a team July 7, 2023 00:27

noahtallen commented Jul 7, 2023

View reviewed changes

noahtallen mentioned this pull request Jul 7, 2023

chore: upgrade jest (and related packages) to v29 #78786

Merged

7 tasks

Remove odyssey stats unit tests

2d1c522

noahtallen requested a review from flootr July 7, 2023 00:45

tyxla approved these changes Jul 7, 2023

View reviewed changes

tyxla mentioned this pull request Jul 7, 2023

Update wordpress packages to the latest version #78711

Merged

17 tasks

noahtallen merged commit d9acd80 into trunk Jul 7, 2023

noahtallen deleted the migrate-apps-unit-tests-away-from-wp-scripts branch July 7, 2023 23:56

github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jul 7, 2023

noahtallen mentioned this pull request Jul 8, 2023

CI: remove unused configurations and further tweaks to filter unnecessary builds. #78973

Merged

9 tasks

roo2 added a commit that referenced this pull request Jul 10, 2023

Revert "Migrate apps unit tests away from wp scripts (#79097)"

085b3ca

This reverts commit d9acd80.

roo2 mentioned this pull request Jul 10, 2023

Revert "Migrate apps unit tests away from wp scripts" #79159

Merged

roo2 added a commit that referenced this pull request Jul 10, 2023

Revert "Migrate apps unit tests away from wp scripts (#79097)" (#79159)

126fb96

This reverts commit d9acd80.

flootr mentioned this pull request Jul 10, 2023

Migrate apps unit tests away from wp scripts 2nd take #79176

Merged

This was referenced Jul 12, 2023

Update wp-env and some ETK PHP fixes #79266

Merged

Add more newspack blocks fixes #79272

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate apps unit tests away from wp scripts #79097

Migrate apps unit tests away from wp scripts #79097

noahtallen commented Jul 7, 2023 •

edited

Loading

matticbot commented Jul 7, 2023

github-actions bot commented Jul 7, 2023 •

edited by matticbot

Loading

noahtallen Jul 7, 2023

noahtallen Jul 7, 2023

noahtallen Jul 7, 2023

noahtallen Jul 7, 2023

noahtallen Jul 7, 2023

noahtallen Jul 7, 2023

noahtallen Jul 7, 2023

matticbot commented Jul 7, 2023 •

edited

Loading

tyxla left a comment

tyxla Jul 7, 2023

noahtallen Jul 7, 2023

roo2 commented Jul 10, 2023

roo2 commented Jul 10, 2023

tyxla commented Jul 10, 2023

noahtallen commented Jul 10, 2023 •

edited

Loading

tyxla commented Jul 11, 2023

noahtallen commented Jul 12, 2023

		@@ -1,3 +1 @@
		import '@testing-library/jest-dom/extend-expect';

Migrate apps unit tests away from wp scripts #79097

Migrate apps unit tests away from wp scripts #79097

Conversation

noahtallen commented Jul 7, 2023 • edited Loading

Proposed Changes

Testing Instructions

matticbot commented Jul 7, 2023

github-actions bot commented Jul 7, 2023 • edited by matticbot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matticbot commented Jul 7, 2023 • edited Loading

tyxla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roo2 commented Jul 10, 2023

roo2 commented Jul 10, 2023

tyxla commented Jul 10, 2023

noahtallen commented Jul 10, 2023 • edited Loading

tyxla commented Jul 11, 2023

noahtallen commented Jul 12, 2023

noahtallen commented Jul 7, 2023 •

edited

Loading

github-actions bot commented Jul 7, 2023 •

edited by matticbot

Loading

matticbot commented Jul 7, 2023 •

edited

Loading

noahtallen commented Jul 10, 2023 •

edited

Loading