Replace tags strategy with implicit 'tags in test' strategy #253
Conversation
Changes: - All modules, subworkflows and workflows include their own name as a tag - All subworkflows and workflows include their dependencies as a tag - By detecting tags with `nf-test list --tags` we can extract all tags - Should be able to flag changed modules and dependent workflows without using a tags.yml file
|
subworkflows/nf-core/fastq_download_prefetch_fasterqdump_sratools/tests/main.nf.test
Outdated
Show resolved
Hide resolved
| tag "UNTAR" | ||
| tag "SRATOOLS_FASTERQDUMP" |
There was a problem hiding this comment.
I'll put the thing we're actually testing first, and then a break line, and then the dependency
There was a problem hiding this comment.
Done now. The format is:
thing "NAME"
tag "NAME"
tag "DEPENDENCY"
| tag "MULTIQC_MAPPINGS_CONFIG" | ||
| tag "SRA_FASTQ_FTP" | ||
| tag "SRA_IDS_TO_RUNINFO" | ||
| tag "SRA_RUNINFO_TO_FTP" | ||
| tag "SRA_TO_SAMPLESHEET" |
There was a problem hiding this comment.
Hmmm...we have to be explicit with all of the workflow dependencies here now?
There was a problem hiding this comment.
Could be quite easy to lose track of these as you update the pipeline / workflow....
There was a problem hiding this comment.
Yes we do. Not ideal, but hard to do automatically. See askimed/nf-test#179 for relevant ticket.
There was a problem hiding this comment.
I had exactly the same thought as Harshil, don't think people will be very good at keeping these lists up to date. But I see the difficulties.
There was a problem hiding this comment.
No magic bullet so far. I've proposed automatically testing dependencies, but I also think nf-test can only go so far and it might just have to be the resposibility of the pipeline developer.
|
Awesome work @adamrtalbot ! 🤩 Few minor niggles I think we should discuss tomorrow. |
This reverts commit b1b117d.
| files_yaml: | | ||
| ".": | ||
| - .github/workflows/** | ||
| - nf-test.config | ||
| - nextflow.config | ||
| tests: | ||
| - assets/* | ||
| - bin/* | ||
| - conf/* | ||
| - main.nf | ||
| - nextflow_schema.json |
There was a problem hiding this comment.
@maxulysse shall we put this in a separate file?
There was a problem hiding this comment.
I'm sorry I don't understand what you meant by that.
There was a problem hiding this comment.
These are supplementary existing tags. We can move this to a separate YAML, e.g. .github/test_paths.yml like we have tags now.
There was a problem hiding this comment.
Further discussion - general consensus was it's all in one place if we leave it here. We can move it out later if we need to.
|
See https://github.com/adamrtalbot/fetchngs/pulls?q=is%3Aopen+is%3Apr+label%3Atags for example PRs including these changes. Feel free to suggest or create more! |
|
@pinin4fjords could you take a look with fresh eyes? I can talk you through it but I'd like someone coming in blind. |
adamrtalbot
changed the title
Replace Nextflow versioning with simple list matrix
tests/main.nf.test
Outdated
| tag "pipeline_fetchngs" | ||
| tag "PIPELINE" | ||
|
|
||
| tag "SRA" |
There was a problem hiding this comment.
It's the immediate dependency of the pipeline. Perhaps we just run pipeline level tests at this point?
There was a problem hiding this comment.
OK I've removed it now. It will not test the workflow tests when we launch pipeline level tests anymore.
We can put it back if that behaviour feels weird.
workflows/sra/tests/main.nf.test
Outdated
| tag "sra_default_parameters" | ||
|
|
||
|
|
||
| // Subworkflows |
There was a problem hiding this comment.
| // Subworkflows | |
| // Workflow |
This one is a "workflow"
workflows/sra/tests/main.nf.test
Outdated
| // Subworkflows | ||
| tag "SRA" | ||
|
|
||
| // Modules |
There was a problem hiding this comment.
| // Modules | |
| // Dependencies |
I'd rather we be explicit there and say dependencies, as any at this level "workflow" can contain modules and/or subworkflows
| tag "modules_local" | ||
| tag "multiqc_mappings_config" | ||
|
|
||
| tag "MULTIQC_MAPPINGS_CONFIG" |
There was a problem hiding this comment.
Not for this PR- but if tag is becoming redundant with the process maybe we should talk to the nf-test folks about allowing nf-test to list processes/ workflows rather than tags?
There was a problem hiding this comment.
I think it's similar to what @adamrtalbot meant when creating this issue: askimed/nf-test#178
There was a problem hiding this comment.
Correct, I've suggested it as a feature. This was the behaviour in pytest-workflow as well.
There was a problem hiding this comment.
Gotcha, sorry for the slow uptake
| tag "MULTIQC_MAPPINGS_CONFIG" | ||
| tag "SRA_FASTQ_FTP" | ||
| tag "SRA_IDS_TO_RUNINFO" | ||
| tag "SRA_RUNINFO_TO_FTP" | ||
| tag "SRA_TO_SAMPLESHEET" |
There was a problem hiding this comment.
I had exactly the same thought as Harshil, don't think people will be very good at keeping these lists up to date. But I see the difficulties.
| tag "modules_nfcore" | ||
| tag "custom" | ||
| tag "custom/sratoolsncbisettings" | ||
|
|
There was a problem hiding this comment.
Changes in nf-core things will be patches, right?
There was a problem hiding this comment.
Yep. Will need a patch.
| tag 'subworkflows' | ||
| tag 'utils_nfcore_fetchngs_pipeline' | ||
| tag 'subworkflows/utils_nfcore_fetchngs_pipeline' | ||
| tag "UTILS_NFCORE_FETCHNGS_PIPELINE" |
There was a problem hiding this comment.
@maxulysse is this the right tag? Too late on a Friday to confirm. Please check on Monday 😆
Using implicit tags
Replacement strategy to replace tags with tags present in nf-test files.
Subject to change, expect the unexpected.
Changes:
nf-test list --tagson any directory with a changed file to find all tags associated with changesfiles_yamlblock of the actiontests/directory withnf-test list --tags.To do:
nf-test list --tagsPR checklist
nf-core lint).nextflow run . -profile test,docker --outdir <OUTDIR>).nextflow run . -profile debug,test,docker --outdir <OUTDIR>).docs/usage.mdis updated.docs/output.mdis updated.CHANGELOG.mdis updated.README.mdis updated (including new tool citations and authors/contributors).