-
Notifications
You must be signed in to change notification settings - Fork 16
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
Adding our nf-test tests to the GitHub Actions CI #66
Conversation
|
Ah, the paths I use to force more CI tests probably don't exist when running in the PR, so I've commented that out. |
Looks like one of the things we're getting hung up on is a missing file for a module test for mesmer:
Looking into how to fix this. |
Easy fix. It just needed a small path change. |
@nf-core-bot fix linting |
Hi @RobJY, |
Looks like everything is passing except this lint error:
Not sure why a file we added is setting off the template "files_unchanged" error. |
@nf-core-bot fix linting |
Hmm, no, bot didn't fix it. |
Ok, that worked, but it seems like more of a workaround than a permanent fix. I just added |
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.
Overall looks good to me! Great job sorting out the linting issues. I just left a couple of questions that are more of a technicality for clarification.
modules/nf-core/basicpy/basicpy.diff
Outdated
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.
Is there a specific reason the module was changed? I have an open PR adding nf-test to basicpy nf-core/modules#6929 if there are some changes to be made to make the module more standardized feel free to add to that PR (it shouldn't be an issue since it's only in MCMICRO currently).
I don't think this is an issue. Especially if the logo is updated down the line, this would be added to |
Co-authored-by: Krešimir Beštak <86408271+kbestak@users.noreply.github.com>
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.
LGTM 🚀
The logo is a known problem. There's a really subtle font rendering difference in the CI environment compared to our local environments. We had it set up previously where the file in git was the version the CI check would approve, and only local lint tests would fail that check (and we know to ignore that). |
rebased on dev
rebased on dev
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.
Looks good overall. My comments are mostly about removing commented-out stuff, but also I think there's a better fix for the lint error with the dark logo png.
PR checklist
nf-core lint
).nextflow run . -profile test,docker --outdir <OUTDIR>
).nextflow run . -profile debug,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).This is still a work in progress, but I think it's getting close.
To do Items: