Pawel plesniak/clearer testing requirements pr template#919
Pawel plesniak/clearer testing requirements pr template#919PawelPlesniak wants to merge 4 commits into
Conversation
emmuhamm
left a comment
emmuhamm
left a comment
There was a problem hiding this comment.
@PawelPlesniak Thanks for fixing this!
Left a couple of minor suggestions that might be worth taking a look at. However, some of them are incredibly minor.
I'll approve this now cuz I'm still happy for this to be merged as is, although I would prefer that they get addressed. Anyways even if you address it I think its below threshold to ask for another review, so with the approval feel free to just merge immediately after changes (if so)
| - [ ] CI workflows fails documented (if present) | ||
| - [ ] Integration tests passed | ||
| - [ ] Integration tests passed (on either np0x or IC HEP clusters) | ||
| - If the reviewer has already ran the relevant tests, this step is optional |
There was a problem hiding this comment.
Technically this is in the reviewer's checklist so I think this line should be removed :p
| - If the reviewer has already ran the relevant tests, this step is optional |
| <!-- - [ ] Drunc integration tests pass (`pytest -m drunc_integration_tests`) Note - at the time of creating this template, these tests have not been written hence remain as a TODO. This will test all the Process Managers in a test bundle. --> | ||
| - [ ] If you have ran the full integration test bundle, leave a comment on the PR stating | ||
| - Which host the integration tests have ran on | ||
| - [Optional] A copy of the test summary |
There was a problem hiding this comment.
Agree that its optional if the tests pass, but if it fails I think its useful to put it in the PR to hone in on where it went wrong and raise a discussion.
Not sure how to write that in the template though
| - [ ] Drunc integration tests pass (`scripts/drunc_integtest_bundle.sh`) | ||
|
|
||
| Once the features are validated and both the unit and integration tests pass, the PRs is ready to be merged. | ||
| Once the above boxes are checked, the PR(s) can be merged. |
There was a problem hiding this comment.
(nitpick):
| Once the above boxes are checked, the PR(s) can be merged. | |
| Once the above boxes are checked, the PR(s) can be merged following the steps below. |
Mainly to remind people about the next steps.
Feel free to ignore though
3 participants
Following the discussion in this week's RC technical meeting, a few clarifications have been added to the PR template to streamline the PR reviewing process.