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.

Sign up for GitHub

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

Jump to bottom

ci: run PR jobs on Ubuntu, MacOS and Windows #329

Merged
merged 4 commits into from
Feb 1, 2022
Merged

ci: run PR jobs on Ubuntu, MacOS and Windows #329

merged 4 commits into from
Feb 1, 2022

Conversation

petebacondarwin
Copy link
Contributor

@petebacondarwin petebacondarwin commented Jan 28, 2022 •
edited
Loading

After the cache has been primed we still get super fast jobs on Ubuntu but somewhat slower on MacOS and Windows:

Screenshot 2022-02-01 at 14 04 17

@changeset-bot
Copy link

changeset-bot bot commented Jan 28, 2022 •
edited
Loading

🦋 Changeset detected

Latest commit: ecacf92

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
wrangler Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@petebacondarwin petebacondarwin force-pushed the path-handling branch 9 times, most recently from 9ea3968 to a843255 Compare January 30, 2022 08:55
@JacobMGEvans JacobMGEvans mentioned this pull request Jan 31, 2022
Closed
@petebacondarwin
Copy link
Contributor Author

Hmm, this is worrying. When we use the CI cache for node_modules in Windows and then run npm install we get this:

Run npm install

removed 1 package, changed 4 packages, and audited 1330 packages in 4s

Followed by a bunch of failures. It seems that whatever we are caching is not quite right for Windows npm... Or Windows npm is broken.

@threepointone
Copy link
Contributor

I can confirm something's up with our caching strategy, but also something's up with how eslint cache even works.I faced CI failures in #348 but not locally. Had to blow away my local .eslintcache to get the error.

@petebacondarwin petebacondarwin force-pushed the path-handling branch 9 times, most recently from 0c0c453 to 9f633ef Compare January 31, 2022 21:35
@petebacondarwin petebacondarwin marked this pull request as ready for review January 31, 2022 21:38
@petebacondarwin petebacondarwin changed the title ci: run PR jobs on both Ubuntu and Windows ci: run PR jobs on Ubuntu, MacOS and Windows Jan 31, 2022
@petebacondarwin
Copy link
Contributor Author

Hmm, this is worrying. When we use the CI cache for node_modules in Windows and then run npm install we get this:

Run npm install

removed 1 package, changed 4 packages, and audited 1330 packages in 4s

Followed by a bunch of failures. It seems that whatever we are caching is not quite right for Windows npm... Or Windows npm is broken.

I worked out what the problem is here.

We were caching the node_modules directories but we are using npm workspaces, which create symlinks from the top level to each of the workspaces (e.g. node_modules/wrangler gets symlinked to packages/wrangler). Unfortunately, on Windows, when the cache is unpacked these symlinks are lost so npm thinks that any dependencies that had been stored locally under packages/wrangler/node_modules were no longer required and are removed. Hence the message above and also hence the broken checks and tests.

@petebacondarwin petebacondarwin added enhancement New feature or request maintenance Maintenance task labels Feb 1, 2022
@petebacondarwin petebacondarwin mentioned this pull request Feb 1, 2022
Closed
@petebacondarwin
Copy link
Contributor Author

This PR will fix #361

threepointone
threepointone approved these changes Feb 1, 2022
View reviewed changes
Copy link
Contributor

@threepointone threepointone left a comment

Choose a reason for hiding this comment

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

Looks great, very excited to see the improvements! Approving tentatively, but with a couple of questions.

package.json Outdated Show resolved Hide resolved
packages/wrangler/src/__tests__/publish.test.ts Show resolved Hide resolved
@petebacondarwin
ci: run PR jobs on both Ubuntu, MacOS and Windows
dd92d04 
- update .gitattributes to be consistent on Windows
- update Prettier command to ignore unknown files
  Windows seems to be more brittle here.
- tighten up eslint config
  Windows seems to be more brittle here as well.
- use the matrix.os value in the cache key
  Previously we were using `running.os` but this appeared not to be working.
@petebacondarwin
test: support testing in CI on Windows
b5d017f 
- Don't rely on bash variables to configure tests
  The use of bash variables in the `npm test` script is not supported in Windows Powershell, causing CI on Windows to fail.
  These bash variables are used to override the API token and the Account ID.

  This change moves the control of mocking these two concepts into the test code, by adding `mockAccountId()` and `mockApiToken()` helpers.

  - The result is slightly more boilerplate in tests that need to avoid hitting the auth APIs.
  - But there are other tests that had to revert these environment variables. So the boilerplate is reduced there.

- Sanitize command line for snapshot tests
  This change applies `normalizeSlashes()` and `trimTimings()` to command line outputs and error messages to avoid inconsistencies in snapshots.
  The benefit here is that authors do not need to keep adding them to all their snapshot tests.

- Move all the helper functions into their own directory to keep the test directory cleaner.
@petebacondarwin
refactor: use helpers to manage npm commands
ac2d7e7 
This also speeds up tests and avoids us checking that npm did what it is supposed to do.
@petebacondarwin
ci: use npm ci and do not cache workspace packages in node_modules
ecacf92 
Previously we were caching all the `node_modules` files in the CI jobs and then running `npm install`. While this resulted in slightly improved install times on Ubuntu, it breaks on Windows because the npm workspace setup adds symlinks into node_modules, which the Github cache action cannot cope with.

This change removes the `node_modules` caches (saving some time by not needing to restore them) and replaces `npm install` with `npm ci`.

The `npm ci` command is actually designed to be used in CI jobs as it only installs the exact versions specified in the `package-lock.json` file, guaranteeing that for any commit we always have exactly the same CI job run, deterministically.

It turns out that, on Ubuntu, using `npm ci` makes very little difference to the installation time (~30 secs), especially if there is no `node_modules` there in the first place.

Unfortunately, MacOS is slower (~1 min), and Windows even worse (~2 mins)! But it is worth this longer CI run to be sure we have things working on all OSes.
@petebacondarwin petebacondarwin merged commit b8a3e78 into cloudflare:main Feb 1, 2022
@petebacondarwin petebacondarwin deleted the path-handling branch February 1, 2022 17:30
@github-actions github-actions bot mentioned this pull request Feb 1, 2022
Merged
@threepointone threepointone mentioned this pull request Feb 2, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@threepointone threepointone threepointone approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request maintenance Maintenance task
Projects
workers-sdk
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@petebacondarwin @threepointone