Skip to content

Add more nf tests #655

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.

Sign up for GitHub

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

Jump to bottom
Open
sofstam wants to merge 106 commits into
base: dev
Choose a base branch
from
Open

Add more nf tests #655

sofstam wants to merge 106 commits into from
+1,128 −26

Conversation

@sofstam
Copy link
Collaborator

@sofstam sofstam commented Oct 9, 2025 •
edited by rannick
Loading

Add nf-test for test profile

PR checklist

  • This comment contains a description of changes (with reason).
  • If you've fixed a bug or added code that should be tested, add tests!
  • If you've added a new tool - have you followed the pipeline conventions in the contribution docs
  • If necessary, also make a PR on the nf-core/taxprofiler branch on the nf-core/test-datasets repository.
  • Make sure your code lints (nf-core pipelines lint).
  • Ensure the test suite passes (nextflow run . -profile test,docker --outdir <OUTDIR>).
  • Check for unexpected warnings in debug mode (nextflow run . -profile debug,test,docker --outdir <OUTDIR>).
  • Usage Documentation in docs/usage.md is updated.
  • Output Documentation in docs/output.md is updated.
  • CHANGELOG.md is updated.
  • README.md is updated (including new tool citations and authors/contributors).

@github-actions
Copy link

github-actions bot commented Oct 9, 2025 •
edited
Loading

nf-core pipelines lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 1bbc577

+| ✅ 289 tests passed       |+
!| ❗  18 tests had warnings |!
Details

❗ Test warnings:

  • files_exist - File not found: conf/igenomes.config
  • files_exist - File not found: conf/igenomes_ignored.config
  • pipeline_todos - TODO string in nextflow.config: Specify any additional parameters here
  • local_component_structure - kraken2_standard_report.nf in modules/local should be moved to a TOOL/SUBTOOL/main.nf structure
  • local_component_structure - krona_cleanup.nf in modules/local should be moved to a TOOL/SUBTOOL/main.nf structure
  • local_component_structure - visualization_krona.nf in subworkflows/local should be moved to a SUBWORKFLOW_NAME/main.nf structure
  • local_component_structure - shortread_complexityfiltering.nf in subworkflows/local should be moved to a SUBWORKFLOW_NAME/main.nf structure
  • local_component_structure - profiling.nf in subworkflows/local should be moved to a SUBWORKFLOW_NAME/main.nf structure
  • local_component_structure - longread_hostremoval.nf in subworkflows/local should be moved to a SUBWORKFLOW_NAME/main.nf structure
  • local_component_structure - longread_preprocessing.nf in subworkflows/local should be moved to a SUBWORKFLOW_NAME/main.nf structure
  • local_component_structure - shortread_fastp.nf in subworkflows/local should be moved to a SUBWORKFLOW_NAME/main.nf structure
  • local_component_structure - longread_adapterremoval.nf in subworkflows/local should be moved to a SUBWORKFLOW_NAME/main.nf structure
  • local_component_structure - standardisation_profiles.nf in subworkflows/local should be moved to a SUBWORKFLOW_NAME/main.nf structure
  • local_component_structure - shortread_preprocessing.nf in subworkflows/local should be moved to a SUBWORKFLOW_NAME/main.nf structure
  • local_component_structure - shortread_adapterremoval.nf in subworkflows/local should be moved to a SUBWORKFLOW_NAME/main.nf structure
  • local_component_structure - longread_filtering.nf in subworkflows/local should be moved to a SUBWORKFLOW_NAME/main.nf structure
  • local_component_structure - nonpareil.nf in subworkflows/local should be moved to a SUBWORKFLOW_NAME/main.nf structure
  • local_component_structure - shortread_hostremoval.nf in subworkflows/local should be moved to a SUBWORKFLOW_NAME/main.nf structure

✅ Tests passed:

Run details

  • nf-core/tools version 3.5.1
  • Run at 2026-01-28 14:21:42

@nf-core-bot
Copy link
Member

nf-core-bot commented Oct 16, 2025 •
edited
Loading

Warning

Newer version of the nf-core template is available.

Your pipeline is using an old version of the nf-core template: 3.4.1.
Please update your pipeline to the latest version.

For more documentation on how to update your pipeline, please see the nf-core documentation and Synchronisation documentation.

@LilyAnderssonLee
Copy link
Contributor

LilyAnderssonLee commented Oct 23, 2025

Should we also add nf-test to other test configs?

@LilyAnderssonLee LilyAnderssonLee marked this pull request as ready for review October 23, 2025 12:57
@sofstam
Copy link
Collaborator Author

sofstam commented Oct 23, 2025

Should we also add nf-test to other test configs?

Yes! On my to-do list, just started with the test config first :)

@LilyAnderssonLee
Copy link
Contributor

LilyAnderssonLee commented Oct 23, 2025

So will you add all those nf-test to this PR?

@sofstam
Copy link
Collaborator Author

sofstam commented Oct 23, 2025

I was thinking one PR for each config.

