nf-test module/subworkflow lint #2494
Conversation
|
Related to #2474 |
|
This PR is needed to complete the nf-test linting implementation in nf-core/modules#4231 |
Codecov Report
@@ Coverage Diff @@
## dev #2494 +/- ##
==========================================
- Coverage 70.62% 70.50% -0.12%
==========================================
Files 88 88
Lines 9493 9515 +22
==========================================
+ Hits 6704 6709 +5
- Misses 2789 2806 +17
|
There was a problem hiding this comment.
I think we should just warn for now, and then we can turn the failure on once we have all the infrastructure in place, and we're fully deprecating the pytest-workflow tests.
https://github.com/orgs/nf-core/projects/43/views/5 should have all the stuff that we need done.
nf_core/modules/lint/module_tests.py
Outdated
| if os.path.exists(module.test_dir): | ||
| module.passed.append(("test_dir_exists", "Test directory exists", module.test_dir)) | ||
| else: | ||
| module.failed.append(("test_dir_exists", "Test directory is missing", module.test_dir)) | ||
| module.failed.append(("test_dir_exists", "nf-test directory is missing", nftest_testdir)) |
There was a problem hiding this comment.
Does this throw a failure or a warning? I'm fine if we want to really push the hard shift, but I think just a warning for now might be the right direction.
There was a problem hiding this comment.
It will throw two failures right now, but yeah I agree maybe the nf-test failure should be a warning. I'll try to figure out how to do that :p
| subworkflow.passed.append(("test_main_exists", "test `main.nf` exists", subworkflow.test_main_nf)) | ||
| else: | ||
| subworkflow.failed.append(("test_main_exists", "test `main.nf` does not exist", subworkflow.test_main_nf)) | ||
| subworkflow.failed.append(("test_main_exists", "test `main.nf.test` does not exist", nftest_main_nf)) |
There was a problem hiding this comment.
Same as above, warn for now, maybe toss a TODO in there.
|
Thanks for the review @emiller88! Currently the main logic still works as before but now it checks for the presence of nf-test tests too and doesn't throw the pytest failures if they are present |
| if os.path.exists(module.test_dir): | ||
| module.passed.append(("test_dir_exists", "Test directory exists", module.test_dir)) | ||
| else: | ||
| module.failed.append(("test_dir_exists", "Test directory is missing", module.test_dir)) | ||
| module.warned.append(("test_dir_exists", "nf-test directory is missing", nftest_testdir)) |
There was a problem hiding this comment.
The nf-test warning should be moved to the else after checking if nf-test dir exists.
Or use if/elif/else otherwise
There was a problem hiding this comment.
That was a typo 😁 The second if statement should have been elif :p
|
Thanks @mirpedrol 🍪 |
PR checklist
CHANGELOG.mdis updateddocsis updated