-
Notifications
You must be signed in to change notification settings - Fork 953
fix(fastk/merge): use topic versions and update snapshots #9666
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
base: master
Are you sure you want to change the base?
Conversation
| Groovy Map containing sample information | ||
| e.g. [ id:'test', single_end:false ] | ||
| - "*.ktab*": | ||
| - '"*.ktab*", hidden: true': |
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 tried "*.ktab*" on my computer and the linting went without error. Is it really needed to hide something here?
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.
It seems to be a regression in the dev version of tools: https://nfcore.slack.com/archives/CE5LG7WMB/p1768480946931369
I agree we should just use *.ktab* here ideally! But we do need to capture the hidden files in the module as the kmer databases consist of a normal index file and N hidden files with a . prefix.
| Groovy Map containing sample information | ||
| e.g. [ id:'test', single_end:false ] | ||
| - "*.{prof,pidx}*": | ||
| - '"*.{prof,pidx}*", hidden: true': |
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.
Same here, is hidden: true really needed?
| then { | ||
| assertAll( | ||
| { assert process.success }, | ||
| { assert snapshot(process.out).match() } |
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 tried in my computer with { assert snapshot(process.out).match() } and it works fine. Is it really needed to check each output channel one by one?
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.
This one was also passing fine locally on my computer, but the snapshot was not stable when run on CI. I think the merging of two tables is non-deterministic in some way that might depend on e.g. the locale...
PR checklist
Closes #XXX
topic: versions- See version_topicslabelnf-core modules test <MODULE> --profile dockernf-core modules test <MODULE> --profile singularitynf-core modules test <MODULE> --profile condanf-core subworkflows test <SUBWORKFLOW> --profile dockernf-core subworkflows test <SUBWORKFLOW> --profile singularitynf-core subworkflows test <SUBWORKFLOW> --profile conda