Add NPM workspace for tests/js. #10387
Conversation
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
|
Build files for b24cc3c are ready:
|
tests/js/package.json
Outdated
| "@lit-labs/react": "^1.2.0", | ||
| "@material-ui/core": "^4.11.4", | ||
| "@material/button": "^2.3.0", | ||
| "@material/checkbox": "^2.3.0", | ||
| "@material/dialog": "^2.3.0", | ||
| "@material/form-field": "^2.3.0", | ||
| "@material/layout-grid": "^0.41.0", | ||
| "@material/linear-progress": "^1.1.0", | ||
| "@material/list": "^2.3.0", | ||
| "@material/menu": "^2.3.0", | ||
| "@material/radio": "^2.3.0", | ||
| "@material/react-card": "^0.15.0", | ||
| "@material/react-chips": "^0.15.0", | ||
| "@material/react-dialog": "^0.15.0", | ||
| "@material/react-select": "^0.15.0", | ||
| "@material/react-tab-bar": "^0.15.0", | ||
| "@material/react-text-field": "^0.15.0", | ||
| "@material/ripple": "^2.3.0", | ||
| "@material/select": "^2.3.1", | ||
| "@material/switch": "^2.3.0", | ||
| "@material/textfield": "^2.3.1", | ||
| "@material/theme": "^1.1.0", | ||
| "@material/web": "^1.0.0-pre.9", | ||
| "@reach/combobox": "^0.10.5", | ||
| "@react-hook/event": "^1.2.1", | ||
| "@react-hook/merged-ref": "^1.3.0", | ||
| "@react-hook/throttle": "^2.2.0", | ||
| "@wordpress/api-fetch": "^3.18.0", |
There was a problem hiding this comment.
I'm a bit concerned that we're duplicating all the dependencies used by our main build here. Can we not simply reference file:{path to assets workspace root} as a dependency here instead in which case they'd always be in sync?
There was a problem hiding this comment.
As far as I can tell, these aren't necessary to include here since they will be installed by the assets workspace anyways 🤔 E.g. tests still work as expected
⚡ npm run test:js -- modules/adsense/components
> test:js
> npm run test:js -w ./tests/js "modules/adsense/components"
> test:js
> cross-env BABEL_ENV=test NODE_ENV=test jest --config=./jest.config.js --passWithNoTests "modules/adsense/components"
PASS ../../assets/js/modules/adsense/components/common/UseSnippetSwitch.test.js (45.5 s)
PASS ../../assets/js/modules/adsense/components/widgets/ConnectAdSenseCTATileWidget.test.js (45.569 s)
PASS ../../assets/js/modules/adsense/components/common/WebStoriesAdUnitSelect.test.js (45.939 s)
PASS ../../assets/js/modules/adsense/components/module/ModuleOverviewWidget/Footer.test.js (46.119 s)
PASS ../../assets/js/modules/adsense/components/settings/AdBlockingRecoveryToggle.test.js (46.627 s)
PASS ../../assets/js/modules/adsense/components/common/AccountSelect.test.js (46.413 s)
PASS ../../assets/js/modules/adsense/components/settings/AdBlockingRecoverySetupCTANotice.test.js (47.27 s)
PASS ../../assets/js/modules/adsense/components/dashboard/AdSenseConnectCTAWidget.test.js (48.108 s)
PASS ../../assets/js/modules/adsense/components/dashboard/AdBlockingRecoverySetupCTAWidget.test.js (49.979 s)
PASS ../../assets/js/modules/adsense/components/setup/AdBlockingRecoveryApp/SetupMain.test.js (50.069 s)
Test Suites: 10 passed, 10 total
Tests: 64 passed, 64 total
Snapshots: 1 passed, 1 total
Time: 53.305 s
Ran all test suites matching /modules\/adsense\/components/i.
I guess it may still be correct to install them as a dependency explicitly to ensure the correct version is loaded, but I'd still think that we can leverage our assets workspace as a package rather than duplicate everything again in this package, which would be a nightmare to maintain. @techanvil @eugene-manuilov ?
There was a problem hiding this comment.
Hmm, that could work if we can specify the assets workspace package as a dependency of this one; it does mean we lose the decoupling of the workspace dependencies which is an aim of this reorganisation, as it allows us to more freely update the dependencies on a per-workspace basis.
Currently, we have a script planned that will help us maintain these dependencies by ensuring the common versions are the same where needed, this is specced in #10091 and also mentioned in the IB for this issue. It looks like #10091 should in fact be a dependency for this one. @ankitrox, you'll want to coordinate with @juniovitorino on that front (assuming we don't decide to change things).
As an alternative to specifying common dependencies in one or more workspace, we could of course manage them via the root package.json; although, that could introduce more dependency version issues for workspaces which don't directly depend on the common deps.
There was a problem hiding this comment.
that could work if we can specify the assets workspace package as a dependency of this one; it does mean we lose the decoupling of the workspace dependencies which is an aim of this reorganisation, as it allows us to more freely update the dependencies on a per-workspace basis.
@techanvil If we're syncing the dependencies between workspaces then they're implicitly coupled rather than explicitly since it's not clear that the packages are there due to their presence in the other space – there's of course coupling in that JS tests rely on packages that are used by these components in tests. In my understanding, I think using the workspace as a dependency should still "require" all the dependencies and not the devDependencies (TBC) so I would think it should be the cleanest way to achieve the same result while hopefully minimizing any additional coupling between the two spaces. I'll share more with you directly.
There was a problem hiding this comment.
Thanks @aaemnnosttv, that's an excellent point. I had forgotten that devDependencies won't be included when adding a dependency to the workspace package, and, as we specifically want to ensure that the dependencies in assets are used with the same versions in the storybook and tests/js workspaces, it does make sense to make assets a dependency for each of these workspaces.
We'll need to change direction for the implementation of this and #10091, but it should hopefully simplify them rather than the opposite.
Incidentally, I just gave this a quick test, applying this change to the branch:
diff --git a/assets/package.json b/assets/package.json
index 65ee7d55e8..b646e09541 100644
--- a/assets/package.json
+++ b/assets/package.json
@@ -1,4 +1,7 @@
{
+ "name": "@googlesitekit/assets",
+ "version": "1.0.0",
+ "private": true,
"scripts": {
"build": "npm run build:production",
"prebuild:dev": "npm run remove-dist",
diff --git a/tests/js/package.json b/tests/js/package.json
index eee1ed5f6b..5054f8cd54 100644
--- a/tests/js/package.json
+++ b/tests/js/package.json
@@ -5,66 +5,7 @@
"test:js:watch": "npm run test:js -- --watch"
},
"dependencies": {
- "@lit-labs/react": "^1.2.0",
- "@material-ui/core": "^4.11.4",
- "@material/button": "^2.3.0",
- "@material/checkbox": "^2.3.0",
- "@material/dialog": "^2.3.0",
- "@material/form-field": "^2.3.0",
- "@material/layout-grid": "^0.41.0",
- "@material/linear-progress": "^1.1.0",
- "@material/list": "^2.3.0",
- "@material/menu": "^2.3.0",
- "@material/radio": "^2.3.0",
- "@material/react-card": "^0.15.0",
- "@material/react-chips": "^0.15.0",
- "@material/react-dialog": "^0.15.0",
- "@material/react-select": "^0.15.0",
- "@material/react-tab-bar": "^0.15.0",
- "@material/react-text-field": "^0.15.0",
- "@material/ripple": "^2.3.0",
- "@material/select": "^2.3.1",
- "@material/switch": "^2.3.0",
- "@material/textfield": "^2.3.1",
- "@material/theme": "^1.1.0",
- "@material/web": "^1.0.0-pre.9",
- "@reach/combobox": "^0.10.5",
- "@react-hook/event": "^1.2.1",
- "@react-hook/merged-ref": "^1.3.0",
- "@react-hook/throttle": "^2.2.0",
- "@wordpress/api-fetch": "^3.18.0",
- "@wordpress/compose": "^3.25.3",
- "@wordpress/data": "^4.27.3",
- "@wordpress/dom-ready": "^2.10.0",
- "@wordpress/element": "^2.20.3",
- "@wordpress/i18n": "^3.14.0",
- "@wordpress/icons": "^2.9.0",
- "@wordpress/keycodes": "^3.2.4",
- "@wordpress/url": "^3.1.1",
- "classnames": "^2.2.6",
- "clipboard-copy": "^3.1.0",
- "compare-versions": "^3.6.0",
- "dompurify": "^2.5.8",
- "focus-trap-react": "^10.0.0",
- "immer": "^9.0.15",
- "intersection-observer": "^0.12.0",
- "invariant": "^2.2.4",
- "just-detect-adblock": "^1.1.0",
- "lodash": "^4.17.21",
- "md5": "^2.2.1",
- "memize": "^1.1.0",
- "prop-types": "^15.7.2",
- "psl": "^1.8.0",
- "punycode": "^2.1.1",
- "query-string": "^7.0.1",
- "react": "^16.14.0",
- "react-dom": "^16.14.0",
- "react-google-charts": "^3.0.15",
- "react-joyride": "^2.9.3",
- "react-router-dom": "^5.2.0",
- "react-use": "^15.3.4",
- "react-use-observer": "^2.2.4",
- "use-memo-one": "^1.1.2"
+ "@googlesitekit/assets": "file:../../assets"
},
"devDependencies": {
"@jackfranklin/test-data-bot": "^2.0.0",
@@ -85,7 +26,7 @@
"eslint-plugin-jsdoc": "^30.7.13",
"eslint-plugin-lodash": "^7.4.0",
"eslint-plugin-react-hooks": "^4.2.0",
- "eslint-plugin-sitekit": "file:packages/eslint-plugin",
+ "eslint-plugin-sitekit": "file:../../packages/eslint-plugin",
"eslint-scope": "^5.1.0",
"eslint-webpack-plugin": "^2.5.1",
"faker": "^5.5.3",I then ran npm install and the resulting changes to package-lock.json and to the installed node_modules looked good.
@ankitrox please note this additional, slightly off topic but required change, and also that you'll need to run npm install and commit the changes to package-lock.json.
- "eslint-plugin-sitekit": "file:packages/eslint-plugin",
+ "eslint-plugin-sitekit": "file:../../packages/eslint-plugin",
package.json
Outdated
| "test:js": "cross-env BABEL_ENV=test NODE_ENV=test jest --config=tests/js/jest.config.js --passWithNoTests", | ||
| "test:js:watch": "npm run test:js -- --watch", | ||
| "test:js": "npm run test:js -w ./tests/js", | ||
| "test:js:watch": "npm run test:js:watch -w ./tests/js", | ||
| "test:storybook": "npm run test:js -- --testMatch '<rootDir>/storybook/*.test.js'", |
There was a problem hiding this comment.
Technically test:storybook is an E2E test (originally I thought it was JS but it uses Jest + Puppeteer) but looks like this may have been missed in putting it the E2E workspace.
There was a problem hiding this comment.
@aaemnnosttv I think this would be good option to move into /storybook workspace that is being introduced in #10091. WDYT?
There was a problem hiding this comment.
@ankitrox, this is not enough. We need to:
- remove all dependencies from this file that are not directly needed to run tests
- remove all dependencies that are needed only for js tests from the main package.json file
As to dependencies that are used by js files that we import in test files, I think they shouldn't be defined here. Instead, we need to update the corresponding GH action to install dependencies for assets and tests/js workspaces. In this case i think it should do the trick 🤔, but if it is not, then let me know and I will help to take a look.
@eugene-manuilov see my related comment above. I don't think any change to |
Yeah, i also have the same feeling that it should work correctly. Something may be broken when we upgrade node.js to the latest version, but we can address it there. For now, we should just use only those deps that are really needed in the workspace. |
I don't think we should rely on dependencies from one workspace being installed for another one to work without making that inter-workspace dependency explicit, at the least. My thinking, though, it that if we specify all the dependencies for each workspace independently it will be easier to manage updating dependencies on a per workspace basis. I've expanded on this a bit in my other comment: #10387 (comment) |
|
Thanks all, just to summarize, we will be simplifying our use of dependencies where we rely on those from another workspace as discussed in this thread above. #10387 (comment) |
Summary
Addresses issue:
Relevant technical choices
PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist