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.

Sign up for GitHub

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

Jump to bottom

Pytest workflow #546

Closed
wants to merge 49 commits into from
Closed

Pytest workflow #546

wants to merge 49 commits into from

Conversation

edmundmiller
Copy link
Contributor

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 pipeline conventions in the contribution docs
  • If necessary, also make a PR on the nf-core/rnaseq branch on the nf-core/test-datasets repository.
  • Make sure your code lints (nf-core lint .).
  • Ensure the test suite passes (nextflow run . -profile test,docker).
  • Usage Documentation in docs/usage.md is updated.
  • Output Documentation in docs/output.md is updated.
  • CHANGELOG.md is updated.
  • README.md is updated (including new tool citations and authors/contributors).

@edmundmiller
Copy link
Contributor Author

edmundmiller commented Jan 8, 2021 •
edited

Alright I've hit a snag. So the tests can't seem to find anyting in bin/ if I use --symlink arg for pytest. Anyone know anything more about how nextflow copies over the bin/ dir?

To reproduce install pytest-workflow and run pytest --tag default --symlink --wt 3 --kwdof

@grst
Copy link
Member

grst commented Jan 8, 2021

afaik it doesn't - it just adds it to PATH. Check the .command.run script, in there should be a line defining PATH.

@edmundmiller
Copy link
Contributor Author

@grst Hmm the Path line is in there, and it looks right. Does PATH not like symlinks?

export PATH="/tmp/pytest_workflow_woixfhep/Run_default_pipeline_with_test_data/bin:\$PATH"

@grst
Copy link
Member

grst commented Jan 8, 2021

I'd assume that something goes wrong with mounting the directory in the Docker container.

@edmundmiller
Copy link
Contributor Author

@pditommaso Could I get your thoughts on why this is happening? I noticed that nextflow-io/nextflow@1405a28 Is in the 5v20.11.0-edge changed the way the bin dir is handled and I'm using that version because of the singularity https fix.

@edmundmiller edmundmiller mentioned this pull request Jan 24, 2021
Closed
@edmundmiller
Copy link
Contributor Author

Created nextflow-io/nextflow#1868

@edmundmiller edmundmiller mentioned this pull request Apr 7, 2021
Open
maxulysse
maxulysse reviewed Apr 27, 2021
View reviewed changes
tests/test_default.yml Outdated Show resolved Hide resolved
@ewels ewels mentioned this pull request May 9, 2021
Closed
@edmundmiller
test(default): Rewrite with pytest-workflow
d8994eb
@edmundmiller
test(star_salmon): Rewrite with pytest-workflow
c9b39ca 
This one starts to get a little clever. Here's some of the highlights:
- All of the tests have the main job name as a tag(`star_salmon`) in
this cause. This will allow for the job to select star_salmon from
`star_rsem`. I'm debating turning these into two separate tags so that
if a change to the star module was made we can run all the tests related
to that.
- The parameter tag then allows for the specific parameter being tested
to be selected for.
- We could just run `--tag star_salmon` by itself but the
matrix.parameters will make the CI jobs run quicker.
- Files and md5 hashes will come later.
- Had to cut off the `--` before each tag because it broke the regex occasionally
@edmundmiller
test: GITHUB_WORKSPACE => main.nf
15e053c 
There's no env variable locally for this
@edmundmiller
test(star_salmon): Use two tags instead of one
115c82d
@edmundmiller
ci: Fix syntax error
67fb0dc
@edmundmiller
test(star_rsem): Rewrite to use pytest-workflow
4da30ab 
See 3bc9078 for details
@edmundmiller
test(hisat2): Rewrite to use pytest-workflow
2c9ed44
@edmundmiller
ci: Fix step names
0c90a56
@edmundmiller
chore: Remove TODOs
b0309d1
@edmundmiller
test(salmon): Rewrite to use pytest-workflow
8ca995a
@edmundmiller
test(sra_download): Rewrite to use pytest-workflow
a75c060
@edmundmiller
ci: Fix typo
2015a0a
@edmundmiller
test: Fix naming so tests get picked up by pytest
afdf853
@edmundmiller
ci: Fix another yaml typo
5d42375
@edmundmiller
ci: Keep working-directories of failures
f0ce755
@edmundmiller
test(sra_download): main => main.nf
002df2f
@edmundmiller
ci: Add " around matrix parameters
ca49170
@github-actions
Copy link

YAML linting is failing

To keep the code consistent with lots of contributors, we run automated code consistency checks.
To fix this CI test, please run:

  • Install yaml-lint
  • Fix the markdown errors
    • Run the test locally: yamllint $(find . -type f -name "*.yml" -o -name "*.yaml")
    • Fix any reported errors in your YAML files

Once you push these changes the test should pass, and you can hide this comment 👍

We highly recommend setting up yaml-lint in your code editor so that this formatting is done automatically on save. Ask about it on Slack for help!

Thanks again for your contribution!

@edmundmiller edmundmiller marked this pull request as draft June 10, 2021 21:19
grst
grst reviewed Dec 30, 2021
View reviewed changes
Copy link
Member

@grst grst left a comment •
edited

Choose a reason for hiding this comment

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

This is really neat! Is there still anything missing?

I guess the next step would be to port this into #tools and add it to the pipeline template?

EDIT: just saw that this is already used in production in #sarek (nf-core/sarek#370). So no major obstacles?

.github/workflows/ci.yml
Comment on lines -50 to +66
name: Test STAR Salmon with workflow parameters
name: Test STAR Salmon with ${{ matrix.parameters }}
Copy link
Member

Choose a reason for hiding this comment

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

Do we need a separate star_salmon for this? Couldn't this just be another entry in the matrix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It could be another entry in the matrix, I was thinking this was a good interim to make sure we didn't miss anything in the conversion.

@edmundmiller
Copy link
Contributor Author

Nope, it should just work! Steps in case I hadn't listed them out

  1. Convert end 2 end (the current ci) to pytest. This is done but definitely needs updating since I wrote it a few months ago.
  2. Start testing local modules
  3. Test local workflows

@grst
Copy link
Member

grst commented Dec 30, 2021

But would you eventually want to merge this PR, or should we get it in tools first and get it into each pipeline using template sync? Ofc, the tests would still need to be written, but I think it would be great to encourage pytest workflow as default for all pipelines.

@grst grst mentioned this pull request Jan 31, 2022
Closed
@edmundmiller edmundmiller mentioned this pull request Oct 10, 2022
Closed
@edmundmiller edmundmiller deleted the pytest-workflow branch October 10, 2022 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maxulysse maxulysse maxulysse left review comments

@grst grst grst left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@edmundmiller @grst @maxulysse