-
Notifications
You must be signed in to change notification settings - Fork 201
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
Implement Test Selection for E2E Test Specs #28218
Comments
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... |
Well, well, well... It's 9:13 and I'm still at it... And just had a breakthrough... Instead of
I just discovered it should be
even though I found it here - actions/checkout#296 Now I'm able to get a list of the changed files in a branch. Yay! |
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. |
If all files are
|
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 |
Runs all tests when file not in |
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
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 |
Package - https://github.com/actions/toolkit/tree/main/packages/core This package provides a Usage example (for both I'm hoping this will allow me to easily capture the value for But GHA seems mostly unresponsive right now... |
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. 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:
i imagine this can be optimized in different ways… |
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 Image 1: Image 2: |
Per Tim Wright's suggestion, I now include tests in 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 |
Link to checks for only running selected tests and tests in |
We're using TestRail to manually test test selection. TestRail plan - https://dsvavsp.testrail.io/index.php?/plans/view/2089 |
To-do: Add test selection logic for |
=> Done |
I added That fixed it. Now the the 'Archive JUnit Test Results' step in the 'Cypress E2E Tests (0)' logged this:
This means that the number of test files in a job with a failed spec weren't being reported in the JUnit Test Results... |
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 - department-of-veterans-affairs/vets-website#18111 |
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. |
FAQ for VFS Engineers - https://docs.google.com/document/d/1lyFKFCR-UJoUUtpzbSj7PbGS9j3ZdNycA2Wid0XF4jU/edit# |
I implemented the following today after speaking with Darius:
GitHub is down right now so I haven't been able to test it out... |
I tested my update for this...
...and got the expected results. I asked for a few re-reviews of the PR. |
Merged... |
Hooray! Any changes to test selection can be collected and worked as a post-MVP effort. |
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:
I'm not sure, given the current state of At a high level two things jump out at me:
Because of this, it seems less-than-easy to determine which tests need to run based on which code changed. And then this bit:
So, if I'm understanding this, we could end up in a situation where tests pass on my branch, I then merge to |
@erikphansen Thanks for commenting Erik. Do you have examples of this you can share?
|
There are lots of places applications are importing from other applications, here's a couple: |
Thanks. Regarding your first two examples, all tests in The second two examples are problematic. I'll investigate further. |
Good to know! I mean, platform code probably shouldn't be dependent upon application code, but that's a different issue 😆 |
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! |
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:
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:
|
|
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 |
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 --
The proposed logic informed by the Shopify case study, Bill's research, and Holden's recent discovery are as follows --
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
Acceptance Criteria
The text was updated successfully, but these errors were encountered: