-
Notifications
You must be signed in to change notification settings - Fork 11
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
Move verification tests to own workflow #4522
Conversation
64ca22c
to
7d05bfa
Compare
7d05bfa was added just to give me confidence the github workflow works. It should be removed before merging to |
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 know I've written quite a few comments, but this does look good and I'm looking forward to less flaky tests.
The other thing to consider as part of this change is that we'd want to be notified in Slack when the workflow fails. I think there's prior art from ehrql that you can look at to figure out how to do that.
9121a1b
to
7f9547f
Compare
Thanks, @lucyb for you excellent comments! I've added what I think should be the right slack integration, I'll induce a verification test failure tomorrow and see if it posts to slack :) |
7fe81d8
to
ed3d0ec
Compare
ed3d0ec
to
6d0f541
Compare
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.
Just one tiny comment, but please remove the two temp
commits before merging!
This looks really nice overall. Thanks for updating the various pieces of documentation and good spot with the docker tests, I'd forgotten about those.
Runs at 00:47 every night Based on test job from main.yml, only change is to just command called.
Update devlopers.md and testing.md to take account of new `just` action names following the separation of verification tests.
The verification tests don't actually make use of the database, so make the database an optional fixture and mark it as disabled for the verification tests. As it's no longer needed, the database setup is also removed from the verification test github workflow.
6d0f541
to
59943e6
Compare
Closes #4434