sarek icon indicating copy to clipboard operation
sarek copied to clipboard

Published 20 hours ago • verified publisher icon nf-core

Adding md5-sums to the test-yml-files

Open asp8200 opened this issue 3 years ago • 2 comments

#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).

asp8200 avatar Jul 28 '22 14:07 asp8200

nf-core lint overall result: Passed :white_check_mark: :warning:

Posted for pipeline commit 7749eb7

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

:heavy_exclamation_mark: Test warnings:

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

:grey_question: 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

:white_check_mark: Tests passed:

Run details

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

github-actions[bot] avatar Jul 28 '22 14:07 github-actions[bot]

Hi @asp8200, I agree with Lasse, it looks good overall. Could you double check some md5sums if they are really correct?

SusiJo avatar Aug 10 '22 14:08 SusiJo

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?

asp8200 avatar Aug 16 '22 09:08 asp8200

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

maxulysse avatar Aug 22 '22 09:08 maxulysse

Could you update CHANGELOG as well?

maxulysse avatar Aug 22 '22 09:08 maxulysse