-
Notifications
You must be signed in to change notification settings - Fork 181
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
Add nf-test review guidelines #2414
Closed
Closed
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
b7da303
Add nf-test review guidelines
nvnieuwk 1425649
Update src/content/docs/contributing/modules.md
nvnieuwk 669b18f
Update src/content/docs/contributing/modules.md
nvnieuwk 5dab39a
Update src/content/docs/contributing/modules.md
nvnieuwk 85fc03b
Update src/content/docs/contributing/modules.md
nvnieuwk a7d920e
Update src/content/docs/contributing/modules.md
nvnieuwk 28a0885
Update src/content/docs/contributing/modules.md
nvnieuwk d65ddaa
Update src/content/docs/contributing/modules.md
nvnieuwk 8aeec1b
Update src/content/docs/contributing/modules.md
nvnieuwk 2a2e7d8
update meta yml checklist
nvnieuwk 933ca83
fix linting
mashehu File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1082,12 +1082,35 @@ A PR review is the process of examining a new modules' submission or the changes | |
- Ensure that temporary unzipped files are removed to avoid mitigating benefits and worsening problems. | ||
- Ensure that large outputs are compressed with the correct tool (follow guidelines for gzip vs bzip2 vs other options). | ||
|
||
#### In `../tests/modules/nf-core/modulename/main.nf` and `../tests/modules/nf-core/modulename/meta.yml`: | ||
#### In `modules/nf-core/modulename/meta.yml`: | ||
|
||
- Check that there are tests for all outputs, including optional ones. | ||
- Check that the `meta.yml` file has correct documentation links and patterns of files. | ||
- Check that the file has correct documentation links and patterns of files. | ||
- Run the tool help and check that important input (usually optional) has not been missed. | ||
- Check that all outputs are captured by running pytest (e.g. on Gitpod). | ||
|
||
#### In `modules/nf-core/modulename/tests/main.nf.test`: | ||
|
||
- Check that there are tests for all outputs/inputs, including optional ones. Also check for all possible parameter variations that could lead to different outputs. | ||
nvnieuwk marked this conversation as resolved.
Show resolved
Hide resolved
|
||
- [Different assertion types](https://nf-co.re/docs/contributing/tutorials/nf-test_assertions) should be used when a straightforward `process.out` snapshot is not feasible. | ||
nvnieuwk marked this conversation as resolved.
Show resolved
Hide resolved
|
||
- Check that a stub test exists for the module. | ||
nvnieuwk marked this conversation as resolved.
Show resolved
Hide resolved
|
||
- Check that tags for any dependent modules are specified to ensure changes to upstream modules will re-trigger tests for the current module. | ||
nvnieuwk marked this conversation as resolved.
Show resolved
Hide resolved
|
||
- Check that the `assertAll()` function is used and that there is at minimum a success assertion and versions in the snapshot. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should lint for this. could somebody open an issue please? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done #2415 |
||
- Check that the test names describe the test dataset and configuration used, some examples below: | ||
- `test("homo_sapiens - [fastq1, fastq2] - bam")` | ||
- `test("sarscov2 - [ cram, crai ] - fasta - fai")` | ||
- `test("Should search for zipped protein hits against a DIAMOND db and return a tab separated output file of hits")` | ||
- Check that all input data is being referenced using the `modules_testdata_base_path` parameter: | ||
- e.g. `file(params.modules_testdata_base_path + 'genomics/sarscov2/illumina/bam/test.paired_end.sorted.bam', checkIfExists: true)` | ||
nvnieuwk marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
#### In `modules/nf-core/modulename/tests/main.nf.test.snap`: | ||
|
||
- Check that all output files are present in the nf-test snapshot file, or at a minimum, check that the files exist. | ||
- Check that there are no md5sums representing empty files (except for the stub tests) | ||
nvnieuwk marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
#### Migrating pytest to nf-test: | ||
|
||
- Ensure all previous pytest assertions are converted to nf-test | ||
- Confirm that all pytest files are deleted | ||
nvnieuwk marked this conversation as resolved.
Show resolved
Hide resolved
|
||
- The module MUST use an `environment.yml` for conda environments. Previously, modules used a `conda=<name>=<version>` statement in the `main.nf` file, which has since been replaced with a dedicated `environment.yml` file. | ||
nvnieuwk marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
## What is the `meta` map? | ||
|
||
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
that's quite an overkill imo... are people really doing this? I like more of an organic growth of inputs tbh
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.
That was already there 😁 do you want me to remove it?
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.
yes, please.