Skip to content

Conversation

bhargava-morampalli
Copy link
Contributor

@bhargava-morampalli bhargava-morampalli commented May 18, 2025

PR checklist

Closes

  • 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.

Several of your tests in the snapshot seem to be outputting the whole test work directory, which is then failing due to not being consistent.
I'm not sure why it is doing that though.
We often see similar behaviour when a file is created that ends in .gz but isn't actually gzipped (often empty files that are present in a stub), but I don't think that is happening here.
You also need to remove the pytest files and swap e.g. file(params.test_data['sarscov2']['genome']['genome_fasta'],checkIfExists:true) to be the newer params.modules_testdata_path` structure.

@github-project-automation github-project-automation bot moved this from Todo to In Progress in nf-test Migration May 20, 2025
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 can't see any obvious reason why nf-test would behave differently for the two bwa tools to snapaligner and bowtie.
I would suggest trying altering the output folder name/structure in one of the two bwa modules and see if that makes any difference (e.g. try tuple val(meta), path("bwa/") , emit: index, or tuple val(meta), path("foo") , emit: index, with changing the code itself to use foo).

@SPPearce
Copy link
Contributor

I think I've figured this out, just need to finish it off

@SPPearce
Copy link
Contributor

I've updated this subworkflow, tests should hopefully now pass.
However, it shouldn't be merged until a decision is made about what to do with the alt_liftover file, currently the bwa/mem modules do not output this into the bwa/ folder as is required for downstream use!
i.e. the channel structure is now wrong.

@SPPearce SPPearce changed the title nf-test created for Fastaindexdna DO NOT MERGE update fasta_index_dna May 29, 2025
@LouisLeNezet
Copy link
Contributor

@SPPearce as per: https://nfcore.slack.com/archives/C043UU89KKQ/p1748441535247279
We can add an extra input to bwa mem to use the alt file as a separate channel as you suggested.
The pytest files can also be definitely removed !

@SPPearce SPPearce requested a review from a team as a code owner August 29, 2025 14:38
@SPPearce SPPearce requested a review from a team as a code owner August 29, 2025 14:38
@SPPearce
Copy link
Contributor

Replaced by #8962

@SPPearce SPPearce closed this Aug 29, 2025
@github-project-automation github-project-automation bot moved this from In Progress to Done in nf-test Migration Aug 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants