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

stub and nf-test created for raxmlng #8161

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

bhargava-morampalli
Copy link
Contributor

@bhargava-morampalli bhargava-morampalli commented Mar 29, 2025

PR checklist

Closes #7676

  • 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 module conventions in the contribution docs
  • If necessary, include test data in your PR.
  • Remove all TODO statements.
  • Emit the versions.yml file.
  • Follow the naming conventions.
  • Follow the parameters requirements.
  • Follow the input/output options guidelines.
  • Add a resource label
  • Use BioConda and BioContainers if possible to fulfil software requirements.
  • Ensure that the test works with either Docker / Singularity. Conda CI tests can be quite flaky:
    • For modules:
      • nf-core modules test <MODULE> --profile docker
      • nf-core modules test <MODULE> --profile singularity
      • nf-core modules test <MODULE> --profile conda
    • For subworkflows:
      • nf-core subworkflows test <SUBWORKFLOW> --profile docker
      • nf-core subworkflows test <SUBWORKFLOW> --profile singularity
      • nf-core subworkflows test <SUBWORKFLOW> --profile conda

Copy link
Contributor

@SPPearce SPPearce left a comment

Choose a reason for hiding this comment

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

I know you are just swapping to nf-test, but think this module could do with some improvements.

I think this module should have a meta map. Then we can use meta.id as the default prefix, as normal.
Is the --model a required argument? As then it should be an input channel; please don't define extra task.ext.foo values.
Can you make the optional file in the stub (bonus points if you make it conditional on something being passed to args, if that is how it decides to make it.

@github-project-automation github-project-automation bot moved this from Todo to In Progress in nf-test Migration Mar 30, 2025
@SPPearce
Copy link
Contributor

If you are not comfortable adjusting the module, I can do that for you.

@SPPearce
Copy link
Contributor

Oh, and don't forget to remove the pytest files and the entry from the pytest config file.

@SPPearce
Copy link
Contributor

And while you are there, you can remove the pytest files and entry for roary.

@bhargava-morampalli bhargava-morampalli requested a review from a team as a code owner April 1, 2025 21:22
@bhargava-morampalli
Copy link
Contributor Author

@SPPearce
I have done the changes you suggested. Can you please take a look at the changes when you have a chance?

  1. I wanted to modify the module myself - so made changes as you suggested
  2. I deleted the entries in pytest config file and deleted the pytest files as well

Copy link
Contributor

@SPPearce SPPearce left a comment

Choose a reason for hiding this comment

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

You can use a modules_args parameter to pass the bootstrapping option to the args, to test that directly rather than adding a new extra parameter.
See https://github.com/nf-core/modules/blob/c9c3ef86c1892413b3c86fb38c4e39fd7288512f/modules/nf-core/ataqv/ataqv/tests/main.nf.test for instance.

Comment on lines +13 to +15
tuple val(meta), path("*.raxml.bestTree") , emit: phylogeny
tuple val(meta), path("*.raxml.support"), optional:true, emit: phylogeny_bootstrapped
path "versions.yml" , emit: versions
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you put the emit: first, then the optional:, so it lines up better.

Comment on lines +37 to +38
type: map
description: Groovy Map containing sample information
Copy link
Contributor

Choose a reason for hiding this comment

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

This should still be the previous description and type:

- "*.raxml.support":
type: file
description: A phylogeny in Newick format with bootstrap values
type: map
Copy link
Contributor

Choose a reason for hiding this comment

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

This should still be the previous description and type


cat <<-END_VERSIONS > versions.yml
"${task.process}":
raxmlng: \$(echo \$(raxml-ng --version 2>&1) | sed 's/^.*RAxML-NG v. //; s/released.*\$//')
END_VERSIONS
"""

stub:
def args = task.ext.args ?: params.raxmlng_args ?: ''
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def args = task.ext.args ?: params.raxmlng_args ?: ''
def args = task.ext.args ?: ''

Comment on lines +41 to +44
if (!meta.id) {
error "Input meta map does not contain 'id'. Received: ${meta}"
}
// Use a dedicated param for stub testing the bootstrap output scenario
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (!meta.id) {
error "Input meta map does not contain 'id'. Received: ${meta}"
}
// Use a dedicated param for stub testing the bootstrap output scenario


stub:
def args = task.ext.args ?: params.raxmlng_args ?: ''
def prefix = task.ext.prefix ?: meta.id
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def prefix = task.ext.prefix ?: meta.id
def prefix = task.ext.prefix ?: "${meta.id}"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

nf-test migration: raxmlng
2 participants