-
Notifications
You must be signed in to change notification settings - Fork 735
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
Add nftests custom modules #7201
base: master
Are you sure you want to change the base?
Conversation
…nftests_custom_modules
You will also need to remove the modules from the pytest config file. Afterwards we should be good to go :) |
There is a linting error: What do you think about this? @famosab |
Huh that is interesting can you commit the "fixed" yaml files and push them to this PR so I can have a look? |
Take a look: eb84fde |
- "*R_sessionInfo.log": | ||
type: map | ||
description: | | ||
Groovy Map containing information on experiment. | ||
e.g. [ id:'test' ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the update was needed because we changed one of the outputs per my request. But why --fix
now does these kind of replacements also seems off to me. @mashehu @mirpedrol what do you guys think? is that expected behavior and should we just manually revert these changes or is something wrong with tools?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing the name of the output and using *R_sessionInfo.log
is the right change but replacing the type and description is not! We saw the same with other modules, it must be a weird bug with the --fix
command, you can revert this.
And the same for the other outputs where this happened, like filtered.csv
and tests.csv
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are almost done :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there was a new convention recently, maybe you can update to adhere to that: https://nf-co.re/docs/guidelines/components/modules#configuration-of-extargs-in-tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to make sure, you are requesting me to pass the values that I'm providing via ext.args using params instead of the actual value? Didn't you correct me to do it this way on a previous comment? https://github.com/nf-core/modules/pull/7201/files/816ff3b787af7e12e75a7d95f76b631f474853ee#r1888548704 Want to make sure I'm doing it the exact way you are suggesting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I did then I am sorry. But what is meant is that the params that you need to pass are written directly in the nf.test file which makes its directly understandable for other people that look at your tests and then they do not have to go through multiple nextflow.config files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, sorry, you are right, I was wrong both times, I just wasn't understanding how you wanted me to structure the config, I think I have now understood and addressed the issues, let me know what you think @famosab 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comment above
It seems that I'm no longer able to push to this branch: |
eb84fde
to
50e2138
Compare
@nschcolnicov I think you should be able to push again :) |
@famosab Tests should be passing but the docker_self_hosted one is having an issue, I don't think it is related to my changes though. I see they all fail with this error: |
PR checklist
Closes #XXX
versions.yml
file.label
nf-core modules test <MODULE> --profile docker
nf-core modules test <MODULE> --profile singularity
nf-core modules test <MODULE> --profile conda
nf-core subworkflows test <SUBWORKFLOW> --profile docker
nf-core subworkflows test <SUBWORKFLOW> --profile singularity
nf-core subworkflows test <SUBWORKFLOW> --profile conda