Skip to content
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 md5-sums to the test-yml-files #696

Merged
merged 71 commits into from Aug 23, 2022

Conversation

asp8200
Copy link
Contributor

@asp8200 asp8200 commented Jul 28, 2022

#695

Adding checks for md5-sums in CI-tests - where possible. For text-based output-files with changing md5-sums, I've tried adding some tests of certain strings in the output-files.

The binary-output-files with changing md5-sums could perhaps be tested with some custom tests (https://pytest-workflow.readthedocs.io/en/stable/#writing-custom-tests). I haven't tried doing that here. It would be another tedious and lengthy task to do custom tests for all the binary-output-files with changing md5-sums.

If this looks okay to you guys, then I'll update the changelog accordingly.

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/sarek branch on the nf-core/test-datasets repository.
  • Make sure your code lints (nf-core lint).
  • Ensure the test suite passes (nextflow run . -profile 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).

@github-actions
Copy link

github-actions bot commented Jul 28, 2022

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 7749eb7

+| ✅ 145 tests passed       |+
#| ❔   4 tests were ignored |#
!| ❗   1 tests had warnings |!

❗ Test warnings:

  • readme - README did not have a Nextflow minimum version badge.

❔ Tests ignored:

  • files_unchanged - File ignored due to lint config: assets/nf-core-sarek_logo_light.png
  • files_unchanged - File ignored due to lint config: docs/images/nf-core-sarek_logo_light.png
  • files_unchanged - File ignored due to lint config: docs/images/nf-core-sarek_logo_dark.png
  • files_unchanged - File ignored due to lint config: lib/NfcoreTemplate.groovy

✅ Tests passed:

Run details

  • nf-core/tools version 2.4.1
  • Run at 2022-08-23 07:32:02

Copy link
Contributor

@FriederikeHanssen FriederikeHanssen left a comment

Choose a reason for hiding this comment

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

This is fantastic ❤️ Thank you so much for going through everything and adding the md5sums.

There are some file for which you are testing for content when the md45sums change and for some you aren't. Is there a reason for it?

tests/test_aligner.yml Show resolved Hide resolved
tests/test_aligner.yml Show resolved Hide resolved
- path: results/reports/fastqc/test-test_L1
- path: results/reports/markduplicates/test/test.md.metrics
# The text-based output-file test.md.metrics only contains comments and no metrics, which is a bit odd.
Copy link
Contributor

Choose a reason for hiding this comment

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

I forgot about this, we should check it out, to make sure it works fine

tests/test_umi.yml Show resolved Hide resolved
@asp8200
Copy link
Contributor Author

asp8200 commented Aug 16, 2022

There are some file for which you are testing for content when the md45sums change and for some you aren't. Is there a reason for it?

If the md5sum is changing, then I have (or should have) tested the file using contains if
1 the file is text-based (as we can't test binary files with contains) and
2 the file contains some string suitable for testing.

There are some text-based files which only contain only comments (didn't seem relevant for testing) and some html-files with a structure that didn't lend itself very well to testing with contains (the problem was including tabs and newlines in the tested strings. I didn't try handling that with contains_regex. I just got fed up with running the tests.)

Is there any particular file or files that you feel should be tested but isn't?

@maxulysse
Copy link
Member

That is some tremendous work.
You went way beyond what I had in mind.
That's wonderful.
I love this so much.
Small comments about code cohesion when having contains.
I think the VEP checks can be improved, but apart from that, it's close to perfection

asp8200 and others added 5 commits August 22, 2022 11:23
Co-authored-by: Maxime U. Garcia <maxime.garcia@scilifelab.se>
Co-authored-by: Maxime U. Garcia <maxime.garcia@scilifelab.se>
Co-authored-by: Maxime U. Garcia <maxime.garcia@scilifelab.se>
Co-authored-by: Maxime U. Garcia <maxime.garcia@scilifelab.se>
@maxulysse
Copy link
Member

Could you update CHANGELOG as well?

Copy link
Member

@maxulysse maxulysse left a comment

Choose a reason for hiding this comment

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

<3

@asp8200 asp8200 merged commit f84cae4 into nf-core:dev Aug 23, 2022
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.

None yet

5 participants