@jfy133
Copy link
Member

jfy133 commented Jan 15, 2026 •
edited
Loading

For future reference, how I'm reviewing:

  1. Run the relevant test profile
  2. View the --outdir with tree in one screen, and on the other screen open the new snapshot
  3. Scroll through the tree, and find the relevant directory in the snapshot (can use<dirname>": when searching on webpage of snapshot to jump to the right location)
  4. Make sure all results/ directories are present by a separate snapshot
  5. Make sure all files are represented by some form of check (exists, string, md5 etc)
  6. When looking at the files, make sure the assertion type makes sense
  7. Make sure the assertion result is correct (e.g. string == true, no empty md5sum string)
  8. Look back at the .nf.test file to make to sure the structure makes sense (e.g. string checks makes sense, that a file cannot be improved - e.g. better string, use a plugin etc)

jfy133
jfy133 reviewed Jan 15, 2026
View reviewed changes
Copy link
Member

@jfy133 jfy133 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary:

Eerything captured now @sofstam and this is MUCH better to look at now, only a couple of errors, and otherwise the final improvement is applying the 'xyz contains string:' to single assertionsin addition to the getAllFilesFromDir - this will makes it MUCH easier to identify which files to investigate if something breaks...

Otherwise overview summary (but mostly just the sentence prefixes)

  • CENTRIFUGE: no combined report? (or missing sentence prefix in assrtions -> there are two 'trues')
  • DIAMOND: tsvs missing sentence prefix (side question: why no combined report for this?)
  • FASTQC: correct this is entirely excluded from assertions (just list file names?)
  • GANON: _report.tre strings equal false! No combined report (or is this the true and missing sentenceprefix?)
  • KAIJU: no sentence prefixes (but correct number of etnries)
  • KMCP: shouldn't these be md5sums?
  • KRAKEN2: no sentence prefixes (but correct number of entries)
  • METAPHLAN: two sets of .biom contains? I think the second should be '_profile.txt` ?
  • NONPAREIL: missing sentence prefixes; the list of files has metaphlan files listed though?
  • SAMTOOLS_STATS: shouldn't these be md5sums?
  • TAXPASTA: missing sentence suffix (but correct number of files)

jfy133
jfy133 reviewed Jan 22, 2026
View reviewed changes
Copy link
Member

@jfy133 jfy133 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Final refinements!

tests/default.nf.test
file("$outputDir/nonpareil/nonpareil_all_samples.tsv").getName() + " contains string:'diversity':" + file("$outputDir/nonpareil/nonpareil_all_samples.tsv").text.contains("diversity"),
stable_content_nonpareil).match("nonpareil") },
{ assert snapshot (file("$outputDir/porechop_abi/ERR3201952_ERR3201952_porechop_abi.log").text.contains("Looking for known adapter"),
stable_content_porechopabi).match("porechop_abi")},
Copy link
Member

@jfy133 jfy133 Jan 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still appears to be a string not an md5sum in the snapshot?

taxprofiler/tests/default.nf.test.snap

Line 729 in 12c9d2b

"ERR3201952_ERR3201952_porechop_abi.log"

Copy link
Collaborator Author

@sofstam sofstam Jan 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we check for the contents of the file? This is an unstable file.

jfy133
jfy133 reviewed Jan 22, 2026
View reviewed changes
sofstam and others added 2 commits January 22, 2026 10:34
@sofstam @jfy133
Update tests/default.nf.test
f6d0ae6 
Co-authored-by: James A. Fellows Yates <[email protected]>
@sofstam @jfy133
Update tests/default.nf.test
54d28d0 
Co-authored-by: James A. Fellows Yates <[email protected]>
@sofstam
Copy link
Collaborator Author

sofstam commented Jan 28, 2026

@jfy133 FASTQC: correct this is entirely excluded from assertions (just list file names?)
What do you mean here? We exclude all fastqc files in .nftignore file. Is there anything else we need to look at?

sofstam and others added 5 commits January 28, 2026 14:45
@sofstam @jfy133
Update tests/default.nf.test
8f95224 
Co-authored-by: James A. Fellows Yates <[email protected]>
@sofstam @jfy133
Update tests/default.nf.test
d4b264a 
Co-authored-by: James A. Fellows Yates <[email protected]>
@sofstam @jfy133
Update tests/default.nf.test
408fd88 
Co-authored-by: James A. Fellows Yates <[email protected]>
@sofstam @jfy133
Update tests/default.nf.test
1035f01 
Co-authored-by: James A. Fellows Yates <[email protected]>
@sofstam @jfy133
Update tests/default.nf.test
1bbc577 
Co-authored-by: James A. Fellows Yates <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jfy133 jfy133 jfy133 left review comments

@rannick rannick rannick approved these changes

@LilyAnderssonLee LilyAnderssonLee Awaiting requested review from LilyAnderssonLee LilyAnderssonLee is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@sofstam @nf-core-bot @LilyAnderssonLee @jfy133 @rannick