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-workflow #80

Merged
merged 11 commits into from
Nov 24, 2020
Merged

Pytest-workflow #80

merged 11 commits into from
Nov 24, 2020

Conversation

edmundmiller
Copy link
Contributor

@edmundmiller edmundmiller commented Nov 16, 2020

Adds basic pytest-workflow tests for each module. So we can go from the CI just proving stuff runs without failing (which is one giant leap for bioinformatics) to that the outputs are as expected too, and if not the tests need to change. Which would make bumping software packages a breeze.

Running locally is just pytest --symlink I think the symlink is due to the inputs being symlinked. Would it be possible to just do ../../../data/test.fasta etc instead?

Oh, I forgot that it can run multiple tests at the same time so the workflows might be better split up. I'll come up with some new structure on that.

I'm keeping it under draft while we decide on some "style" points, then I'll refactor the rest of the modules(possibly in another PR) once that's approved so it'll stay quick to review any changes.

This will check for the actual output files and their contents not just
whether the module runs or not.
The md5 sums change for the zip files every run so it's difficult to hit
a moving target.
@edmundmiller edmundmiller self-assigned this Nov 16, 2020
@ewels
Copy link
Member

ewels commented Nov 16, 2020

Love it! What do you think about trying to bundle the yaml test files with in the module directories so that all the files are there together? Too much?

I'd love to also have the GitHub actions files there too, but I don't think that this is possible sadly..

@edmundmiller
Copy link
Contributor Author

edmundmiller commented Nov 16, 2020

I was just following the getting started instructions

  • Create a tests directory in the root of your repository.
  • Create your test yaml files in the tests directory. The files need to start with test and have a .yml or .yaml extension.

So sadly I don't think that's a possibility. I personally would prefer something like

tests/fastqc/test_fastqc.yml
tests/fastqc/main.nf

And then in the main.nf the processes are imported. Keeping the tests and src separate. But I've seen either structuring in similar structures. But I think if we want to take advantage of --wt, --workflow-threads We'll need to break these up into smaller workflows. Or possibly we can use -entry let me test that really quick.

`--wt, --workflow-threads` is now an option and can be used to run tests
simultaneously.
@edmundmiller
Copy link
Contributor Author

Yep that worked as expected! Note there's no need for a main workflow anymore either and we can really start to break the testing up into smaller pieces. Then the main workflow can be for pieces of software that might need more e2e testing, like creating a genome index and then aligning it!

@edmundmiller
Copy link
Contributor Author

I'd love to wrap this up, I think the only thing holding me up is:

  • Do we want all the tests changed in this PR? Or this one as a proof of concept and then a mass PR for the rest?
  • Do we want the tests still in each module (SOFTWARE/subcommand/test/main.nf & tests/test_fastqc.yml)? Or would we rather have all tests in the test directory since the directory is required for pytest-workflow(tests/fastqc/main.nf and tests/fastqc/test_fastqc.yml)
  • Should we keep the symlinking of the input files?

@grst
Copy link
Member

grst commented Nov 24, 2020

Just had a closer look and this does look great 🎉

Regarding the organization of directories:

  • I don't really like that we need a separate GH actions yml file for each module
  • I don't really like that we need this tests directory.

(happy to hear other opinions!)

I was wondering if using the bits @ewels posted on slack, we could solve both of these issues and
introduce a "test autodiscovery" from a single github action, that spawns "pytest workflow" for each changed folder using the "test matrix" strategy.
Like that the pytest-workflow test-dir could be set to each module directory, and the tests folder contained in each module.

IMO, that would clean up quite a few redundancies.

@edmundmiller
Copy link
Contributor Author

edmundmiller commented Nov 24, 2020

* I don't really like that we need this `tests` directory.

Just so my opinion is heard I prefer the tests in the tests directory. I don't about the main.nf for the tests because I couldn't figure out clean paths if I did them from the test directory.

I was wondering if using the bits @ewels posted on slack, we could solve both of these issues and
introduce a "test autodiscovery" from a single github action, that spawns "pytest workflow" for each changed folder using the "test matrix" strategy.

I agree I think that could be fixed. My opinion would be to create an issue of this, and address it in a future PR so we can get the tests moved over to pytest-workflows and then discuss that after.

Like that the pytest-workflow test-dir could be set to each module directory, and the tests folder contained in each module.

This is definitely the solution to the tests dir requirement if that's the path we go.

@grst
Copy link
Member

grst commented Nov 24, 2020

Just so my opinion is heard I prefer the tests in the tests directory. I don't about the main.nf for the tests because I couldn't figure out clean paths if I did them from the test directory.

I'm fine with a tests directory -- but then everything test-related should go there. I'd just like to avoid having a mixture of both a global and per-module tests directory.

My opinion would be to create an issue of this, and address it in a future PR so we can get the tests moved over to pytest-workflows and then discuss that after.

Fine for me! As a proof-of-principle, I'd say this PR is good to go!

@edmundmiller
Copy link
Contributor Author

I'm fine with a tests directory -- but then everything test-related should go there. I'd just like to avoid having a mixture of both a global and per-module tests directory.

Exactly how I feel also! The mixture gets confusing.

@edmundmiller
Copy link
Contributor Author

Alright I daisy-chained #84 to this one so it can be quickly reviewed what it would look like in comparision. I left a few different options for the test files

  • ${projectDir}/input/test_single_end.fastq.gz
  • ${launchDir}/tests/data/fastq/rna/test_R1.fastq.gz
  • ${projectDir}/../data/fastq/rna/test_R2.fastq.gz

I prefer the second, first then third in my order of preferences. The second allows us to remove the symlinks and cut down on refactoring them to work if the structure changes in the future.

@edmundmiller edmundmiller marked this pull request as ready for review November 24, 2020 20:22
edmundmiller and others added 4 commits November 24, 2020 20:26
Co-authored-by: Harshil Patel <drpatelh@users.noreply.github.com>
Co-authored-by: Harshil Patel <drpatelh@users.noreply.github.com>
@grst
Copy link
Member

grst commented Nov 24, 2020

I really like the new structure! 🚀

Co-authored-by: Harshil Patel <drpatelh@users.noreply.github.com>
Copy link
Member

@drpatelh drpatelh left a comment

Choose a reason for hiding this comment

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

Beautiful 😍 Just beautiful @emiller88 🙇 So we don't need to change the Actions workflow to factor in the changed directory structure?

@edmundmiller
Copy link
Contributor Author

edmundmiller commented Nov 24, 2020

No see #83 for the next refactor, we'll have a single test_module.yml or something that will run a matrix based off the directory structure! That'll be a new PR. So up next

  • Refactor github actions to be more DRY
  • Refactor all the modules to use pytest-workflow

@drpatelh drpatelh merged commit 794a5cb into master Nov 24, 2020
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.

None yet

4 participants