nf-core / modules

Repository to host tool-specific module files for the Nextflow DSL2 community!
https://nf-co.re/modules
MIT License
286 stars 730 forks source link

Snapshot content of version files instead of a hash #6505

Open edmundmiller opened 3 months ago

edmundmiller commented 3 months ago

Currently we're snapshoting the versions.yml and getting a hash:

https://github.com/nf-core/modules/blob/17b8bedf740a8403b548e49853246f1d9a24caa3/modules/nf-core/ale/tests/main.nf.test.snap#L26-L28


                "versions": [
-                    "versions.yml:md5,949da9c6297b613b50e24c421576f3f1"
+                    "content": ["{ALE={ale=20180904}}"],
                ]
edmundmiller commented 3 months ago

@nvnieuwk Pointed out that topic channels won't be hashable

4517

famosab commented 3 months ago

How exactly would we assert for the versions then? I tried something like this:

{ assert snapshot(
             process.out,
             file(process.out.versions[0]).readLines()
             ).match()
}

We might need to adapt the linting or snapshot both md5 and the content. Otherwise this error occurs:

╭─ [✗] 1 Module Test Failed ───────────────────────────────────────────────────╮
│              ╷                               ╷                               │
│ Module name  │ File path                     │ Test message                  │
│╶─────────────┼───────────────────────────────┼──────────────────────────────╴│
│ wgsim        │ modules/nf-core/wgsim/tests/… │ versions not found in         │
│              │                               │ snapshot file                 │
│              ╵                               ╵                               │
╰──────────────────────────────────────────────────────────────────────────────╯
╭───────────────────────╮
│ LINT RESULTS SUMMARY  │
├───────────────────────┤
│ [✔]  50 Tests Passed  │
│ [!]   0 Test Warnings │
│ [✗]   1 Test Failed   │
╰───────────────────────╯
Error: Process completed with exit code 1.

Of course only if we do not assert process.out completely

{ assert snapshot(
             process.out.fastq[0][1].collect { file(it).name },
             file(process.out.versions[0]).readLines(),
             ).match()
}

Maybe there is a more elegant way?

edmundmiller commented 3 months ago

We might need to adapt the linting or snapshot both md5 and the content. Otherwise this error occurs:

Luckily, we make the rules of the linter so we can ignore any errors like that and fix it right after we settle on a standard!

I like file(process.out.versions[0]).readLines() but let me see what the snapshot looks like.

Ideally I'd love for it to look like


"sarscov2 [fasta_gz] - paired-end sorted bam": {
        "content": [
            {
                "0": [
                    [
                        {
                            "id": "test",
                            "single_end": false
                        },
                        "test_ALEoutput.txt:md5,4abcbd60ae1dbf78138c97e5fed97f3e"
                    ]
                ],
                "1": [
                    "versions.yml:md5,949da9c6297b613b50e24c421576f3f1"
                ],
                "ale": [
                    [
                        {
                            "id": "test",
                            "single_end": false
                        },
                        "test_ALEoutput.txt:md5,4abcbd60ae1dbf78138c97e5fed97f3e"
                    ]
                ],
-                "versions": [
-                    "versions.yml:md5,949da9c6297b613b50e24c421576f3f1"
-                ]
+                "versions": {
+                    "ale": "20180904"
+                }
            }
        ],
        "meta": {
            "nf-test": "0.8.4",
            "nextflow": "23.10.1"
        },
        "timestamp": "2024-03-19T09:06:19.589167"
    },
edmundmiller commented 3 months ago

Maybe there is a more elegant way?

I think we could write a lib function for this, either in this repo or the nf-test/utils plugin.

Two other things to consider:

  1. 4517 I think we need to see how these look when snapshotted.

  2. 4306 and the automated bumping of conda dependancies coming soon™️. I was just thinking about this and I'm proposing maybe we move to just getting the versions through the stub workflow. This would:

    A) Make running them actually test something besides stub functionality B) Allow us to do nf-test test --tag stub --update-snapshot ... in CI to automatically bump the version snapshot, and then run the actual tests without the fear of bumping any real test outputs that aren't the version. I haven't found a better way around this without requesting specific snapshot updates in nf-test itself.

SPPearce commented 3 months ago

Maybe there is a more elegant way?

I think we could write a lib function for this, either in this repo or the nf-test/utils plugin.

Two other things to consider:

  1. Migration to topic channels #4517 I think we need to see how these look when snapshotted.
  2. Renovate Workflow  #4306 and the automated bumping of conda dependancies coming soon™️. I was just thinking about this and I'm proposing maybe we move to just getting the versions through the stub workflow. This would: A) Make running them actually test something besides stub functionality B) Allow us to do nf-test test --tag stub --update-snapshot ... in CI to automatically bump the version snapshot, and then run the actual tests without the fear of bumping any real test outputs that aren't the version. I haven't found a better way around this without requesting specific snapshot updates in nf-test itself.

I definitely support snapshoting the version information itself, think that is much clearer than the md5sum. The stub test could get the versions, it only needs to be once, that sounds reasonable. And every module should have a stub and stub test (eventually), so sounds good to me.

edmundmiller commented 2 months ago

There's a function for yaml files in nf-test(that may have snuck in with 0.9.0) https://www.nf-test.com/docs/assertions/files/

edmundmiller commented 2 months ago

Magic line is:

{ assert snapshot(path(process.out.versions.get(0)).yaml).match("versions") },