Implement Test Selection for E2E Test Specs

pjhill commented 3 years ago

Issue Description

Given that the Platform is moving toward Autonomous Deployments for App Teams in the middle-term, we need to come up with a solution for test selection that works with the autonomous deployment paradigm. Currently, Front-end Tools team is doing discovery on --

updates that need to be made to the DevOps process to support isolated deployments. This includes Review Instances and the staging / dev deployment processes.

The proposed logic informed by the Shopify case study, Bill's research, and Holden's recent discovery are as follows --

If any of the file paths in a PR don't start with /src/applications, run all tests (assume change affects all apps)
Else iterate through all files in PR, create list of test files by grabbing the spec paths in /src/applications/[app_name]], pass list to Cypress, run tests

Additionally, we want to consider a split approach depending on the event in GitHub Actions. For example, a push to a branch would trigger the selected for tests only; whereas, a PR merging to master would trigger the execution of all available E2E test specs.

Tasks

[x] Modify the CI workflow to include logic that scans modified files and retrieves a list of relevant Cypress E2E test specs
[x] Modify the CI workflow to execute only the relevant Cypress E2E test specs across our parallelized Cypress runner infrastructure

Acceptance Criteria

[x] Pushes to branches trigger the execution of a subset of relevant Cypress E2E test specs when files within a subset of application folders have been changed
[x] Pushes to branches trigger the execution of all Cypress E2E test specs when files outside of the application folder have been changed
[x] Pushes to branches that involve changes to files in more than one application subfolder trigger the execution of the Cypress specs contained in each changed application's folder structure
[ ] The creation of a Pull Request that will merge a branch into master triggers the execution of all available Cypress E2E test specs

holdenhinkle commented 3 years ago

PR - https://github.com/department-of-veterans-affairs/vets-website/pull/18111

holdenhinkle commented 3 years ago

I quickly coded up most of the logic outlined in the ADR, but I'm having trouble getting the list of changed files for the repo.

Coding is fast, but configuring the GitHub Workflow file is what's slowing me down.

I'm still at it...

holdenhinkle commented 3 years ago

Well, well, well...

It's 9:13 and I'm still at it...

And just had a breakthrough...

Instead of

run: git diff --name-only master...

I just discovered it should be

run: git diff --name-only origin/master...

even though run: git diff --name-only master... works locally.

I found it here - https://github.com/actions/checkout/issues/296

Now I'm able to get a list of the changed files in a branch. Yay!

holdenhinkle commented 3 years ago

Helpful links:

I tried using this action but couldn't get it to work, not matter what I tried. It would only compare the last two commits in the branch.

holdenhinkle commented 3 years ago

If all files are .md files (so there are no tests to run) and we pass in an empty string to the Cypress run command (see the end of this command) all the tests run:

$ cypress run --config-file config/cypress.json --browser chrome --headless --reporter cypress-multi-reporters --reporter-options configFile=config/cypress-reporters.json --spec ''

Example - https://github.com/department-of-veterans-affairs/vets-website/runs/3242645388?check_suite_focus=true

holdenhinkle commented 3 years ago

I've got test selection working for changes in application files - https://github.com/department-of-veterans-affairs/vets-website/runs/3243386667?check_suite_focus=true

holdenhinkle commented 3 years ago

Runs all tests when file not in src/applications and one or more files in src/applications are commited to branch - https://github.com/department-of-veterans-affairs/vets-website/runs/3243827702?check_suite_focus=true => this is good!

holdenhinkle commented 3 years ago

Test selection is working.

I spent the second half of the day researching how to conditionally set the number of containers required for the selected Cypress tests. Basically, I need to conditionally set the value of ci_node_index here:

      matrix:
        ci_node_index: [0, 1, 2, 3, 4, 5, 6, 7]

I'm about 2/3 of the way coding this part up, and feel confident about my solution. I'm hoping to finish it tomorrow.

I came across the following resource for conditionally setting a matrix (I'm not using this approach though):

https://stackoverflow.com/questions/65384420/how-to-make-a-github-action-matrix-element-conditional

holdenhinkle commented 3 years ago

Package - https://github.com/actions/toolkit/tree/main/packages/core

This package provides a core.exportVariable function - core.exportVariable('envVar', 'Val');

Usage example (for both .js and .yml files) - https://github.com/actions/runner/issues/317#issuecomment-579437440

I'm hoping this will allow me to easily capture the value for ci_node_index from a js script in one job, and use the value to set ci_node_index in the cypress-test job.

But GHA seems mostly unresponsive right now...

holdenhinkle commented 3 years ago

My solution is 100% coded up. GHA is down so I can't run it. I'm sure there's going to be some debugging to do.

My solution basically reduces the number of containers needed to run the tests, depending on the number of tests selected. If only a few tests are selected, text execution time will be greatly reduced.

Otherwise, if there > 20 tests to run for example, the only benefit will be using fewer containers to run the tests. I'm hoping that will reduce costs and free up the runners to pop other jobs off the queue.

Here’s what that part of the code looks like right now:

if (numTests === 0) {
  core.exportVariable('NUM_CONTAINERS', 0);
} else if (numTests < 20) {
  core.exportVariable('NUM_CONTAINERS', 1);
  core.exportVariable('CI_NODE_INDEX', [0]);
} else if (numTests < 40) {
  core.exportVariable('NUM_CONTAINERS', 2);
  core.exportVariable('CI_NODE_INDEX', [0, 1]);
} else if (numTests < 60) {
  core.exportVariable('NUM_CONTAINERS', 3);
  core.exportVariable('CI_NODE_INDEX', [0, 1, 2]);
} else if (numTests < 80) {
  core.exportVariable('NUM_CONTAINERS', 4);
  core.exportVariable('CI_NODE_INDEX', [0, 1, 2, 3]);
} else if (numTests < 100) {
  core.exportVariable('NUM_CONTAINERS', 5);
  core.exportVariable('CI_NODE_INDEX', [0, 1, 2, 3, 4]);
} else if (numTests < 120) {
  core.exportVariable('NUM_CONTAINERS', 6);
  core.exportVariable('CI_NODE_INDEX', [0, 1, 2, 3, 4, 5]);
} else if (numTests < 140) {
  core.exportVariable('NUM_CONTAINERS', 7);
  core.exportVariable('CI_NODE_INDEX', [0, 1, 2, 3, 4, 5, 6]);
} else {
  core.exportVariable('NUM_CONTAINERS', 8);
  core.exportVariable('CI_NODE_INDEX', [0, 1, 2, 3, 4, 5, 6, 7]);
}

if (numTests > 0) core.exportVariable('TESTS', tests);

i imagine this can be optimized in different ways…

holdenhinkle commented 3 years ago

I have test selection working and the number of containers needed to run the tests is being set dynamically based on the number of tests selected.

Here's a nice little example:

I changed two files in the facility-locator app and one file in the search app. Only the facility-locator and search Cypress tests should run.

Image 1:

12 spec files
run in 1 container
set ci_node_index to [0]

Image 2:

Only 1 container was created
Tests ran in 2:28 mins
1 test failed because I edited a file causing it to fail

holdenhinkle commented 3 years ago

Per Tim Wright's suggestion, I now include tests in src/platform when only selected tests are to run:

function selectedTests() {
  const tests = [];
  const applicationNames = pathsOfChangedFiles.map(filePath => {
    return filePath.split('/')[2];
  });

  [...new Set(applicationNames)].forEach(name => {
    const selectedTestsPattern = path.join(
      __dirname,
      '../..',
      'src/applications',
      `${name}/tests/**/*.cypress.spec.js?(x)`,
    );

    tests.push(...glob.sync(selectedTestsPattern));
  });

  // always run the tests in src/platform
  const defaultTestsPattern = path.join(
    __dirname,
    '../..',
    'src/platform',
    '**/tests/**/*.cypress.spec.js?(x)',
  );

  tests.push(...glob.sync(defaultTestsPattern));
  return tests;
}

Slack convo - https://dsva.slack.com/archives/C01CHAY3ULR/p1628213652045200

holdenhinkle commented 3 years ago

Link to checks for only running selected tests and tests in src/platform - https://github.com/department-of-veterans-affairs/vets-website/runs/3262812500?check_suite_focus=true

holdenhinkle commented 3 years ago

We're using TestRail to manually test test selection.

TestRail plan - https://dsvavsp.testrail.io/index.php?/plans/view/2089

holdenhinkle commented 3 years ago

To-do:

Add test selection logic for .md file and file(s) in src/applications => should only run tests for application(s) in src/applications path

holdenhinkle commented 3 years ago

Add test selection logic for when there are both .md file and file(s) in src/applications
- should only run tests for application(s) in src/applications path

=> Done

holdenhinkle commented 3 years ago

Link - https://github.com/department-of-veterans-affairs/vets-website/runs/3280480291?check_suite_focus=true

It seems that when only 1 container is used to run tests, the 'Testing Reports' job fails:

Step: 'Download Cypress JUnit test results'

Run actions/download-artifact@v2
Starting download for cypress-junit-test-results
Error: Unable to find an artifact with the name: cypress-junit-test-results

The 'Archive JUnit Test Results' step in the 'Cypress E2E Tests (0)' job wasn't run so there's nothing to download:

This doesn't happen when all tests run (when 8 containers are used).

Let me see if it happens when more than 1 container is used...

holdenhinkle commented 3 years ago

Ah, when a test fails in a 'Cypress E2E Tests (n)' job, the 'Archive JUnit test results' step is skipped...

How to handle the 'Download Cypress JUnit test results' in the 'Testing Reports' job if only 1 container and a test fails?

holdenhinkle commented 3 years ago

I added if: ${{ always() }} to 'Archive JUnit test results' step in 'cypress-tests' job.

That fixed it.

Link - https://github.com/department-of-veterans-affairs/vets-website/runs/3281201763?check_suite_focus=true

Now the the 'Archive JUnit Test Results' step in the 'Cypress E2E Tests (0)' logged this:

Run actions/upload-artifact@v2
/usr/bin/docker exec  2301eabbc704e44327cdcb44eeeb8186463930735d864c361b4b9cf4fda1caec sh -c "cat /etc/*release | grep ^ID"
With the provided path, there will be 16 files uploaded
Total size of all the files uploaded is 6054 bytes
Finished uploading artifact cypress-junit-test-results. Reported size is 6054 bytes. There were 0 items that failed to upload
Artifact cypress-junit-test-results has been successfully uploaded!

This means that the number of test files in a job with a failed spec weren't being reported in the JUnit Test Results...

holdenhinkle commented 3 years ago

All manual testing is complete.

I think I found the reason for the difference in the number of files/tests that Mochawesome and JUnit are reporting.

PR submitted - https://github.com/department-of-veterans-affairs/vets-website/pull/18111

holdenhinkle commented 3 years ago

This ticket is complete except for "The creation of a Pull Request that will merge a branch into master triggers the execution of all available Cypress E2E test specs". We (Peter and I) should talk about this.

holdenhinkle commented 3 years ago

FAQ for VFS Engineers - https://docs.google.com/document/d/1lyFKFCR-UJoUUtpzbSj7PbGS9j3ZdNycA2Wid0XF4jU/edit#

holdenhinkle commented 3 years ago

I implemented the following today after speaking with Darius:

Run selected tests on push
Run all tests on merge to master

GitHub is down right now so I haven't been able to test it out...

holdenhinkle commented 3 years ago

I tested my update for this...

@ddzz asked if the E2E tests would still be run when a PR is merged to master. That had changed--only the selected tests* would run.

I updated the workflow so 1) selected tests* run on each push, and 2) all tests run on merge to master.

