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

Fast cypress #8459

Merged
merged 8 commits into from
Jul 17, 2024
Merged

Fast cypress #8459

merged 8 commits into from
Jul 17, 2024

Conversation

wpears
Copy link
Member

@wpears wpears commented Jun 6, 2024

This PR changes how our cypress functional tests run against changes to code in this repo. It does this by defining a mapping between files & directories of application code and corresponding directories of cypress tests that need to be run. This mapping is defined in the spec-map.js file and the changed files that are tested against this map are discovered via diffing against main (locally) / using a changed-files action that wraps the diff (in GHA). How this actually affects Cypress is by using the mapping to rewrite a cypress config file with constrained spec paths.

Cypress can be run locally using this mapping with yarn run fast-cypress. The entire suite can be run locally using yarn run cypress (as normal).

Broadly speaking, the answer to "what Cypress tests should run against this PR" is usually decently easy to answer and so I expect this naive mapping to mostly function without many false negatives. Indeed, any detected false negatives can just simply be added to the map.

Some choices I made:

  • If NO changed files match relevant tests, cypress runs a dummy test that always passes.
  • If the spec map, any requirements, or package.json (sorry ans_update_deps) are changed, ALL the specs run, as there can be no guarantee that random tests won't fail

Let me know what y'all think!

@wpears wpears force-pushed the fast-cypress branch 4 times, most recently from f9ce4fe to 23a698d Compare June 12, 2024 14:27
Also, provide a `fast-cypress` npm script to call a similar local
command. Cypress tests should now only be invoked when files they test
are changed. If this is found incomplete, more files can be defined in
the `spec-map.js` file, as needed.
@wpears wpears requested a review from a team June 12, 2024 22:34
@wpears wpears marked this pull request as ready for review June 12, 2024 22:34
@chosak
Copy link
Member

chosak commented Jun 13, 2024

I like this idea in theory but worry about false negatives. There is some common Python code that underlies most of the functionality being tested but isn't included in the map; for example anything in Wagtail relies on cfgov/v1, all Django requests go through middleware and probably other stuff in cfgov/core, all templates rely on base.html.

Indeed, any detected false negatives can just simply be added to the map.

The risk is that we don't detect the false negative until way after a change has gone live by which point it may not be a simple matter to fix or undo. If the goal here is to speed up checks/merges to main, should we consider reintroducing the full test suite as a gate to production deployment?

@wpears
Copy link
Member Author

wpears commented Jun 13, 2024

for example anything in Wagtail relies on cfgov/v1, all Django requests go through middleware and probably other stuff in cfgov/core, all templates rely on base.html

I added the python files in core and base.html to the full suite trigger in 416e252
I also made any python changes in cfgov/v1 call all component and admin tests in 35d862335d8623

The risk is that we don't detect the false negative until way after a change has gone live by which point it may not be a simple matter to fix or undo

I agree that this approach inherently carries some level of risk. However, I think it's actually smaller than you might think, namely (1) areas where the files changed SHOULD trigger certain tests but don't, where (2) those tests would fail under the given change in (3) a way that points to an unintended behavior/bug that (4) isn't caught in development by the code author or (5) the reviewer. Certainly possible, but I'd bet reasonably rare and fairly minor in reach/scope if it did happen.

@contolini
Copy link
Member

If the goal here is to speed up checks/merges to main, should we consider reintroducing the full test suite as a gate to production deployment?

To assuage these concerns should we run the full suite against the merge queue? i.e.

jobs:
  cypress:
    if: ${{ vars.GITHUB_EVENT_NAME == 'merge_group' }}
    steps:
      ...

  fast-cypress:
    if: ${{ vars.GITHUB_EVENT_NAME == 'pull_request' }}
    steps:
      ...

@wpears
Copy link
Member Author

wpears commented Jun 26, 2024

To assuage these concerns should we run the full suite against the merge queue?

I think that would be a decent option. I do kinda want to get to a place where we are only running tests that we really think need to run though, to reduce all the various delays that aren't usually necessary.

I'd almost rather just run it on a push to main, which would let us know early that there's a failure but wouldn't gate the merges on the full suite (so 97% of the time merges -> deploys are quicker)

Copy link
Member

@contolini contolini left a comment

Choose a reason for hiding this comment

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

I vote to give this a whirl and adjust as needed in the future

@wpears wpears added this pull request to the merge queue Jul 16, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 16, 2024
@wpears wpears added this pull request to the merge queue Jul 17, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 17, 2024
@wpears wpears added this pull request to the merge queue Jul 17, 2024
Merged via the queue into main with commit fcb4eab Jul 17, 2024
11 checks passed
@wpears wpears deleted the fast-cypress branch July 17, 2024 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants