Skip to content
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

Merged
merged 9 commits into from
Aug 14, 2024

Conversation

Jongmassey
Copy link
Contributor

@Jongmassey Jongmassey commented Aug 8, 2024

Closes #4434

@Jongmassey Jongmassey force-pushed the Jongmassey/move-verification-tests branch from 64ca22c to 7d05bfa Compare August 9, 2024 15:30
@Jongmassey
Copy link
Contributor Author

7d05bfa was added just to give me confidence the github workflow works. It should be removed before merging to main

@Jongmassey Jongmassey marked this pull request as ready for review August 9, 2024 15:41
Copy link
Contributor

@lucyb lucyb left a 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.

.github/workflows/verification-tests.yml Outdated Show resolved Hide resolved
.github/workflows/verification-tests.yml Outdated Show resolved Hide resolved
.github/workflows/verification-tests.yml Outdated Show resolved Hide resolved
.github/workflows/verification-tests.yml Outdated Show resolved Hide resolved
.github/workflows/verification-tests.yml Outdated Show resolved Hide resolved
justfile Show resolved Hide resolved
docs/adr/0021-move-verification-tests.md Show resolved Hide resolved
docs/adr/0021-move-verification-tests.md Outdated Show resolved Hide resolved
docs/adr/0021-move-verification-tests.md Outdated Show resolved Hide resolved
justfile Outdated Show resolved Hide resolved
@Jongmassey Jongmassey force-pushed the Jongmassey/move-verification-tests branch 4 times, most recently from 9121a1b to 7f9547f Compare August 12, 2024 20:26
@Jongmassey
Copy link
Contributor Author

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 :)

@Jongmassey Jongmassey force-pushed the Jongmassey/move-verification-tests branch from 7fe81d8 to ed3d0ec Compare August 13, 2024 09:49
@Jongmassey Jongmassey force-pushed the Jongmassey/move-verification-tests branch from ed3d0ec to 6d0f541 Compare August 13, 2024 10:27
@Jongmassey Jongmassey requested a review from lucyb August 14, 2024 07:30
Copy link
Contributor

@lucyb lucyb left a 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.

docs/adr/0021-move-verification-tests.md Show resolved Hide resolved
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.
@Jongmassey Jongmassey force-pushed the Jongmassey/move-verification-tests branch from 6d0f541 to 59943e6 Compare August 14, 2024 09:55
@Jongmassey Jongmassey enabled auto-merge August 14, 2024 09:56
@Jongmassey Jongmassey merged commit ed9974a into main Aug 14, 2024
8 checks passed
@Jongmassey Jongmassey deleted the Jongmassey/move-verification-tests branch August 14, 2024 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Verification tests fail in CI with 403 status codes
2 participants