Report v3 attachments to VSTest

[bradwilson]

xunit/xunit#3063
xunit/xunit#2457 (comment)

[matt-richardson]

Digging into this, we found that this tidbit - the attachments are available on ITestFinished, but the test results are sent on ITestFailed/ITestPassed/etc.

Thinking about it, we could cache the attachments in the OnTestFinished event, and send them on the OnTestFailed event (or vice-versa. depending on which event comes first).

Or, we could revisit the decision to put the attachments on ITestFinished.

I'm happy to contribute a PR if it helps - would be very useful to get your input on it before I start on it @bradwilson.

[bradwilson]

I've been thinking about this, and it's not a simple answer.

The test messages come in this order: test starting => test result (passed/failed/etc.) => test finished

Probably the best way to do this is to short-lived cache the result message (via a ConcurrentDictionary, keyed by the test's unique ID) while waiting for the finish message, and then construct the test result during finish rather than result.

If you want to PR this, I'd be happy to accept it; if not, I'm also happy to do it. 😄 I definitely want to get it done before shipping 3.0.2 (which will co-ship with v3 1.1.0, which seems like its not-quite-10 feature/bug commits is probably close to ready to ship).

[matt-richardson]

I've had a play with this, trying to get it working, and I'm not having much luck.

Rider doesn't appear to support test attachments, and it's not working so well via TeamCity (maybe a faulty assumption (that it should work) on my behalf in there).

There also seems to be a mismatch how things plug together - xunit messages have a string/byte array, but the attachments (on Microsoft.VisualStudio.TestPlatform.ObjectModel.TestResult) expect a filename.

Finally, the only way I could get the tests to work was to #if NETCOREAPP - it was hanging when accessing args.Message.Attachments under net472.

I'm just about to head on vacation for a couple of weeks, where I wont be able to look at this, so if you're happy to tackle it, feel free to go for it.

[bradwilson]

Thanks for the research. The hang on net472 is surprising and worth looking into.

As for everything expecting filenames, I think the answer is to write things to files in the Temp folder, and the assumption is that VSTest owns those things. If it doesn't take a copy up into itself, then the Temp folder will be cleaned out when the computer reboots. Not optimal, but I suspect that's what they intend so they'll take ownership of that file (and hopefully delete the Temp version).

[matt-richardson]

Pushed up the code in case it's helpful 👍

[bradwilson]

I think the reason this isn't working as expected is that the attachments aren't actually showing up in the finished message. I'm digging into that to see if I can figure out what the issue is.

If the fix is required in the core framework and it's a breaking change, that means I won't be able to get it in until I ship core framework 2.0.0 (which means it won't be in 3.0.2), so 🤞🏼 that it isn't a big problem.

[bradwilson]

De-serialization of TestFinished.Attachments was broken. This is fixable in 1.1.0, so this should make it into the release that's coming out shortly.

[bradwilson]

When using this test:

[Fact]
public void Passing()
{
    var bytes = File.ReadAllBytes("logo-128-transparent.png");

    TestContext.Current.AddAttachment("Note", "This is important information");
    TestContext.Current.AddAttachment("Logo", bytes, "image/png");
}

I now see the attachments in Visual Studio:

The file extension is determined by mapping the media type to file extension, and it's fed by a pre-populated cache taken from https://developer.mozilla.org/en-US/docs/Web/HTTP/MIME_types/Common_types. On Windows OSes, when the media type is not in that list, we attempt to look it up in the system registry; on non-Windows OSes, there is no standardized fallback. For any media type with an unknown file extension, .bin will be used.

[bradwilson]

Available in 3.0.2-pre.7 https://xunit.net/docs/using-ci-builds

Your PR didn't allow me to push changes, so I made local updates and then pushed up the resulting commit here: b67d776

Notably, I made changes so that all attachments were in a sub-folder in Temp based on the test unique ID, so that the attached filename would look reasonable in the Test Explorer UI. That included adding sanitization logic for the attachment name => filename, plus the file extension cache above (that went into the core framework). I also added a bit of try/catch and some logging to note error conditions.

I'll just close your PR, since the code is already merged. 😄