PROTOTYPE: Cucumber messages v32.0.1 with changes to attachment handling

[clrudolphi]

🤔 What's changed?

This is early prototype. This branch depends upon the branch from PR #984 (as that one has other Cucumber upgrades that are prerequisites to this).

Adds support for Cucumber.Messages v32.0.1 and the new Message type 'ExternalAttachments'
New configuration will be available, such as:

"formatters": {
    "message": {
      "outputFilePath": "CucumberMessages/reqnroll_externalAttachments_report.ndjson",
      "attachmentHandlingOptions": {
        "attachmentHandling": "External",
        "externalAttachmentsStoragePath": "path/to/CucumberMessages/Attachments"
      }
    }
  }

There is a new section called 'attachmentHandlingOptions'. The 'attachmentHandling' property could be None/Embed/External
If External the 'externalAttachmentsStoragePath' is used when crafting ExternalAttachment objects.

The overall idea is that users will call IReqnrollOutputHelper("pathToAttachment");

The presumption is that this 'pathToAttachment' is given to the underlying test frramework (such as NUnit) and that it might copy/move that attachment to a permanent location that the report should reference.
Thus with the 'externalAttachmentsStoragePath', we're asking users to tell us where the test framework will place the attachments (post-test run).
Custom formatters that inherit from FormatterBase will be able to programmatically override that setting and/or override how attachment handling works overall.

⚡️ What's your motivation?

🏷️ What kind of change is this?

• ⚡ New feature (non-breaking change which adds new behaviour)

♻️ Anything particular you want feedback on?

anything;

📋 Checklist:

• I've changed the behaviour of the code
  • I have added/updated tests to cover my changes.
• My change requires a change to the documentation.
  • I have updated the documentation accordingly.
• Users should know about my change
  • I have added an entry to the "[vNext]" section of the CHANGELOG, linking to this pull request & included my GitHub handle to the release contributors list.

---

This text was originally taken from the template of the Cucumber project, then edited by hand. You can modify the template here.

[304NotModified]

Nitpick

Maybe instead of

 "attachmentHandlingOptions": {
        "attachmentHandling": "External",
        "externalAttachmentsStoragePath": "path/to/CucumberMessages/Attachments"
      }

This would be more clear and clean IMO:

 "attachmentHandling": {
        "mode": "External",
        "attachmentsPath": "path/to/CucumberMessages/Attachments"
      }

[clrudolphi]

Nitpick

Not at all! That's a really good idea.

[clrudolphi]

I want to come back to this prior to a v4 release. I have rethought this and I am leaning towards scrapping this PR and proceeding in a different direction.
From what I can find in the documentation of the test frameworks, none of them "move" the file. Not all of them even support attachments.
So my intended direction is to extend UnitTestProviderConfiguration and have each test framework adapter set a handling mode value during their initialization (NONE, External, Embed). There will be no configuration of the path. There will be no configuration per provider.

• MsTest: External (given the file path/name)
• NUnit: External (given the file path/name)
• xUnit v2: NONE (not supported)
• xUnit v3: Embedded
• TUnit: External (given the file path/name)

Any objections? Thoughts?
@gasparnagy @304NotModified

[gasparnagy]

I want to come back to this prior to a v4 release. I have rethought this and I am leaning towards scrapping this PR and proceeding in a different direction. From what I can find in the documentation of the test frameworks, none of them "move" the file. Not all of them even support attachments. So my intended direction is to extend UnitTestProviderConfiguration and have each test framework adapter set a handling mode value during their initialization (NONE, External, Embed). There will be no configuration of the path. There will be no configuration per provider.

• MsTest: External (given the file path/name)
• NUnit: External (given the file path/name)
• xUnit v2: NONE (not supported)
• xUnit v3: Embedded
• TUnit: External (given the file path/name)

Any objections? Thoughts? @gasparnagy @304NotModified

I think I disagree. The unit test providers maybe don't move the files, but VsTest and the TRX logger does copy the files to the test results folder. So as a result the test results folder - with the TRX file and the related subfolders contain a standalone copy of the "logical test result".

In our case, the HTML/NDJSON file was the standalone "logical test result" so it was good that we embedded the attached files into that.

The new feature in Cucumber Messages is about providing an option to extend the meaning of "logical test result" to multiple file system files. It is now our decision if we want to use this option or not.

(Side note: The external embedded files are not yet supported by many downstream Cucumber Messages tooling, so by using the "external" option for almost all attachments would cause issues.)

Seeing the problem now, after some time, I think we should just upgrade Cucumber Messages and keep using the embedded attachments for now (so not use the new feature). We can reconsider using the external one later, once we will better see how other tools support it.

[clrudolphi]

I think we should just upgrade Cucumber Messages and keep using the embedded attachments for now (so not use the new feature). We can reconsider using the external one later, once we will better see how other tools support it.

OK. I've parked #1015 (this PR) and #1056 (which is an alternative take on how to support external attachments, which I suspect you'll like even less).

Please do review #984 as that PR is the one that updates to all the latest Cucumber dependencies (without changing how we emit attachments).

[gasparnagy]

Please do review #984 as that PR is the one that updates to all the latest Cucumber dependencies (without changing how we emit attachments).

That one is already approved by me. I was waiting for the release decisions with the merge. Let's discuss this on Discord.