Skip to content

Draft: Nftest migration metaphlan3 #8466

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

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

Conversation

nsang0u
Copy link

@nsang0u nsang0u commented May 14, 2025

Closes #7621

Description of changes:
This PR migrates unit tests for metaphlan3/metaphlan3 from pytest to nf-test, and makes some minor corrections to these tests, described below.

  • Instead of checking full file hash of *_profile.txt output file, we skip the first two lines and compare the subsequent text content of the file. Reasoning: second header line contains command used to produce the file, preventing the ability to reproducably snapshot the file in different environments.
  • Omitted previous test conditions which checked the hashes of metaphlan database files. Reasoning: these are static input files and do not need to be checked for integrity in this unit test.
  • Changed references to the path of test input files to use params.modules_testdata_base_path
  • Omitted previous test case which called metaphlan3 without a database input. Current implementation of metaphlan3 process appears to fail in absence of a database input.
  • Updated config for test case with SAM input to have samtools job correctly produce SAM output as opposed to BAM

@nsang0u nsang0u changed the title Draft: Nftest migration metaphlan3 Nftest migration metaphlan3 May 14, 2025
@nsang0u nsang0u assigned nsang0u and maxulysse and unassigned nsang0u May 14, 2025
@nsang0u nsang0u changed the title Nftest migration metaphlan3 Draft: Nftest migration metaphlan3 May 14, 2025
@nsang0u nsang0u marked this pull request as ready for review May 14, 2025 21:01
Copy link
Contributor

@famosab famosab left a comment

Choose a reason for hiding this comment

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

Looks good already :) While your on this - can you also add stub tests?

Copy link
Contributor

Choose a reason for hiding this comment

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

We do not need this here as we have this defined globally :)

Comment on lines +14 to +26
setup {
run("UNTAR") {
script "../../../untar/main.nf"
process {
"""
input[0] = [
[ id:'test' ],
file(params.modules_testdata_base_path + 'delete_me/metaphlan_database.tar.gz', checkIfExists: true)
]
"""
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

You can move the setup block out of the actual test to save some runs :)

{ assert path(process.out.biom[0][1]).text.contains('"format": "Biological Observation Matrix 1.0.0","format_url": "http://biom-format.org","generated_by"') },
{ assert snapshot(
process.out.bt2out,
path(process.out.profile.get(0).get(1)).readLines()[2..-1]
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
path(process.out.profile.get(0).get(1)).readLines()[2..-1]
path(process.out.profile.get(0).get(1)).readLines()[2..-1].join('\n').md5()

{ assert path(process.out.biom[0][1]).text.contains('"format": "Biological Observation Matrix 1.0.0","format_url": "http://biom-format.org","generated_by"') },
{ assert snapshot(
process.out.bt2out,
path(process.out.profile.get(0).get(1)).readLines()[2..-1]
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
path(process.out.profile.get(0).get(1)).readLines()[2..-1]
path(process.out.profile.get(0).get(1)).readLines()[2..-1].join('\n').md5()

Comment on lines +95 to +106
setup {
run("UNTAR") {
script "../../../untar/main.nf"
process {
"""
input[0] = [
[ id:'test' ],
file(params.modules_testdata_base_path + 'delete_me/metaphlan_database.tar.gz', checkIfExists: true)
]
"""
}
}
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
setup {
run("UNTAR") {
script "../../../untar/main.nf"
process {
"""
input[0] = [
[ id:'test' ],
file(params.modules_testdata_base_path + 'delete_me/metaphlan_database.tar.gz', checkIfExists: true)
]
"""
}
}

Comment on lines +149 to +157
// [
// [
// [
// [id:test, single_end:false],
// /Users/nnsangou/projects/nf-core/modules/.nf-test/tests/1b0b84df5ba4ed86ad76e5e202370d46/work/01/56a87b6f66c6ae
// bfee78b7301d0d0c/test_profile.txt
// ]
// ]
// ]
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
// [
// [
// [
// [id:test, single_end:false],
// /Users/nnsangou/projects/nf-core/modules/.nf-test/tests/1b0b84df5ba4ed86ad76e5e202370d46/work/01/56a87b6f66c6ae
// bfee78b7301d0d0c/test_profile.txt
// ]
// ]
// ]

Comment on lines +160 to +172
setup {
run("UNTAR") {
script "../../../untar/main.nf"
process {
"""
input[0] = [
[ id:'test' ],
file(params.modules_testdata_base_path + 'delete_me/metaphlan_database.tar.gz', checkIfExists: true)
]
"""
}
}
}
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
setup {
run("UNTAR") {
script "../../../untar/main.nf"
process {
"""
input[0] = [
[ id:'test' ],
file(params.modules_testdata_base_path + 'delete_me/metaphlan_database.tar.gz', checkIfExists: true)
]
"""
}
}
}

{ assert path(process.out.biom[0][1]).text.contains('"format": "Biological Observation Matrix 1.0.0","format_url": "http://biom-format.org","generated_by"') },
{ assert snapshot(
process.out.bt2out,
path(process.out.profile.get(0).get(1)).readLines()[2..-1]
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
path(process.out.profile.get(0).get(1)).readLines()[2..-1]
path(process.out.profile.get(0).get(1)).readLines()[2..-1].join('\n').md5()

Comment on lines +3 to +4
// publishDir = { "${params.outdir}/${task.process.tokenize(':')[-1].tokenize('_')[0].toLowerCase()}" }

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
// publishDir = { "${params.outdir}/${task.process.tokenize(':')[-1].tokenize('_')[0].toLowerCase()}" }

Comment on lines +5 to +12
withName: METAPHLAN3_METAPHLAN3 {
ext.args = '--index mpa_v30_CHOCOPhlAn_201901 --add_viruses --bt2_ps very-sensitive-local'
}

withName: SAMTOOLS_VIEW {
ext.args = "--output-fmt sam"
}

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 move these to the nf-test file itself? :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

nf-test migration: metaphlan3/metaphlan3
3 participants