Update PR template #37
Conversation
nwiltsie
requested review from
tyamaguchi-ucla,
yashpatel6 and
Faizal-Eeman
as code owners
yashpatel6
assigned sorelfitzgibbon, jmlivingstone, jsahrmann, Faizal-Eeman, yashpatel6, j2salmingo and kiarod
|
@uclahs-cds/nextflow-wg Any comments/suggestions for the PR template here? |
| - input csv: <!-- path/to/input.csv --> | ||
| - config: <!-- path/to/xxx.config --> | ||
| - output: <!-- path/to/output --> | ||
| <!-- If you did not run NFTest, please justify _why_ you feel your changes |
There was a problem hiding this comment.
Would it help a reviewer if the justification for not running a test was under a subheader?
There was a problem hiding this comment.
My read is no, just in the interest of keeping things streamlined.
If I did run the tests, the "test skip justification" header is just one more chunk of text that I need to remove.
If I didn't run the tests, the existence of that header would remove a little bit of the social pressure to go back and do so, because clearly other people skip the tests too.
The fact that the CHANGELOG checkbox says "I have added the changes included in this pull request to the CHANGELOG.md" without any weasel words ("I have added the major changes", "I have added the important changes", etc.) has made me go back and update it at the last second. That's the tone I'm going for here with test running.
Totally a debatable topic though! Thoughts?
There was a problem hiding this comment.
I also prefer the current single heading format. In my mind, it's really streamlined for review because you can look in one place and see either the tests or the absence of tests with justification. They're both the same step of "checking tests"
Co-authored-by: Sorel Fitz-Gibbon <[email protected]>
| understand: | ||
| * How you tested the pipeline | ||
| * How others can test it (including paths to config and YAML files) | ||
| * How the results demonstrate the changes in this PR. Examples of this |
There was a problem hiding this comment.
Think the note about "How the results demonstrate the changes in this PR" is great to include Nick. It's good to know why any additional cases outside of the standard NFTest checks were important.
Have you considered adding this justification as one of the explicit fields below 'Additional Case 1'?
There was a problem hiding this comment.
I couldn't think of a good formalization of this that would fit into the existing bulleted list - every prompt I could come up with was just repeating this comment again. Do you have any suggestions?
There was a problem hiding this comment.
yeah it's a bit tough to pin down. In general NFTest would(should) catch any changes that would break the pipeline or corrupt the output, so additional tests would have to be for changes that don't change the final output but change how the final output is produced? right? or am I missing a scenario where additional tests would be used?
candidate field names:
'why' --succinct but maybe too undirected.
'necessity of test' --straightforward but wordy.
'signal tested' --if it's not an change to output, what was the signal that was used to see if the test passed?
am I on the right track here?
This updates and fills in the templated parts of the pull request template.
In addition to removing the template text and cleaning up a few markdown quirks, I overhauled the
Testing Resultssection. My impression is that users (myself included) almost never completely fill out the existing "Case 1: sample, input csv, ..." table (if we do at all). I switched instead to "requiring" the path to and the last few lines of the NFTest log file, or a justification of why testing isn't required (for example, this PR doesn't require testing because it's a documentation-only change). If/when anyone cares to go digging, the NFTest logfile has the details of the test run and is stored adjacent to the output files themselves.This is functionally identical to what I wrote up for #36, and I think it's simultaneously more informative and more usable, but I welcome any feedback on it! If everyone likes it I think this (or something like it) could be a good template to apply to other repos making use of NFTest.
Checklist
I have read the code review guidelines and the code review best practice on GitHub check-list.
I have reviewed the Nextflow pipeline standards.
The name of the branch is meaningful and well formatted following the standards, using [AD_username (or 5 letters of AD if AD is too long)]-[brief_description_of_branch].
I have set up or verified the branch protection rule following the github standards before opening this pull request.
I have added my name to the contributors listings in the
manifestblock in thenextflow.configas part of this pull request, am listedalready, or do not wish to be listed. (This acknowledgement is optional.)
I have added the changes included in this pull request to theI don't want to conflict with Make output deterministic, add NFTest case #36 for nowCHANGELOG.mdunder the next release version or unreleased, and updated the date.I have updated the version number in the
metadata.yamlandmanifestblock of thenextflow.configfile following semver, or the version number has already been updated. (Leave it unchecked if you are unsure about new version number and discuss it with the infrastructure team in this PR.)I have tested the pipeline on at least one A-mini sample.