Nf-test for merged nf-core template v3.3.1 #246
Conversation
|
Alright, now the failing tests have meaningful output:
|
|
Thanks for the hint! I already added the line |
Check out the tests that are running here automatically, e.g. |
|
Al 10000 most there, only 2 md5sums left that differ, looks good! |
|
I saw in the dev ampliseq branch that you created many snapshots for different test profiles. Is it necessary for bacass as well? |
|
Hi, ultimately it would be good to add one .nf.test and .nf.test.snap file pair per test config. However, I think its fine to split template update + one test and the remaining tests into separate PRs. So I'd say this PR should be merged now (I'll review, and @Daniel-VM as an active maintainer should maybe also have the chance to look at it) and a separate PR cn then contain the remaining tests. |
There was a problem hiding this comment.
Template update and test look good to me.
I am however concerned about the addition of the parameter discard_trimmed_pass because it appear to me it should never be true anyway. @Daniel-VM what do you think?
| false, // we don't use discard_trimmed_pass at the moment | ||
| val_discard_trimmed_pass, |
There was a problem hiding this comment.
Looking at the descripton of that param discard_trimmed_pass in subworkflows/nf-core/fastq_trim_fastp_fastqc/meta.yml, I'd say this param may never be true in this pipeline. Therefore I am not sure this change here is appropriate. Maybe this should have been kept here as false in any case?
There was a problem hiding this comment.
You are right, this param should be set to "false"
There was a problem hiding this comment.
But I have already set them as false in nextflow.config
There was a problem hiding this comment.
Yes, so the pipeline works as expected with this default setting. However, if anyone for any reason sets this to true, then the pipeline will fail, as far as I get it. Did you test that?
If the pipeline indeed cannot handle when --discard_trimmed_pass is used, as we suspect, then it makes no sense to allow this option.
There was a problem hiding this comment.
I tested it with following command
nextflow run path/to/bacass/main.nf -profile test,singularity --discard_trimmed_pass true --outdir results
it worked well.
As I understood, without this option it will be difficult to pass nf-core linting, as it requested from me the updated fastq_trim_fastp_fastqc subworkflow. What I can propose is to hide this option with "hidden": true in nextflow_schema.json
There was a problem hiding this comment.
Ok, that is unexpected to me. Then I would prefer a proper description what the parameter does in the nextflow_schema.json, if thats unclear, then hide it.
|
Hii!! Looks good.
Yes, totally agree. The remaining test can go in a separate PR. |
|
Thanks for the amazing work, @brovolia . Could you go ahead and merge this PR (green button "merge pull request")? Many thanks |
I resolved merging conflicts and tested the updated pipeline. The test completed successfully on our HPC cluster with Singularity support.