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

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

Open
4 tasks done
eugene-manuilov opened this issue Jan 13, 2025 · 7 comments
Open
4 tasks done
Labels
P1 Medium priority QA: Eng Requires specialized QA by an engineer Team M Issues for Squad 2 Type: Infrastructure Engineering infrastructure & tooling

Comments

@eugene-manuilov
Copy link
Collaborator

eugene-manuilov commented Jan 13, 2025

Feature Description

In order to resolve js dependency conflicts, we need to separate dependencies used by different parts of our project and put them into separate workspaces. The first part of this process is to move js dependencies needed for E2E tests to run into a separate workspace.


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

Acceptance criteria

  • The package.json file is updated to have the workspaces section with the only one workspace for E2E tests.
  • JavaScript dependencies that are needed for E2E tests to function properly are moved to the new workspace.
  • GitHub actions for E2E tests are updated accordingly to continue running E2E tests for all PRs.

Implementation Brief

  • Update the package.json file to have the new workspaces section using [ "tests/e2e" ] as its value.
  • Create a new package.json file in the tests/e2e folder. You can use npm init -w ./tests/e2e to do that.
    • Copy over e2e commands from the main package.json file into the new one.
  • Scan all dependencies that we have in the main package.json file and move all dependecies that are used in e2e tests to its own package by uninstalling them in the main package.json file and installing with -w ./tests/e2e argument to specify the correct workspace.
  • Update the pacakge.json file to proxy e2e commands to the correct workspace using the npm run ... -w ./tests/e2e approach.

Test Coverage

  • N/A

QA Brief

  • QA Engineer to verify changes.

Changelog entry

  • Move e2e related dependencies into their own workspace.
@eugene-manuilov eugene-manuilov added P1 Medium priority QA: Eng Requires specialized QA by an engineer Type: Enhancement Improvement of an existing feature Type: Infrastructure Engineering infrastructure & tooling and removed Type: Enhancement Improvement of an existing feature labels Jan 13, 2025
@techanvil techanvil self-assigned this Jan 15, 2025
@techanvil
Copy link
Collaborator

Nice work here @eugene-manuilov, this looks like a good way to get us started with an incremental move to NPM workspaces.

IB ✅

@techanvil techanvil removed their assignment Jan 15, 2025
@eclarke1 eclarke1 added the Team M Issues for Squad 2 label Jan 16, 2025
@hussain-t hussain-t self-assigned this Jan 17, 2025
@hussain-t
Copy link
Collaborator

Hi @eugene-manuilov, since npm 7+ is required to use npm workspaces, this issue effectively depends on #6026 to upgrade Node to 18 (and npm 7+). If we remain on Node 14 (npm 6), we’ll have to manually upgrade npm and set up the workspace. It would be best to complete the Node upgrade first.

cc: @techanvil

@hussain-t hussain-t removed their assignment Jan 20, 2025
@eugene-manuilov
Copy link
Collaborator Author

Oh... that's unpleasant... Then we probably need to do it as a separate package.json file for now and compose all packages into workspaces later when we can upgrade node. What do you think, @techanvil? This also means that we would have to use a separate .nvmrc for e2e package.

@techanvil techanvil self-assigned this Jan 20, 2025
@techanvil
Copy link
Collaborator

@hussain-t @eugene-manuilov I don't see an issue upgrading to NPM 7 seeing as it only requires Node 10 upward.

Hussain, do you envisage this causing any particular problems?

https://github.com/npm/cli/blob/04eb43f2b2a387987b61a7318908cf18f03d97e0/package.json#L239-L241

Image

@techanvil techanvil assigned hussain-t and unassigned techanvil Jan 20, 2025
@hussain-t hussain-t removed their assignment Jan 22, 2025
@ankitrox ankitrox assigned ankitrox and unassigned ankitrox Jan 22, 2025
@benbowler benbowler self-assigned this Jan 24, 2025
@benbowler
Copy link
Collaborator

benbowler commented Jan 24, 2025

In order to upgrade npm as mentioned above you have to run npm install -g [email protected] as this is not the default version bundled with node 14.

This isn't as easily definable as adding an .nvmrc or adding it to the package.json. Should I update a README or developer handbook somewhere with this detail or otherwise share with the team.

@eugene-manuilov @techanvil

@techanvil techanvil self-assigned this Jan 24, 2025
@techanvil
Copy link
Collaborator

techanvil commented Jan 24, 2025

Hey @benbowler, although we will no doubt want to install it globally for convenience, and we should mention this in the handbook, I would suggest that we do add it to package.json and run it using npx in our various scripts and infrastructure.

This way we clearly track which version we depend on and don't have to override the globally installed version if we don't want to.

With [email protected] installed as a dev dependency, I'd suggest adding an NPM script for convenience and self documentation, e.g.:

"npm7": "npx [email protected]"

Which can then be run like this:

npx run npm7 install
npx run npm7 storybook

Once we've updated to a more recent Node version we can roll this back and revert to the stock global NPM. WDYT?

@techanvil techanvil removed their assignment Jan 24, 2025
@benbowler benbowler removed their assignment Jan 27, 2025
@techanvil
Copy link
Collaborator

techanvil commented Jan 28, 2025

@benbowler, maybe we could call npm in the NPM scripts rather than npx, as that should be resolved to the locally installed version (I think). Worth a look during execution. Thanks to @aaemnnosttv for suggesting this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 Medium priority QA: Eng Requires specialized QA by an engineer Team M Issues for Squad 2 Type: Infrastructure Engineering infrastructure & tooling
Projects
None yet
Development

No branches or pull requests

6 participants