Create npm workspace for E2E tests #10119

benbowler · 2025-01-27T10:26:55Z

Summary

Addresses issue:

Move js dependencies required for E2E tests into their own workspace #10013

Relevant technical choices

puppeteer and puppeteer-core are used in storybook tests currently, we should remove these from the main package.json as part of #10091.

I created a script install-global-npm to help developers and the GH actions to install npm v7.

PR Author Checklist

My code is tested and passes existing unit tests.
My code has an appropriate set of unit tests which all pass.
My code is backward-compatible with WordPress 5.2 and PHP 7.4.
My code follows the WordPress coding standards.
My code has proper inline documentation.
I have added a QA Brief on the issue linked above.
I have signed the Contributor License Agreement (see https://cla.developers.google.com/).

Do not alter or remove anything below. The following sections will be managed by moderators only.

Code Reviewer Checklist

Run the code.
Ensure the acceptance criteria are satisfied.
Reassess the implementation with the IB.
Ensure no unrelated changes are included.
Ensure CI checks pass.
Check Storybook where applicable.
Ensure there is a QA Brief.
Ensure there are no unexpected significant changes to file sizes.

Merge Reviewer Checklist

Ensure the PR has the correct target branch.
Double-check that the PR is okay to be merged.
Ensure the corresponding issue has a ZenHub release assigned.
Add a changelog message to the issue.

…refactoring.

github-actions · 2025-01-27T10:50:43Z

Build files for b04f70b have been deleted.

benbowler · 2025-01-27T14:43:13Z

Note the one failing E2E test will be fixed in #10111

eugene-manuilov

Thanks, @benbowler. Nice work, but we need to move more dependencies. Please, see my comments.

tests/e2e/package.json

Co-authored-by: Eugene Manuilov <eugene.manuilov@get10up.com>

…t globally.

…olve path to modules.

benbowler · 2025-02-04T14:59:15Z

@aaemnnosttv

I found that the scripts workspace scripts appeared to be running with mixed npm versions and found npx npm -v -w tests/e2e returned v6 so I installed npm v7 on the dev dependencies of the e2e tests too which fixes this.

However, one thing to note is that the workspace wants the root package to be upgraded as well so npx npm ci -w tests/e2e will try to update the root package-lock with the warning The package-lock.json file was created with an old version of npm (here meaning the root package-lock). I assume this is because npm workspaces resolves packages across all package.json files in the project so having package-lock files generated by two versions of npm (one of which doesn't support workspaces) is causing issues. In this case npx npm ci -w tests/e2e fails with sh: patch-package: command not found as it's trying to run the root postinstall script.

To me it seems that trying to run multiple npm versions is causing more problems than it's worth. Back in my first implementation I sent to review with globally installed npm v7, I had the workspaces and E2E tests working. With #6026 in EB, what do you think about requiring npm v7 globally in the interim, all devs need to do is run npm install -g npm@7.24.2 .

benbowler · 2025-02-04T14:59:55Z

I'm open to suggestions if you can get the dev dependency method working reliably, allowing install commands and scripts to run reliably at the root and package level.

benbowler · 2025-02-04T15:15:21Z

However, one thing to note is that the workspace wants the root package to be upgraded as well so npx npm ci -w tests/e2e will try to update the root package-lock with the warning The package-lock.json file was created with an old version of npm (here meaning the root package-lock).

One possible way to avoid this is by updating packages in the workspace like this for now: cd tests/e2e && npx npm ci

package.json

…move incorrect package-lock.

benbowler · 2025-02-05T12:48:10Z

Removed pre e2e script from root as these are run by the scripts.

@aaemnnosttv I tried npm 9 then 8 but these failed to install packages the following error so I've stuck with npm v7 and this can upgrade along with future node.js updates.

197 error code ERESOLVE
198 verbose stack TypeError: Cannot read property 'length' of null
198 verbose stack     at explainDependents (/Users/benbowler/.nvm/versions/node/v14.21.3/lib/node_modules/npm/lib/utils/explain-dep.js:94:22)
198 verbose stack     at explainNode (/Users/benbowler/.nvm/versions/node/v14.21.3/lib/node_modules/npm/lib/utils/explain-dep.js:5:3)
198 verbose stack     at explain (/Users/benbowler/.nvm/versions/node/v14.21.3/lib/node_modules/npm/lib/utils/explain-eresolve.js:25:26)
198 verbose stack     at report (/Users/benbowler/.nvm/versions/node/v14.21.3/lib/node_modules/npm/lib/utils/explain-eresolve.js:63:21)
198 verbose stack     at errorMessage (/Users/benbowler/.nvm/versions/node/v14.21.3/lib/node_modules/npm/lib/utils/error-message.js:41:37)
198 verbose stack     at exitHandler (/Users/benbowler/.nvm/versions/node/v14.21.3/lib/node_modules/npm/lib/utils/exit-handler.js:181:53)
198 verbose stack     at module.exports (/Users/benbowler/.nvm/versions/node/v14.21.3/lib/node_modules/npm/lib/cli-entry.js:72:12)
199 verbose cwd /Users/benbowler/Code/sitekit.10uplabs.com/app/public/wp-content/plugins/google-site-kit

Later npm versions also have different module resolution rules which causes an issue with these now deprecated modules. If we upgrade to later npm versions we will need to resolve this issue so as not to run all install/ci commands with --legacy-peer-deps.

npm ERR! code ERESOLVE
npm ERR! ERESOLVE could not resolve
npm ERR! 
npm ERR! While resolving: @storybook/addon-storyshots-puppeteer@6.3.6
npm ERR! Found: @storybook/addon-storyshots@6.5.16
npm ERR! node_modules/@storybook/addon-storyshots
npm ERR!   dev @storybook/addon-storyshots@"^6.5.16" from the root project
npm ERR! 
npm ERR! Could not resolve dependency:
npm ERR! peer @storybook/addon-storyshots@"6.3.6" from @storybook/addon-storyshots-puppeteer@6.3.6
npm ERR! node_modules/@storybook/addon-storyshots-puppeteer
npm ERR!   dev @storybook/addon-storyshots-puppeteer@"^6.3.6" from the root project
npm ERR! 
npm ERR! Conflicting peer dependency: @storybook/addon-storyshots@6.3.6
npm ERR! node_modules/@storybook/addon-storyshots
npm ERR!   peer @storybook/addon-storyshots@"6.3.6" from @storybook/addon-storyshots-puppeteer@6.3.6
npm ERR!   node_modules/@storybook/addon-storyshots-puppeteer
npm ERR!     dev @storybook/addon-storyshots-puppeteer@"^6.3.6" from the root project
npm ERR! 
npm ERR! Fix the upstream dependency conflict, or retry
npm ERR! this command with --force or --legacy-peer-deps
npm ERR! to accept an incorrect (and potentially broken) dependency resolution.
npm ERR! 
npm ERR! 
npm ERR! For a full report see:
npm ERR! /Users/benbowler/.npm/_logs/2025-02-04T17_39_00_347Z-eresolve-report.txt

benbowler · 2025-02-05T12:50:04Z

@eugene-manuilov this is now running E2E tests locally and on GH actions, failing checks are existing known instabilities.

In terms of the CLA check it appears this joint commit was not verified which is causing the failure:

tests/e2e/package.json

package.json

techanvil · 2025-02-05T20:12:26Z

Hey @benbowler, as per the sync earlier I've taken a look to revisit the idea of running a dev-installed version of NPM.

I noticed there's a bit of a chicken and egg situation when trying to initially install NPM 7 with workspaces configured in package.json when the global version is NPM 6.

This can be resolved, here's a potential approach with a bootstrap-npm7 to do the initial npm install, thus cleanly creating the initial package-lock.json. It could use a modification to support running npm ci for a clean start with the package-lock.json in place: infrastructure/10013-npm-workspaces-e2e...infrastructure/10013-npm-workspaces-e2e-local-npm

However, one thing to note is that the workspace wants the root package to be upgraded as well so npx npm ci -w tests/e2e will try to update the root package-lock with the warning The package-lock.json file was created with an old version of npm (here meaning the root package-lock). I assume this is because npm workspaces resolves packages across all package.json files in the project so having package-lock files generated by two versions of npm (one of which doesn't support workspaces) is causing issues.

Hmm, there should only be a single package-lock.json, at the repo root. And, this of course implies we should not expect to have to support multiple versions of NPM within the repo.

In this case npx npm ci -w tests/e2e fails with sh: patch-package: command not found as it's trying to run the root postinstall script.

This case, however seems to be due to npm ci -w tests/e2e only installing the direct dependencies for the workspace. Even with NPM 7 installed globally, I'm seeing the same thing. Given that patch-package seems to have quite a few default patches, it might be prudent to make this a dependency for all workspaces (this would merit a double check, I'm basing this on a quick search for now).

patch-package search results

34 results - 5 files

package-lock.json:
     146  				"npm": "7.24.2",
     147: 				"patch-package": "^8.0.0",
     148  				"postcss": "^8.4.31",

   55125  		},
   55126: 		"node_modules/patch-package": {
   55127  			"version": "8.0.0",
   55128: 			"resolved": "https://registry.npmjs.org/patch-package/-/patch-package-8.0.0.tgz",
   55129  			"integrity": "sha512-da8BVIhzjtgScwDJ2TtKsfT5JFWz1hYoBl9rUQ1f38MC2HwnEIkK8VN3dKMKcP7P7bvvgzNDbfNHtx3MsQb5vA==",

   55148  			"bin": {
   55149: 				"patch-package": "index.js"
   55150  			},

   55155  		},
   55156: 		"node_modules/patch-package/node_modules/ansi-styles": {
   55157  			"version": "4.3.0",

   55170  		},
   55171: 		"node_modules/patch-package/node_modules/chalk": {
   55172  			"version": "4.1.2",

   55186  		},
   55187: 		"node_modules/patch-package/node_modules/ci-info": {
   55188  			"version": "3.9.0",

   55201  		},
   55202: 		"node_modules/patch-package/node_modules/color-convert": {
   55203  			"version": "2.0.1",

   55213  		},
   55214: 		"node_modules/patch-package/node_modules/color-name": {
   55215  			"version": "1.1.4",

   55219  		},
   55220: 		"node_modules/patch-package/node_modules/cross-spawn": {
   55221  			"version": "7.0.3",

   55233  		},
   55234: 		"node_modules/patch-package/node_modules/has-flag": {
   55235  			"version": "4.0.0",patch-package

   55242  		},
   55243: 		"node_modules/patch-package/node_modules/path-key": {
   55244  			"version": "3.1.1",

   55251  		},
   55252: 		"node_modules/patch-package/node_modules/rimraf": {
   55253  			"version": "2.7.1",

   55264  		},
   55265: 		"node_modules/patch-package/node_modules/semver": {
   55266  			"version": "7.6.2",

   55276  		},
   55277: 		"node_modules/patch-package/node_modules/shebang-command": {
   55278  			"version": "2.0.0",

   55288  		},
   55289: 		"node_modules/patch-package/node_modules/shebang-regex": {
   55290  			"version": "3.0.0",

   55297  		},
   55298: 		"node_modules/patch-package/node_modules/slash": {
   55299  			"version": "2.0.0",

   55306  		},
   55307: 		"node_modules/patch-package/node_modules/supports-color": {
   55308  			"version": "7.2.0",

   55318  		},
   55319: 		"node_modules/patch-package/node_modules/which": {
   55320  			"version": "2.0.2",

   55333  		},
   55334: 		"node_modules/patch-package/node_modules/yaml": {
   55335  			"version": "2.4.5",

  111172  		},
  111173: 		"patch-package": {
  111174  			"version": "8.0.0",
  111175: 			"resolved": "https://registry.npmjs.org/patch-package/-/patch-package-8.0.0.tgz",
  111176  			"integrity": "sha512-da8BVIhzjtgScwDJ2TtKsfT5JFWz1hYoBl9rUQ1f38MC2HwnEIkK8VN3dKMKcP7P7bvvgzNDbfNHtx3MsQb5vA==",

package.json:
   49  		"bootstrap-npm7": "node bin/bootstrap-npm7.js",
   50: 		"postinstall": "patch-package"
   51  	},

  229  		"npm": "7.24.2",
  230: 		"patch-package": "^8.0.0",
  231  		"postcss": "^8.4.31",

package.json.new:
   49  		"bootstrap-npm7": "node bin/bootstrap-npm7.js",
   50: 		"postinstall": "patch-package"
   51  	},

  229  		"npm": "7.24.2",
  230: 		"patch-package": "^8.0.0",
  231  		"postcss": "^8.4.31",

bin/bootstrap-npm7.js:
  16  
  17: // Create a wrapper script that handles patch-package
  18  const wrapperScript = `

  29      
  30:     // Run patch-package after successful npm operations that modify dependencies
  31      const command = process.argv[2];
  32      if (['install', 'i', 'ci', 'add', 'update', 'upgrade'].includes(command)) {
  33:         execSync('node_modules/.bin/patch-package', { stdio: 'inherit' });
  34      }

  53  
  54: 	console.log( 'npm7 wrapper installed. Use `npx npm7` to run npm commands with automatic patch-package handling.' );
  55  

Untitled-1:
  1: 		"postinstall": "node -e \"if(process.env.SKIP_PATCH !== 'true') require('child_process').execSync('patch-package')\"",
  2: 		"apply-patches": "patch-package"

It's worth noting that other script/command dependencies of the root package.json will also be missing when installing via npm ci -w <workspace> - the one I noticed was husky, which is admittedly unlikely to cause an issue in real-world usage as it's only used for commit hooks and it seems quite unlikely we'd want to use npm ci -w <workspace> outside of a CI context.

To me it seems that trying to run multiple npm versions is causing more problems than it's worth. Back in my first implementation I sent to review with globally installed npm v7, I had the workspaces and E2E tests working. With #6026 in EB, what do you think about requiring npm v7 globally in the interim, all devs need to do is run npm install -g npm@7.24.2 .

I can see it's more complicated than I had imagined, but at the same time it seems that some of the issues you've come across are not related to the local/global NPM question.

On balance I'm still likely to agree with the conclusion that simply installing the required version globally is the path of least resistance, but I'm interested to get your thoughts on my findings.

assets/js/modules/sign-in-with-google/components/setup/SetupForm.js

This reverts commit 5ff1a38.

benbowler · 2025-02-06T14:00:22Z

Hey @techanvil, thanks for taking a look at this. Based on your change devs would be instructed to run npx npm ci or ./node_modules/.../npm ci, it that what you were thinking? Or the assumption is npm <7 will handle a package-lock created by npm 7 correctly?

It appears to me that either route devs will need to change their process slightly, so if this is the case, the least painful in my experience so far is ensure there is only one npm version being used linked to the nvm installed version. It's fairly simple to revert to the default npm by reinstalling node 14 using nvm (although I doubt many devs will have other projects still using node 14 - although until this is merged devs might need to switch back and forward as they review tickets).

…13-npm-workspaces-e2e.

techanvil · 2025-02-06T14:34:58Z

Hey @techanvil, thanks for taking a look at this. Based on your change devs would be instructed to run npx npm ci or ./node_modules/.../npm ci, it that what you were thinking? Or the assumption is npm <7 will handle a package-lock created by npm 7 correctly?

Hey @benbowler, if we took the approach indicated by my change then (assuming NPM 6 is installed globally):

Firstly, one engineer would need to run - npm run bootstrap-npm7 in order to run npm install using a copy of NPM 7 installed in a temporary location, thus generating a valid NPM 7 compatible package-lock.json for the repo which includes NPM 7 as a dev dependency.
Subsequently, the first time another engineer wants to use the repo, they would need to run npm run ci-npm7 where the ci-npm7 script is a modified version of bootstrap-npm7 that runs npm ci instead of npm install, this is to ensure they are running npm ci with NPM 7 before it's installed as a dev dependency.
Following this, it should be possible to "simply" run npx npm ci to perform additional clean installs, although we could always use the ci-npm7 script again if this proved problematic.

There wouldn't be any expectation that NPM 6 would handle an NPM 7 package-lock.json, we wouldn't run into a scenario where we tried to use one with NPM 6.

It appears to me that either route devs will need to change their process slightly, so if this is the case, the least painful in my experience so far is ensure there is only one npm version being used linked to the nvm installed version. It's fairly simple to revert to the default npm by reinstalling node 14 using nvm (although I doubt many devs will have other projects still using node 14 - although until this is merged devs might need to switch back and forward as they review tickets).

Indeed, with that all said above, I'll refer back to the last line in my previous comment:

On balance I'm still likely to agree with the conclusion that simply installing the required version globally is the path of least resistance, but I'm interested to get your thoughts on my findings.

Basically it does look like trying to use a local NPM 7 is going to add more complexity/overhead than simply upgrading the global version, so I'd say let's stick with that. It will become a moot point once we upgrade Node anyway.

benbowler · 2025-02-06T14:55:22Z

Thanks @techanvil for working through the options here.

@eugene-manuilov I've addressed the dependencies left in the root package and left some inline comments.

eugene-manuilov

Great work, @benbowler. Looks good to me.

benbowler added 3 commits January 24, 2025 12:32

Create E2E workspace and make scripts functional without any package …

835fd58

…refactoring.

Update package-lock to npm version 7 format.

f1eaef0

Move E2E to npm workspace.

34a622a

benbowler added 3 commits January 27, 2025 11:13

Update npm version in e2e GitHub action.

ddf20d9

Update npm version in e2e GitHub action.

3894782

Update npm version in JS lint GitHub action.

60b77c2

benbowler changed the title ~~Infrastructure/10013-npm-workspaces-e2e~~ Create npm workspace for E2E tests Jan 28, 2025

eugene-manuilov requested changes Jan 30, 2025

View reviewed changes

tests/e2e/package.json Outdated Show resolved Hide resolved

tests/e2e/package.json Outdated Show resolved Hide resolved

tests/e2e/package.json Outdated Show resolved Hide resolved

tests/e2e/package.json Outdated Show resolved Hide resolved

benbowler and others added 15 commits January 31, 2025 15:19

Apply suggestions from code review

96aac9b

Co-authored-by: Eugene Manuilov <eugene.manuilov@get10up.com>

Include dependencies shared with root package and e2e workspace.

b756ac7

Move local-env scripts to tests/e2e workspace.

846d233

Update package-lock with nppm 7.

054adf8

Include other required packages in e2e/tests.

1a5bff0

Fix paths in docker-compose and revert change in start.sh.

582f0ec

Include npm v7 as dev dependency so developer don't need to install i…

2912b48

…t globally.

Remove googlesitekit-e2e from deps.

4234514

Move jest-puppeteer config to workspace root.

762c366

Remove a few packages from root dev deps.

2d0732d

Update jest-puppeteer to version matching puppeteer.

477444b

Update GH actions to use dev dependency version of npm.

f7b4b46

Revert shell script change and remove code comment.

8064c60

Update lint script to install workspace packages to fix unable to res…

f3b9a69

…olve path to modules.

Add npm v7 to workspace dev dependencies.

8bf4eaf

benbowler added 2 commits February 4, 2025 15:16

Update package installation method.

ed28fa2

Restore working base setup with global npm config.

7741226

benbowler commented Feb 5, 2025

View reviewed changes

package.json Show resolved Hide resolved

benbowler added 3 commits February 5, 2025 10:26

Update local-env paths.

226a0e1

Update workspace package.json to include all required packages and re…

44664d8

…move incorrect package-lock.

Fix volume path to correctly load test plugins in Docker images.

5510bd8

eugene-manuilov reviewed Feb 5, 2025

View reviewed changes

tests/e2e/package.json Show resolved Hide resolved

Remove e2e packages in from the root packages.

1e4f326

eugene-manuilov requested changes Feb 5, 2025

View reviewed changes

tests/e2e/package.json Show resolved Hide resolved

tests/e2e/package.json Show resolved Hide resolved

package.json Show resolved Hide resolved

Revert to commit building in actions.

d1a1de7

benbowler force-pushed the infrastructure/10013-npm-workspaces-e2e branch from 067e649 to d1a1de7 Compare February 6, 2025 11:54

Revert to commit building in actions.

2d318a0

This comment was marked as resolved.

Sign in to view

Remove un-needed root dependency.

b19e8f8

benbowler commented Feb 6, 2025

View reviewed changes

assets/js/modules/sign-in-with-google/components/setup/SetupForm.js Show resolved Hide resolved

benbowler added 4 commits February 6, 2025 12:20

Remove un-needed root dependency.

950e3e5

Remove un-needed root dependency.

7a26cb9

Remove un-needed root dependency.

5ff1a38

Revert "Remove un-needed root dependency."

94414d6

This reverts commit 5ff1a38.

benbowler mentioned this pull request Feb 6, 2025

Move js dependencies required for storybook into their own workspace #10091

Open

5 tasks

benbowler added 2 commits February 6, 2025 14:00

Remove un-needed root dependency.

d946f16

Merge remote-tracking branch 'origin/develop' into infrastructure/100…

b04f70b

…13-npm-workspaces-e2e.

eugene-manuilov approved these changes Feb 6, 2025

View reviewed changes

eugene-manuilov merged commit aaa172c into develop Feb 6, 2025
21 of 24 checks passed

eugene-manuilov deleted the infrastructure/10013-npm-workspaces-e2e branch February 6, 2025 20:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create npm workspace for E2E tests #10119

Create npm workspace for E2E tests #10119

benbowler commented Jan 27, 2025 •

edited

Loading

github-actions bot commented Jan 27, 2025 •

edited

Loading

benbowler commented Jan 27, 2025

eugene-manuilov left a comment

benbowler commented Feb 4, 2025

benbowler commented Feb 4, 2025

benbowler commented Feb 4, 2025 •

edited

Loading

benbowler commented Feb 5, 2025

benbowler commented Feb 5, 2025

techanvil commented Feb 5, 2025

This comment was marked as resolved.

benbowler commented Feb 6, 2025

techanvil commented Feb 6, 2025

benbowler commented Feb 6, 2025

eugene-manuilov left a comment

Create npm workspace for E2E tests #10119

Create npm workspace for E2E tests #10119

Conversation

benbowler commented Jan 27, 2025 • edited Loading

Summary

Relevant technical choices

PR Author Checklist

Code Reviewer Checklist

Merge Reviewer Checklist

github-actions bot commented Jan 27, 2025 • edited Loading

benbowler commented Jan 27, 2025

eugene-manuilov left a comment

Choose a reason for hiding this comment

benbowler commented Feb 4, 2025

benbowler commented Feb 4, 2025

benbowler commented Feb 4, 2025 • edited Loading

benbowler commented Feb 5, 2025

benbowler commented Feb 5, 2025

techanvil commented Feb 5, 2025

This comment was marked as resolved.

benbowler commented Feb 6, 2025

techanvil commented Feb 6, 2025

benbowler commented Feb 6, 2025

eugene-manuilov left a comment

Choose a reason for hiding this comment

benbowler commented Jan 27, 2025 •

edited

Loading

github-actions bot commented Jan 27, 2025 •

edited

Loading

benbowler commented Feb 4, 2025 •

edited

Loading