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

Pytest requirements #1620

Merged
merged 5 commits into from
Jun 2, 2022
Merged

Pytest requirements #1620

merged 5 commits into from
Jun 2, 2022

Conversation

fabianegli
Copy link
Contributor

@fabianegli fabianegli commented Jun 1, 2022

nf-core has specific pytest and pytest-workflow version dependencies. I added them to the requirements.txt file.

The pytest dependency has been moved from the dev to the standard requirements file because it is a requirement since the introduction of the nf-core modules test command.

Surfaced in nf-core Slack

PR checklist

  • This comment contains a description of changes (with reason)
  • CHANGELOG.md is updated
  • If you've fixed a bug or added code that should be tested, add tests!
  • Documentation in docs is updated

@codecov
Copy link

codecov bot commented Jun 1, 2022

Codecov Report

Merging #1620 (2117653) into dev (dc4e8f1) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##              dev    #1620   +/-   ##
=======================================
  Coverage   64.71%   64.71%           
=======================================
  Files          54       54           
  Lines        6235     6235           
=======================================
  Hits         4035     4035           
  Misses       2200     2200           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dc4e8f1...2117653. Read the comment docs.

@jfy133 jfy133 requested review from ewels and mirpedrol June 2, 2022 07:41
@jfy133
Copy link
Member

jfy133 commented Jun 2, 2022

Would this require a separate bioconda update? Or is the recipe updated automated from requirements.txt?

Copy link
Member

@ewels ewels left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

yes this also needs to be done separately in bioconda. Can edit the auto-generated PR that happens after release on PyPI or can update the recipe right away whilst incrementing the build number.

I think the latter approach would be best here 👍🏻

@ewels ewels merged commit f476a07 into nf-core:dev Jun 2, 2022
@fabianegli fabianegli deleted the pytest-requirements branch June 2, 2022 08:56
@fabianegli
Copy link
Contributor Author

My pleasure 😄

@ewels
Copy link
Member

ewels commented Jun 3, 2022

@fabianegli did you do the Bioconda PR as well?

@fabianegli
Copy link
Contributor Author

No, I haven't. Which repo? I didn't find nf-core in the recipies in the bioconda repo.

@ewels
Copy link
Member

ewels commented Jun 6, 2022

Here: https://github.com/bioconda/bioconda-recipes/tree/master/recipes/nf-core

Need to bump the build number to 1 and then add to the requirements list.

The docs say you should test locally and stuff but I wouldn't bother personally. Just fork, edit and push a PR and see if the tests pass.. :)

@fabianegli
Copy link
Contributor Author

@ewels
Copy link
Member

ewels commented Jun 6, 2022

Nice! And because you made the PR, I was able to merge it 😄

@fabianegli
Copy link
Contributor Author

In bioconda, something seems to be off, though. The CI for the nightly fails: https://github.com/bioconda/bioconda-recipes/runs/6766508609

Someone reported the issue: bioconda/bioconda-recipes#35015

Not sure if this is something that needs our attention, but I think it means the new recipe has not been published, yet? At least when I installed nf-core from bioconda just now nf-core build 0 was installed.

@ewels
Copy link
Member

ewels commented Jun 12, 2022

Hmm, not ideal. Maybe worth posting in the Bioconda Gitter chat..

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

Successfully merging this pull request may close these issues.

3 participants