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

Update hamronization/abricate #4851

Merged
merged 9 commits into from
Feb 7, 2024
Merged

Conversation

jasmezz
Copy link
Contributor

@jasmezz jasmezz commented Feb 5, 2024

  • Update hamronization version
  • Add nf-test + stub sections

PR checklist

  • 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:
    • PROFILE=docker pytest --tag <MODULE> --symlink --keep-workflow-wd --git-aware
    • PROFILE=singularity pytest --tag <MODULE> --symlink --keep-workflow-wd --git-aware
    • PROFILE=conda pytest --tag <MODULE> --symlink --keep-workflow-wd --git-aware

@jasmezz jasmezz requested a review from a team as a code owner February 5, 2024 16:20
@jasmezz jasmezz requested a review from ahvigil February 5, 2024 16:20
Copy link
Contributor

@CarsonJM CarsonJM left a comment

Choose a reason for hiding this comment

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

Just one small question, mainly out of my own curiosity rather than an actual issue

Comment on lines 42 to 44
input[1] = ''
input[2] = ''
input[3] = ''
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't you use the same inputs as the main test? What you have works, I'm just curious why you chose to have them remain different!

Copy link
Member

Choose a reason for hiding this comment

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

I wondered that too.. but then had same thought, 'true not necessarily needed, nm' 😆

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't have a strong opinion about those, but @ahvigil has (see below comment), so I went for same inputs for stub and normal tests now ;)

process {
"""
input[0] = [ [ id:"test" ],file("dummy.tsv") ]
input[1] = ''
Copy link
Contributor

Choose a reason for hiding this comment

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

i think input[1] should still be one of the valid format strings. If you look at the snapshot for this stub both tsv and json outputs are empty, and the stub will end up doing an echo ... > test. because the format is an empty string. All of that works in that nothing complains, but doesn't totally test the stub in the way that its meant to be used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I changed it to the inputs from the normal test to have it as similar as possible. So far, I used stub tests only for modules with intensive ressource demand (e.g. database downloads or 'out of memory' errors on Github CI checks). But yes, makes sense to keep everything else the same between stub and normal test 👍

Comment on lines 46 to 55
echo "hamronize \\
abricate \\
${report} \\
$args \\
--format ${format} \\
--analysis_software_version ${software_version} \\
--reference_database_version ${reference_db_version} \\
> ${prefix}.${format}"

touch ${prefix}.${format}
Copy link
Contributor

Choose a reason for hiding this comment

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

no need to both echo and touch here, I think you can pick one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, removed!

@jasmezz
Copy link
Contributor Author

jasmezz commented Feb 7, 2024

Thanks @jfy133, @CarsonJM and @ahvigil ! 🚀

@jasmezz jasmezz added this pull request to the merge queue Feb 7, 2024
Merged via the queue into nf-core:master with commit be5430a Feb 7, 2024
11 checks passed
@jasmezz jasmezz deleted the update_hamronization branch February 7, 2024 21:41
jch-13 pushed a commit to jch-13/modules that referenced this pull request Mar 19, 2024
* abricate --migrate_pytest

* Complete nf-test + add stub

* Fix linting

* Fix linting

* Update test names

* Update stub test

* Stage proper input files for stub tests

* Remove `echo <command>` from main.nf
jennylsmith pushed a commit to RSC-RP/modules that referenced this pull request Mar 20, 2024
* abricate --migrate_pytest

* Complete nf-test + add stub

* Fix linting

* Fix linting

* Update test names

* Update stub test

* Stage proper input files for stub tests

* Remove `echo <command>` from main.nf
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants