CHANGE @W-18396029@ Running tests against only affected packages #283
Conversation
| @@ -7,6 +7,9 @@ charset = utf-8 | |||
| trim_trailing_whitespace = true | |||
| insert_final_newline = true | |||
|
|
|||
| [package.json] | |||
There was a problem hiding this comment.
I got sick of fighting with my IDE about formatting on package.json files, so I just updated the editor config file about it.
| @@ -72,6 +72,9 @@ jobs: | |||
| - name: Support long paths | |||
| run: git config --global core.longpaths true | |||
| - uses: actions/checkout@v4 | |||
| with: | |||
There was a problem hiding this comment.
We're still checking out the same ref, but we need to do it like the validate-packages job does where the history is pulled too.
| npm run build `cat workspace-args.txt` | ||
| npm run test `cat workspace-args.txt` |
There was a problem hiding this comment.
I tried assigning cat workspace-args.txt to a variable and then using that, but for whatever reason that didn't want to work, so I'm doing this instead. It's a little cumbersome but it'll work.
| node ./.github/workflows/verify-pr/identify-affected-workspaces.mjs "$CHANGED_FILES" | ||
| npm run build `cat workspace-args.txt` | ||
| npm run test `cat workspace-args.txt` | ||
| npm run lint |
There was a problem hiding this comment.
LInting is fast, so there's no reason not to lint all the files.
| @@ -83,4 +86,12 @@ jobs: | |||
| with: | |||
| python-version: 3.12 | |||
| - run: npm install | |||
| - run: npm run all | |||
There was a problem hiding this comment.
Technically we're losing the invocation of npm package that is contained by npm run all. If we want it, then I can put it back in.
| const changedPackages = identifyChangedPackages(changedFiles); | ||
| if (changedPackages.length === 0) { | ||
| console.log(`No package-level changes. Using empty workspace arg to test all packages.`); | ||
| fs.writeFileSync(tmpFilePath, ''); |
There was a problem hiding this comment.
In this case (and the similar case above), the file will be empty, meaning that the GHA will run npm run test , causing all packages to be tested.
I had to choose between "changing only top-level files causes no packages to be tested" and "changing only top-level files causes all packages to be tested", and I chose the latter because it was easier to implement.
Also, corner case: if you change top-level files and also one package-level file, then only that package and its dependents will be tested. Are we okay with that?
| const affectedPackages = [...(new Set([...changedPackages, ...dependentPackages]).keys())]; | ||
| displayList('BASED ON THE ABOVE, THE FOLLOWING PACKAGES ARE AFFECTED BY CHANGES, AND WILL REQUIRE TESTING:', affectedPackages); | ||
|
|
||
| const correspondingWorkspaceArgs = affectedPackages.map(name => `--workspace ${name}`); |
There was a problem hiding this comment.
When changes can be tied to specific packages (and the ones that depend on those packages), we can construct a bunch of --workspace blah arguments and write them to a file, then we can use that file to turn npm run test into npm run test --workspace blah1 --workspace blah2 and thereby only run the tests in the affected workspaces.
jfeingold35
requested review from
randi274 and
stephen-carter-at-sf
and removed request for
randi274
.github/workflows/verify-pr.yml
Outdated
| BASE_SHA=${{ github.event.pull_request.base.sha }} | ||
| HEAD_SHA=${{ github.event.pull_request.head.sha }} | ||
| CHANGED_FILES=`git diff --name-only $HEAD_SHA $BASE_SHA` | ||
| node ./.github/workflows/verify-pr/identify-affected-workspaces.mjs "$CHANGED_FILES" |
There was a problem hiding this comment.
Will we have any issues with clipping if the number of changed files is massive?
Would it be better to output this to a file and then read it from the script instead?
| @@ -52,6 +52,10 @@ function displayList(header, list) { | |||
| console.log(''); | |||
| } | |||
|
|
|||
| function readChangedFilesFile(changedFilesFileName) { | |||
| return fs.readFileSync(path.join(__dirname, '..', '..', '..', changedFilesFileName), 'utf-8').split('\n').map(s => s.trim()); | |||
There was a problem hiding this comment.
Can you just use pathToRoot here instead of using __dirname, .., .., .. again?
There was a problem hiding this comment.
Yeah, good call. Thanks.
This PR makes the following changes:
verify-pr.ymljob can now identify which specific packages are affected by the changes in the PR, and will only run the tests in those packages.See this Github Action run from an experimental branch to see it only running tests for
code-analyzer-regex-engineandcode-analyzer-flow-engine, and see the checks for this PR itself for an example of it running on all packages because no specific package was changed.