*By 'selected tests' I mean any tests that should run, depending on the changed files in the branch, which means all tests would run if a 'global' file has been changed. The logic powering test selection hasn't changed.

...and got the expected results. I asked for a few re-reviews of the PR.

holdenhinkle commented 3 years ago

Merged...

pjhill commented 3 years ago

Hooray! Any changes to test selection can be collected and worked as a post-MVP effort.

erikphansen commented 3 years ago

Hello!

I just became aware of this work and should say off the bat that I didn't look at the related PR that was merged so I could be concerned about nothing. I am also nowhere near being an expert in CI (GitHub Actions or otherwise). But I saw this:

If any of the file paths in a PR don't start with /src/applications, run all tests (assume change affects all apps)

Else iterate through all files in PR, create list of test files by grabbing the spec paths in /src/applications/[app_name]], pass list to Cypress, run tests

I'm not sure, given the current state of vets-website, if it's really possible to accurately determine which Cypress tests should be run based on which code was changed in a PR.

At a high level two things jump out at me:

There are instances where platform code is importing from applications, which should never happen, but it does.
There are instances where application A is importing code from application B. Also should never happen, but it does.

Because of this, it seems less-than-easy to determine which tests need to run based on which code changed.

And then this bit:

a push to a branch would trigger the selected for tests only; whereas, a PR merging to master would trigger the execution of all available E2E test specs.

So, if I'm understanding this, we could end up in a situation where tests pass on my branch, I then merge to master, CI then fails because all the tests ran, revealing a problem. So a deployment doesn't happen. That's good. But now we've got master in a failing state and we might not be able to easily see if our PR to fix things in master will work because the failing tests might not run on our PR branch.

holdenhinkle commented 3 years ago

@erikphansen Thanks for commenting Erik. Do you have examples of this you can share?

There are instances where platform code is importing from applications, which should never happen, but it does.

There are instances where application A is importing code from application B. Also should never happen, but it does.

erikphansen commented 3 years ago

https://github.com/department-of-veterans-affairs/vets-website/blob/79208e35451ed0817f91b75a3127f72d238004dc/src/platform/forms/save-in-progress/ApplicationStatus.jsx#L6-L10

https://github.com/department-of-veterans-affairs/vets-website/blob/fb3f20466fb26a07eb0774d3c0accd648b254d70/src/platform/user/authentication/utilities.js#L10

There are lots of places applications are importing from other applications, here's a couple:

https://github.com/department-of-veterans-affairs/vets-website/blob/310adba721c54cfb7fdd92301d048ae8b90bbe93/src/applications/hca/enrollment-status-helpers.jsx#L7

https://github.com/department-of-veterans-affairs/vets-website/blob/e4fca21d2f2cd0e6b6c890aa6285ac14717b1dab/src/applications/personalization/rated-disabilities/components/RatedDisabilityView.jsx#L5

holdenhinkle commented 3 years ago

Thanks.

Regarding your first two examples, all tests in src/platform always run, so instances like this have been handled.

The second two examples are problematic. I'll investigate further.

erikphansen commented 3 years ago

Regarding your first two examples, all tests in src/platform always run, so instances like this have been handled

Good to know!

I mean, platform code probably shouldn't be dependent upon application code, but that's a different issue 😆

holdenhinkle commented 3 years ago

I think we're going to create a test selection whitelist. Apps that do cross-app imports will not be on it, but can update their app, then add their app to the whitelist to benefit from test selection. I scheduled a meeting tomorrow to discuss this further with member of my team. I sent you an invite incase you'd like to join us.

Thanks for bringing this issue to our attention!

holdenhinkle commented 3 years ago

My latest idea, which I think is much better than a whitelist, because it this doesn't need to be maintained and still incorporates test selection:

Build a cross-app import/require graph for each app that runs on each push:

Check all the imports and requires in all files in src/applications
- If an import/require is outside of the application of the file currently being checked and that file is in src/applications, add that application to the graph:

const graph = {
    application_1: [ application_1, application_2],
    application_2: [ application_2, application_1,],
    application_3: [application_3],
    application_4: [application_4],
}

Then, get the application names that correspond to the files changed in the branch, lookup those applications in the graph, create a list of applications whose tests need to run, select those tests, run the tests...

What ifs:

Global file? Only need to run all tests if file is changed
src/platform file? Handled -- these tests always run

pjhill commented 3 years ago

MVP of this did not include the graph.
Post-MVP release does include a graph to select additional E2E test specs when an application imports another application's files.

bahmutov commented 3 years ago

Hi @holdenhinkle I came on this thread randomly looking to implement running changed / added Cypress specs first, before running all specs. Thanks for your comments, it is always something simple, isn't it - like origin/master instead of master :) Cheers

department-of-veterans-affairs / va.gov-team