Skip to content
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

Faulty test command generated by create-test-yaml #1562

Closed
matthdsm opened this issue May 11, 2022 · 8 comments
Closed

Faulty test command generated by create-test-yaml #1562

matthdsm opened this issue May 11, 2022 · 8 comments
Assignees
Labels
bug Something isn't working
Milestone

Comments

@matthdsm
Copy link
Contributor

Description of the bug

@drpatelh
First mentioned in nf-core/modules#1645 (comment)

generates:

command: nextflow run tests/modules/bowtie2/align -entry test_bowtie2_align_single_end -c tests/config/nextflow.config

should be:

 command: nextflow run ./tests/modules/bowtie2/align -entry test_bowtie2_align_single_end -c ./tests/config/nextflow.config -c ./tests/modules/bowtie2/align/nextflow.config

Command used and terminal output

nf-core modules create-test-yml -f -p bowtie2/align

System information

nf-core v2.3.2

@matthdsm matthdsm added the bug Something isn't working label May 11, 2022
@drpatelh
Copy link
Member

drpatelh commented May 11, 2022

Appears like we need to update this line to reflect what we have in the module:

f"nextflow run tests/modules/{self.module_name} -entry {entry_point} -c tests/config/nextflow.config"

Not sure whether this happens when you have single/multiple tests to run.

@drpatelh drpatelh added this to the 2.4 milestone May 11, 2022
@ewels
Copy link
Member

ewels commented May 12, 2022

- command: nextflow run tests/modules/bowtie2/align -entry test_bowtie2_align_single_end -c tests/config/nextflow.config
+ command: nextflow run ./tests/modules/bowtie2/align -entry test_bowtie2_align_single_end -c ./tests/config/nextflow.config -c ./tests/modules/bowtie2/align/nextflow.config

ok so we're missing a ./ (that bit shouldn't matter, right?) and missing a module-specific config file, gotcha 👍🏻

Not sure whether this happens when you have single/multiple tests to run.

I think you have to customise it with the CLI interactive prompt instead of using the default value in the code. Not sure what else we can do here.

@ewels ewels self-assigned this May 12, 2022
@ewels
Copy link
Member

ewels commented May 12, 2022

-c ./tests/modules/bowtie2/align/nextflow.config - I don't think that this is needed. The workflow is in ./tests/modules/bowtie2/align so the nextflow.config in the same folder should be loaded automatically by Nextflow.

@matthdsm
Copy link
Contributor Author

Actually, I've never had any issue. Like you said, the config is loaded automatically.
@drpatelh, what's the issue here exactly?

@ewels
Copy link
Member

ewels commented May 12, 2022

I think that the main reason was that the module-template should match the default command. I've put a draft PR together in #1568 where I (a) add the ./ prefix for clarity and to ensure it's local and (b) remove the unnecessary config load from the module template.

Not sure if this change will mean widespread changes are needed across nf-core/modules? 🤔 I think it's best to keep the commands as short as possible though.

@drpatelh
Copy link
Member

drpatelh commented May 12, 2022

There were issues but I can't remember now. Either way it would be good to be consistent with the syntax and for it to match the module template in tools because it makes it easier to update 100's of modules if everything looks the same. Had to do this before and it's not nice having to manually update things for consistency.

Maybe let's keep it as it is for now and we can deal with it later if required all in one go.

@ewels
Copy link
Member

ewels commented May 12, 2022

Maybe let's keep it as it is for now and we can deal with it later if required all in one go.

You mean keep the explicit -c in for now? Fine by me, shouldn't cause any issues and if removing it means manually updating tonnes of code then I see the attraction of keeping it 😅

Did we ever discuss a nf-core modules sync? 🤔

ewels added a commit to ewels/nf-core-tools that referenced this issue May 12, 2022
Avoid needing to change loads of module files due to the template update.
See nf-core#1562 (comment)
@ewels
Copy link
Member

ewels commented May 12, 2022

Added back in d61a51d

@ewels ewels closed this as completed May 12, 2022
drpatelh added a commit to nf-core/modules that referenced this issue Oct 10, 2022
* test(align_bowtie2): Add tags

* test(bam_sort_samtools): Add tags

* test(homer_groseq): Add tags

* test(subworkflows): Update tags for annotation/ensemblevep

* test(subworkflows): Add tags to snpeff

* test(subworkflows): Add bam_qc_picard tags

* style(subworkflows): Sort by name

* test(subworkflows): Add tags to fgbio_create_umi_consensus

* test(subworkflows): Add tags for gatk_create_som_pon

* test(subworkflows): Add tags for gatk_tumor_normal_somatic_variant_calling

* test(subworkflows): Add tags for gatk_tumor_only_somatic_variant_calling

* test(subworkflows): Add tags for srafastq

* test(subworkflows): Remove subworkflow specific config

nf-core/tools#1562

Co-authored-by: Harshil Patel <drpatelh@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants