Skip to content

Refactor nf-tests #161

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
Closed
Aratz wants to merge 13 commits into from
Closed

Refactor nf-tests #161

Aratz wants to merge 13 commits into from

Conversation

@Aratz
Copy link
Contributor

@Aratz Aratz commented Nov 5, 2025 •
edited
Loading

Nf-tests take a really long time to complete (ca 40min locally). This is too long to comfortably iterate and develop new features.

This PR does the following:

  • remove all stub tests. As far as I understand, stubs are useful if a process takes too long to be part of a test. Here this seems unnecessary since we run all processes anyways
  • merge all pipeline tests into one. Running the whole pipeline on test data takes time (ca 200s per run). As much as possible we should avoid running the pipeline multiple times and instead test all the parameters we can in one single run.
  • Run the experiment summary in test mode
  • Remove subworkflow tests. In our case the PNA and MPX subworkflows span almost the whole pipeline and there is little benefit into testing them individually

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 necessary, also make a PR on the nf-core/pixelator 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>).
  • CHANGELOG.md is updated.

Aratz added 11 commits November 4, 2025 15:07
@Aratz
Merge all mpx tests
2c798a5
@Aratz
Merge all pna tests
9b03d63
@Aratz
Remove pipeline stub tests
fb38632
@Aratz
Remove all module stub tests
501c3d2 
As far as I understand, stub tests are usefull to mock specific modules
if they take too long. Here however we test both the real module and the
stubbed version, which I think is overkill.
@Aratz
Remove demux multiple chunk test
6f51730 
Testing `--output-chunk-reads` can be done in pixelator instead
@Aratz
Remove pna subworkflow tests
7874ff7 
These tests are almost identical to the pipeline level tests
@Aratz
Remove subworkflow stub
d8b4a0a
@Aratz
Execute ES in test mode for tests
7ec18bc
@Aratz
Add tests for pna layout
1038d8f
@Aratz
Recompute snapshots
866c383
@Aratz
Merge remote-tracking branch 'origin/dev' into fix/refactor_tests
b9c444f
@Aratz Aratz self-assigned this Nov 5, 2025
@github-actions
Copy link

github-actions bot commented Nov 5, 2025 •
edited
Loading

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

Posted for pipeline commit 8cfdddc

+| ✅ 275 tests passed       |+
#| ❔   7 tests were ignored |#
!| ❗   4 tests had warnings |!
Details

❗ Test warnings:

  • files_unchanged - LICENSE does not match the template
  • pipeline_todos - TODO string in main.nf: Optionally add in-text citation tools to this list.
  • pipeline_todos - TODO string in main.nf: Optionally add bibliographic entries to this list.
  • pipeline_todos - TODO string in main.nf: Only uncomment below if logic in toolCitationText/toolBibliographyText has been filled!

❔ Tests ignored:

✅ Tests passed:

Run details

  • nf-core/tools version 3.4.1
  • Run at 2025-11-05 14:56:28

@Aratz Aratz marked this pull request as ready for review November 5, 2025 14:57
@Aratz Aratz requested a review from a team as a code owner November 5, 2025 14:57
johandahlberg
johandahlberg approved these changes Nov 5, 2025
View reviewed changes
Copy link
Collaborator

@johandahlberg johandahlberg left a comment

Choose a reason for hiding this comment

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

This looks good to me. However, I don't know if there are any nf-core standards that require us to have separate stub tests and/or subworkflow tests. Perhaps we should check in a bit on that on slack, so we don't end up having to add them back when we do a release?

@fbdtemme
Copy link
Collaborator

fbdtemme commented Nov 5, 2025 •
edited
Loading

While I fully agree that a speedup is needed I think we should check that we are not removing useful tests.

On the module level we follow the nf-core/module test requirements. These require stub tests to be present. Stubs are usefull for quickly trying out workflow logic without actually running the tools. The module level stub tests are fast anyways, mainly the startup/teardown logic you pay for each test anyways.

I also would not remove subworkflow tests, even thought they indeed largely overlap with the full pipeline tests. Being able to run the subworkflows independently through the subworkflows unit tests is very handy during development.

On the pipeline level there are indeed multiple tests because the output structure looks quite different when using --save_all vs the default. Dropping those tests increases the risk of us missing issues related to that. I think we can merge those together in one test case but that should be paired with a simplification of the publishing logic. I am especially not a fan of the "only publish pxl file from latest stage in the publishDir root behaviour" since the interaction with various skip_<stage> options becomes a pain quickly.

I think a big win would be to specialize the testdata for each module test. Now we use a rather big subset 300k reads that can run through the whole pipeline. By specializating the subset for just that step the processing time can be cut by quite a bit.

I also thing further subsampling of the minimal full run sample further can help a lot here.

Instead of removing tests that are usefull only in selected development scenarios we can also use tags to exclude those from running on each PR.

@fbdtemme
Copy link
Collaborator

fbdtemme commented Nov 5, 2025

To me the main issue is that the extensive snapshots now can require a lot of --update-snapshot runs. By limiting checks to "file exists" checks we can still catch most nextflow logic issues without needing to regenerate that often.

@Aratz
Copy link
Contributor Author

Aratz commented Nov 6, 2025

Wrapping up what we discussed today and what needs to be done

  • Go through each test output and focus on checking the output file exists. Content validation is done with tests on the python side. In most case, it's only the version file that needs to be snapshot
  • Add a smoke-test tag on one pipeline test with good coverage. The goal of this tag is to be able to run a good subset of the tests to get a quick idea if anything broke down after some changes
  • Add a script or task file to quickly run tests locally, if possible in parallel
  • Restore the stub tests for MPX modules, and add stub tests for PNA modules
  • Look into specializing the test data for the module tests to make them faster (demux and analysis take the most time and have the most potential to be sped up, the other tests are not that much slower than their stubbed version so there is not that much to gain there)
  • Check for slow imports in the pixelator cli and see if they can be done lazily

I'm gonna close this PR and make a new one with the changes above, thanks for good discussions today!

@Aratz Aratz closed this Nov 6, 2025
@Aratz Aratz mentioned this pull request Nov 7, 2025
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@johandahlberg johandahlberg johandahlberg approved these changes

Assignees

@Aratz Aratz

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Aratz @fbdtemme @johandahlberg