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

feat(ci): isolate smoke tests, introduce caching #8449

Merged
merged 11 commits into from
Jun 3, 2023

Conversation

jtoar
Copy link
Contributor

@jtoar jtoar commented May 30, 2023

Sometimes it feels like our CI leaves a lot to be desired, specifically the Windows smoke tests. It's not that our CI isn't thorough, but that it takes anywhere from twenty to forty minutes to run. Most of this time is spent setting up the test project fixture for the smoke tests.

The test project fixture doesn't change on every commit, but we rebuild it from scratch on every commit. We can start caching it more to speed things up. This PR introduces...

  • caching for the test project fixture
    • we maintain two caches, one that has the test project fixture with yarn rwfw project:deps and yarn install run, and one that has the rest of the steps run. The first cache is invalidated when we install new modules, the second, when we change @redwoodjs/* packages' source code (far more common)
  • a smoke-tests directories that uses the new playwright config for starting a server: https://playwright.dev/docs/api/class-testconfig#test-config-web-server
  • caching for the RedwoodJS framework's install step
    • we always cached yarn's cache, but now we cache it's install state and node_modules

I think we'd benefit from faster CI at the cost of the drawbacks. But here they are:

  • caching can introduce false positives. This is the main one. I feel like caching node_modules is a little dangerous since some packages have a postinstall step and configure things outside the framework, like Cypress and Playwright. But I feel like it's worth trying to figure out what these problematic packages are
  • with the new playwright config for starting a server, you can't run all the smoke tests all at once anymore
    • to be fair, I'm not sure you really want to do this anyway. Not only is the logging virtually unreadable and useless, usually if a smoke test fails in CI, it's one of them, not all of them. And if it's all of them, you have a bug that affects all of them and don't need to run all of them to figure it out
    • there's an open issue about configuring per-project webServers that would let us recombine them if we want: [Feature] Configure web servers per project microsoft/playwright#22496

If we like this strategy, I can do the same for the e2e test, and split the smoke-tests workflow into multiple jobs.

Working todos

  • fix script in package.json for running a smoke-test locally
  • re-enable replay
  • configure set-up-yarn-cache action to cache always

@jtoar jtoar added the release:chore This PR is a chore (means nothing for users) label May 30, 2023
uses: ./.github/actions/set-up-yarn-cache

- name: 🐈 Yarn install
run: yarn install --inline-builds
Copy link
Contributor Author

Choose a reason for hiding this comment

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

inline builds will let us debug linking errors. They're not as common as they used to be, but we used to have with @vscode/ripgrep

@jtoar jtoar force-pushed the ds-ci/start-caching-smoke-tests branch from 14f73ea to fc76694 Compare May 30, 2023 00:48
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead of trying to add caching to the test-project script, I pulled out the relevant steps here. This isn't ideal as we don't want things to get out of sync, but I don't want to make that script any more complicated than it already is. Plus all we ever do here in CI is copy and link the fixture

@replay-io
Copy link

replay-io bot commented May 30, 2023

@jtoar jtoar marked this pull request as ready for review May 30, 2023 18:47
@thedavidprice
Copy link
Contributor

@jtoar re:

caching can introduce false positives. This is the main one. I feel like caching node_modules is a little dangerous since some packages have a postinstall step and configure things outside the framework, like Cypress and Playwright. But I feel like it's worth trying to figure out what these problematic packages are

Is there a TTL equivalent for the cache mechanism... e.g. could we make sure we're invalidating every 12 or 24 hours?

@jtoar
Copy link
Contributor Author

jtoar commented Jun 1, 2023

@thedavidprice it doesn't seem like @actions/cache includes any TTL logic; we'd have to implement that ourselves which probably wouldn't be too hard. But I think 12-24 hours is too aggressive; in that timeframe, you're actively working on a PR. But if you haven't pushed any commits for three days to a week, it could make sense to consider the cache too stale to use once you've gotten back to it.

I can make the caching for the test project even "cleaner" per PR by adding the PR's branch to the key so that it doesn't even benefit from other PRs. But we already don't include a lock file in the test project, so every PR's test project gets a completely different lock file (which is part of the reason why it takes so long to generate on Windows). But once a PR gets a test project, it seems reasonable to keep it and it's node_modules state unless you install new packages in the PR, in which case, everything is re-done.

@thedavidprice
Copy link
Contributor

But we already don't include a lock file in the test project, so every PR's test project gets a completely different lock file (which is part of the reason why it takes so long to generate on Windows). But once a PR gets a test project, it seems reasonable to keep it and it's node_modules state unless you install new packages in the PR, in which case, everything is re-done.

This is a great insight and addresses a primary concern I was thinking about (i.e. lockfile). And if a PR goes dormant for a while (even as little as a few days), it's most likely there have been changes in main that will invalidate the cache when the PR branch is updated. ('cause renovate...)

This reasoning is good enough for me to negate the need to add any custom time-based invalidation.

Well done 🚀

@jtoar jtoar merged commit d818939 into main Jun 3, 2023
@jtoar jtoar deleted the ds-ci/start-caching-smoke-tests branch June 3, 2023 03:02
@redwoodjs-bot redwoodjs-bot bot added this to the next-release milestone Jun 3, 2023
@jtoar
Copy link
Contributor Author

jtoar commented Jun 3, 2023

@thedavidprice, after reviewing with @Tobbe, we made the test project cache stricter by scoping it to the PR for now. We have an idea about how to safely widen it, but till then, if any PR can rewrite the cached test project used by any other PR, there's definitely going to be some edge cases we could get into. So for now, things will be faster, but the first run will always take a while. Still better than what we had before.

Copy link
Contributor

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:chore This PR is a chore (means nothing for users)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants