-
Notifications
You must be signed in to change notification settings - Fork 34
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
Separate CI tests #350
Separate CI tests #350
Conversation
|
format the if condition
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.
Getting there!
.github/workflows/ci.yml
Outdated
- "--preprocessing_qc_tool=falco" | ||
- "--shortread_qc_tool=fastp" | ||
- "--shortread_qc_tool=adapterremoval" | ||
- "--shortread_complexityfilter_tool=bbduk" |
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 guess these still need to be replaced with profiles?
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.
Do you mean changed it into:
- "docker,--preprocessing_qc_tool=falco --shortread_qc_tool=fastp --shortread_qc_tool=adapterremoval --shortread_complexityfilter_tool=bbduk --shortread_complexityfilter_tool=prinseqplusplus"
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, if i have a spare 30 minutes today I'll push changes what I mean
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.
What I mean is: -profile test_falco
for example, then -profile test_bbduk
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.
Thanks! I will prepare the rest of test configs.
.github/workflows/ci.yml
Outdated
wget https://raw.githubusercontent.com/motu-tool/mOTUs/master/motus/downloadDB.py | ||
python downloadDB.py > download_db_log.txt | ||
echo 'tool,db_name,db_params,db_path' > 'database_motus.csv' | ||
echo 'motus,db_mOTU,,db_mOTU' >> 'database_motus.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.
And these lines need to be replace with a bash if else statement (if matrix.tags), and merged with the main tests, and the github actions YAML if:
statement needs to be removed -> allowing to have a single test, entry in the YAML and the conditionals within the command:
block (I hope that makes sense!)
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 problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll need to check the bash, something is stopping the full evaluation before the final fi
. You could try copy pasting into shellcheck and maybe that will give you more info
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 agree with you. I though it could be caused by indentation. But the command lines work in shell if I just replace the 2 space (default in yml ) into tab (default in shell). No idea how to solve it.
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.
Thanks to @samuell the issue was solved. It was not allowed to use multiline code within a bash block while using Wandalen/wretry.action@v1.0.11
@jfy133 All CI tests passed except test_motus which seems caused by the path of database_motus.csv. Have tried to change it into ${{github.workspace}}/database_motus.csv and it didn't help. |
Edit: maybe try |
@jfy133 you are right that it seems like I can not use if condition under action according to Wandalen/wretry.action#16 |
Oops - @LilyAnderssonLee all test profiles except test_nothing and test_full!! The latter is a special case that will only run on AWS! |
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.
A few minor tweaks:
- For test configs that use tools that 'modify' the FASTQ files (e.g. complexity filtering) I think it is safer to actually run all downstream steps to make sure for whatever reason they don't 'corrupt' the FASTQ file for the downstream steps for whatever reason (however with a smaller set of profilers to reduce our computational footprint)
Good point! I agree with changes. |
PR checklist
Fix the issue #314
nf-core lint
).nextflow run . -profile test,docker --outdir <OUTDIR>
).docs/usage.md
is updated.docs/output.md
is updated.CHANGELOG.md
is updated.README.md
is updated (including new tool citations and authors/contributors).