Skip to content

test conversion to nf-test#239

Closed
matbonfanti wants to merge 25 commits intonf-core:devfrom
matbonfanti:28-add-nf-test
Closed

test conversion to nf-test#239
matbonfanti wants to merge 25 commits intonf-core:devfrom
matbonfanti:28-add-nf-test

Conversation

@matbonfanti
Copy link
Copy Markdown

PR checklist

  • This comment contains a description of changes (with reason).
  • If you've fixed a bug or added code that should be tested, add tests!
  • If you've added a new tool - have you followed the pipeline conventions in the contribution docs
  • If necessary, also make a PR on the nf-core/crisprseq branch on the nf-core/test-datasets repository.
  • Make sure your code lints (nf-core pipelines lint).
  • Ensure the test suite passes (nextflow run . -profile test,docker --outdir <OUTDIR>).
  • Check for unexpected warnings in debug mode (nextflow run . -profile debug,test,docker --outdir <OUTDIR>).
  • Usage Documentation in docs/usage.md is updated.
  • Output Documentation in docs/output.md is updated.
  • CHANGELOG.md is updated.
  • README.md is updated (including new tool citations and authors/contributors).

@matbonfanti matbonfanti self-assigned this Mar 24, 2025
@matbonfanti matbonfanti marked this pull request as draft March 24, 2025 14:17
@matbonfanti matbonfanti mentioned this pull request Mar 24, 2025
Comment thread nf-test.config
Comment thread tests/main_screening.nf.test Outdated
Comment thread tests/main_screening.nf.test Outdated
@matbonfanti
Copy link
Copy Markdown
Author

hi @mirpedrol, sorry if it took me so long, but I have now completed the tests.

Thanks a lot for the suggestion, it was very useful! I took the configuration and also the format of the test from the development version of the template. I think that when it will be time to update the project with new release of the template the tests should more or less be ready.

@matbonfanti matbonfanti marked this pull request as ready for review April 11, 2025 15:28
@matbonfanti
Copy link
Copy Markdown
Author

@mirpedrol I think that the PR is ready for review.

The new nf-test-based tests are currently not included in the GitHub CI workflow, as I haven't modified the existing workflow configuration. I believe it would be more convenient to wait for the release of the updated template and then integrate the new GitHub workflow in its entirety.

@matbonfanti matbonfanti requested a review from mirpedrol April 11, 2025 15:34
@mirpedrol
Copy link
Copy Markdown
Member

thanks for working on this @matbonfanti, I will have a look asap 🙂

Copy link
Copy Markdown
Member

@mirpedrol mirpedrol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! I will approve the PR, but I would like someone more experienced with nf-test than me to have a second look

Comment thread tests/.nftignore Outdated
Comment thread tests/.nftignore Outdated
Comment thread tests/main_screening_full.nf.test Outdated
Comment thread tests/main_targeted_full.nf.test Outdated
@mirpedrol mirpedrol requested a review from LaurenceKuhl April 14, 2025 09:57
@maxulysse
Copy link
Copy Markdown
Member

I would update the CI so that it ran nf-test, as far as I can tell, only the old tests were run, and none of the new ones

@mirpedrol
Copy link
Copy Markdown
Member

@matbonfanti to add the CI you can copy this file form the template. Let me know if you need help 😄

matbonfanti and others added 2 commits April 14, 2025 12:49
Co-authored-by: Júlia Mir Pedrol <mirp.julia@gmail.com>
Co-authored-by: Júlia Mir Pedrol <mirp.julia@gmail.com>
@matbonfanti
Copy link
Copy Markdown
Author

@mirpedrol sure, no problem, will do!

@mirpedrol
Copy link
Copy Markdown
Member

Hi @matbonfanti,
Some other files were missing for the nf-test CI, I added them, I hope that's ok. Once the tests pass, I think this PR can be merged

@mirpedrol
Copy link
Copy Markdown
Member

@nf-core-bot fix linting please!

@matbonfanti
Copy link
Copy Markdown
Author

hi @mirpedrol sorry for not being very responsive... anyway, thanks for taking care of the nf-test pipeline... I will look into this in the next few days when I have time!

@mirpedrol
Copy link
Copy Markdown
Member

I am a bit confused why the tests are failing, I regenerated the snapshots from codespaces to make sure we were using the same nextflow version as in the CI, but now new errors appear 😱 maybe you have better luck than me when you can have a look 😄

@matbonfanti
Copy link
Copy Markdown
Author

Hi @mirpedrol,
I’ve made some progress, it turned out a couple of files had embedded timestamps causing issues.

However, the "Test targeted UMIs" is still failing in a puzzling way. In the CI workflow, the behavior of the Nextflow run seems completely different. For example, the test produces only 19 output files in CI, whereas I get 79 when running locally.
Since I’m not very familiar with the targeted analysis workflow, I was wondering if you might have some insight into what could be happening?

Comment thread tests/main_targeted_umis.nf.test Outdated
Comment thread nf-test.config Outdated
@matbonfanti
Copy link
Copy Markdown
Author

Hi @mirpedrol, there are still errors for the "old style" testing workflow, but I cannot figure out what the problem is... do you have any idea of what is going on?

@matbonfanti
Copy link
Copy Markdown
Author

@mirpedrol it seems that the update of the template fixed the previous issue.

However, now I get again a problem with the main_screening.nf.test. As far as I see, the MAGECK_FLUTEMLE_CONTRASTS writes a different number of files when run with the latest nextflow, which seems a very weird behaviour. I need to look more closely into this...

@mirpedrol
Copy link
Copy Markdown
Member

I missed your last message @matbonfanti. This last test failing is weird. Does it work for you if you run nf-test locally? We can check the .nextflow.log to see if it gives us more information.

@matbonfanti
Copy link
Copy Markdown
Author

Yes, I re-created the snap locally at some point, and it was working fine. I was using nf 24.04.2, but it would be very strange if the version of nextflow determines the number of files that are written by MAGeCKFlute... if this is really the case, there is something to fix in this process...

@mirpedrol
Copy link
Copy Markdown
Member

I tested with the last version and it passes both locally and on gitpod, so maybe it could be related to the NF version + the runners. I will modify the github action to be able to check the .nextflow.log and debug

@mirpedrol
Copy link
Copy Markdown
Member

well, my addition didn't work, so we can't check the .nextflow.log, but the test passed now 😅 removing my change, and then we can merge!

@matbonfanti
Copy link
Copy Markdown
Author

this test is driving me nuts 😮

anyway, thanks a lot for the support!

@matbonfanti matbonfanti closed this May 9, 2025
@mirpedrol
Copy link
Copy Markdown
Member

Was this closed by mistake? 👀

@matbonfanti
Copy link
Copy Markdown
Author

No, I did it on purpose... I merged the PR yesterday (and if you check the commit history of dev you can in fact see that the merge was successfull).

However, for some reason the PR here seems to be stuck. There is this message "This pull request is closed, but the 28-add-nf-test branch has unmerged commits." but if I am not mistaking this is not the case. So, I decided to close the PR.

@mirpedrol
Copy link
Copy Markdown
Member

I see the commits in the dev branch now, so weird that the PR shows as closed. Thanks for the clarification!

LeonHornich pushed a commit to LeonHornich/crisprseq that referenced this pull request May 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants