-
Notifications
You must be signed in to change notification settings - Fork 91
chore: reorganise GA to check and build once and run tests with matrix strategy VSCODE-703 #1133
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
base: main
Are you sure you want to change the base?
Conversation
Earlier snyk was never scanning the vscode project because of the combination of project auto-detection and the presence of .vscode-test folder which contains several directories with package.json files. This commit disables the auto-detection so that snyk run tests on the current project. Additionally the current project was having a problem with a package declaring optional dependencies. These optional dependencies were platform specific so for any platform, all the optional dependencies will never be installed, only the ones that are platform compatible. Snyk requires what is declared in package-lock.json to be also present in node_modules folder which is why it would've failed. In the same commit, we added a pre and post test hook to remove the identified problematic optional dependencies from package-lock file before running the test and then restore it when the test is finished.
Earlier we were running check, build and tests on each OS in our matrix which did not allow us to test whether a build on ubuntu would work fine on Windows / MacOS or not. With this commit we are re-organising to check and build once on ubuntu and use the final artifact later in the test job to run tests against. Accordingly the test-and-build-from-fork and draft-release workflows have been modified to adapt the same change.
dedcaad to
de36329
Compare
himanshusinghs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am unable to approve (as author) but looks good to me 👍. I added a small question for clarification.
| status-check: | ||
| name: Test and Build | ||
| runs-on: ubuntu-latest | ||
| needs: [build-and-package, test] | ||
| if: always() | ||
| steps: | ||
| - name: Check job results | ||
| run: | | ||
| if [ "${{ needs.build-and-package.result }}" == "failure" ] || [ "${{ needs.test.result }}" == "failure" ]; then | ||
| echo "One or more jobs failed" | ||
| exit 1 | ||
| elif [ "${{ needs.build-and-package.result }}" == "cancelled" ] || [ "${{ needs.test.result }}" == "cancelled" ]; then | ||
| echo "One or more jobs were cancelled" | ||
| exit 1 | ||
| else | ||
| echo "All jobs completed successfully or were skipped" | ||
| exit 0 | ||
| fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious about this, why was this needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah this seems unnecessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was hanging without this. Perhaps there's a more elegant solution, but its not just AI slop. 😅
| name: Test and Build | ||
| runs-on: ubuntu-latest | ||
| needs: [build-and-package, test] | ||
| if: always() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why have this if?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it was hanging, when running a normal build, this would start but never resolve, so the checks never passed.
Description
Earlier we were running check, build and tests on each OS in our matrix which did not allow us to test whether a build on ubuntu would work fine on Windows / MacOS or not.
With this commit we are re-organising to check and build once on ubuntu and use the final artifact later in the test job to run tests against.
Accordingly the test-and-build-from-fork and draft-release workflows have been modified to adapt the same change.
This PR simply re-organise the existing actions and does not introduce any install tests just yet. That will come in a different PR.
Checklist
Motivation and Context
Open Questions
Dependents
Types of changes