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

Automating pytest migration #3138

Draft
wants to merge 12 commits into
base: dev
Choose a base branch
from
Draft

Automating pytest migration #3138

wants to merge 12 commits into from

Conversation

GallVp
Copy link
Member

@GallVp GallVp commented Aug 23, 2024

Hi Team

As suggested by @mashehu , I am opening this draft PR.

  • Here I have tried to automate pytest to nf-test conversion as much as possible. Currently it only works for pytests for modules with a single include statement, as chaining is not needed.
  • If we are planning to deprecate pytest, I don't think we should merge this as it will add unnecessary code. There are too many edge cases. I am happy to create conversion PRs on modules and take care of the edge cases along the way.
  • One thing that we might want to take from here are the semi-automated power assertions with nf-core modules lint --update-assertions or such. Lots of modules fail nf-test after their creation date because md5s are added for gz files or log files. If we lint for power assertions and automate some of them such as nft-bam or nft-vcf, that will encourage good practice across modules and save people from downstream pain.

Comment on lines +198 to +200
if migrate_pytest_hard and not migrate_pytest:
log.error("--migrate_pytest_hard can only allowed in combination with --migrate_pytest.")
sys.exit(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just add flag --hard to the existing command? Or indeed --force which is more consistent with the rest of nf-core tools.

e.g. --migrate_pytest --force instead of --migrate_pytest --migrate_pytest_hard

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you @adamrtalbot

I like your idea. I have also thought of not adding any additional flag at all. The code should try to extract and convert the pytest workflows by default, but if an exception is thrown, it should catch that, and revert back to the original migration scheme. That way, we also don't need to update the documentation.

However, the problem with this PR and the functionality it implements is that it is very fragile. I am using regex which cannot cover many edge cases. A more robust approach is to use a language parser such as pyPEG (https://stackoverflow.com/questions/6571964/which-tool-to-use-to-parse-programming-languages-in-python). But then we have plans to deprecate pytest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

2 participants