-
Notifications
You must be signed in to change notification settings - Fork 1
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
PF-2033, PF-2989: Trigger Unit Tests On PR Commits #116
Conversation
.github/workflows/unitTest.yaml
Outdated
branches: [ '**' ] | ||
|
||
jobs: | ||
run-unit-tests: |
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.
Maybe this should use the CI workflow from the template project? https://github.com/DataBiosphere/terra-java-project-template/blob/main/.github/workflows/build-and-test.yml
The template workflow runs build separately from test.
Maybe that can wait though; we'd also want to get sonar working with coverage.
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.
This was just a quick experiment to see if I could get the tests to run and pass at all. Agree it should use the standard CI workflow
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.
After looking some more I don't think it makes sense to use an exact copy paste of the template repo ci workflow here. It includes some additional logic which isn't relevant for stairway or other libraries such as running integration tests with test runner and building a docker image and running scans against it etc..
Running a build of the binary and the unit test suite in parallel does make sense to me though.
The failure notifications to slack would also make sense to me to add here, but I think that can be done separately since it requires a terraform change to setup the secrets.
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.
Also agree we should have sonar here but I think that needs to be setup separately by infosec.
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.
Thanks so much for doing this!
Do we also want to run connected tests on PRs? (Even if so, I'd say that we want to handle that separately.)
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.
🎉
Adds a github actions workflow to add CI that will run all the unit tests and perform a build of the library upon PR commits and merges to develop for this repo.
Some future improvements we may want to consider adding are asking AppSec to setup Sonar scans for this repo and adding Workflow failure to slack reporting.