Allow attaching files to TestResults #2152

ChrisMaddock · 2017-05-07T21:14:01Z

This is the framework functionality required for #1670. (Adding this PR to the milestone separately, as I want to track runner implementation with the issue.)

Specific design points:

API is TestContext.AddTestAttachment(string filepath, string description = null) Are we happy with that?
I forced all files to be absolute paths - based on the logic that it's the runner that decides where the test result XML will end up, I believe? This is the same approach MSTest takes.

What do people think? I've also added some line comments on things to check.

ChrisMaddock · 2017-05-07T21:17:01Z

src/NUnitFramework/framework/Internal/TestExecutionContext.cs

+
+            if (!Path.IsPathRooted(filepath))
+                filepath = Path.Combine(TestContext.CurrentContext.WorkDirectory, filepath);
+


Is this the correct way to build an absolute file path? I wanted to use Path.GetFullPath() - but it's not available in PCL.

Looks good to me, although I think Path.GetFullPath() will be available once I get the .NET Standard version working.

Actually Path.GetFullPath() would be incorrect. We promise to store all output files relative to our WorkDirectory, which is not the current directory. The choice of name for the WorkDirectory, may have been a bad one, since it sounds a bit like the working directory, i.e. cwd, but I believe we are stuck with it.

A more basic question... why do we want to make this absolute at all? Why not just use what the user gives us?

Thanks for the explanation!

This was something else I went back and forth on. I wanted to make it absolute, as I had previously thought:

The attachments should always be relative the the results xml - as the file paths written there should be correct to the files current location.

The runner controls where the results xml is written, meaning that the runner could theoretically move where the results ended up.

What I didn't realise is that WorkDirectory wasn't the working directory. 😄 So will any results xml file always end up in TestContext.WorkDirectory? Or is that a runner setting, could TestResults be written anywhere, like I'd first thought?

I did think full paths would be easier to resolve should the user move the result xml file around for any CI etc. Is there a disadvantage to writing full paths, if we already define that they're relative to WorkDirectory? I guess if they remain relative, and you were adding all your attachments to the same subdir, you could move the directory as a whole?

The runner passes Work directory to the framework to be used in exactly this circumstance. The XML result file is saved there.
In the case of the console runner, it can be set by a command-line argument.

Ok, cool. I'll remove the stuff that meddles with paths in that case - if we have the concrete definition that paths will always be relative to WorkDirectory/results location. Thanks.

ChrisMaddock · 2017-05-07T21:18:11Z

src/NUnitFramework/tests/nunit.framework.tests-3.5.csproj

@@ -122,6 +123,7 @@
    <Compile Include="Internal\Results\TestResultOutputTests.cs" />
    <Compile Include="Internal\Results\TestResultSuccessTests.cs" />
    <Compile Include="Internal\Results\TestResultTests.cs" />
+    <Compile Include="Internal\Results\TestResultWarningTests.cs" />


This is unrelated - just noticed this file was missing from all except the 2.0 build.

ChrisMaddock · 2017-05-07T21:32:24Z

Regarding file location - MSTest also copies the file - but has a fully fledged directory structure to support that.

This currently just references whatever the user passes in - meaning the link will be broken if the results are transferred between machines. Do we want to go any further, and (e.g.) copy the files to a directory next to the result xml? I wasn't entirely sure of the advantage, given a) potential copy permission errors and b) still requiring the user to copy both files and results, if that was the case. Also - the framework isn't currently aware of the result output location, I don't think? So I'm not sure how we'd determine where to copy the files to!

I'm tempted to think the simple option will be ok, until such a point as someone has a compelling argument to change it?

CharliePoole · 2017-05-07T21:36:13Z

@ChrisMaddock What do you mean by "I want to track runner implementation with the issue?"

ChrisMaddock · 2017-05-07T21:40:02Z

@CharliePoole - There's a number of people following issue #1670. Rather than close it, and leave them to track down the related runner issues, which need to be completed before they can use the functionality, I've converted it to an Epic, and will add issues for the different runners, once we've decided what to do for each.

The side effect of that, is that there's no dedicated issue for just the framework component of #1670 - so I added the PR to the Epic.

CharliePoole · 2017-05-07T21:51:23Z

I'm glad you took this on as it's a needed feature. I do need a bit of general explanation before doing a detailed review.

You are adding an entry to TestExecutionContext for attachments and then copying them to the result. Is there any reason you don't want to simply add them to the result directly? This would be analogous to how we add text output to the result.

In the TestExecutionContext constructor, you are copying the attachments from each higher level to the lower level. Is there a specific reason for that? Won't that mean the attachments are duplicated all the way down the tree of results?

ChrisMaddock · 2017-05-07T22:07:28Z

You are adding an entry to TestExecutionContext for attachments and then copying them to the result. Is there any reason you don't want to simply add them to the result directly? This would be analogous to how we add text output to the result.

I hadn't realised the TestResult was created at the start of the test - I followed where StartTime was set, which was in WorkItem.WorkItemComplete(). This sounds like a much better idea.

In the TestExecutionContext constructor, you are copying the attachments from each higher level to the lower level. Is there a specific reason for that? Won't that mean the attachments are duplicated all the way down the tree of results?

Nope - that's just me not looking into things properly. Sorry.

It's getting on a bit UK-time, will make the changes tomorrow. Thanks for those!

CharliePoole · 2017-05-07T22:33:25Z

@ChrisMaddock Great! I'll wait till you update to do a detailed review.

ChrisMaddock · 2017-05-08T20:09:38Z

src/NUnitFramework/tests/Internal/TestExecutionContextTests.cs

+            var context = new TestExecutionContext { CurrentResult = fakeResult };
+            return context;
+        }
+


CreateFakeContext() is pretty ugly. Am I missing a better way to create temporary context to test with? I didn't want to attach anything to the actual TestExecutionContext.

It's easy to create a context, harder to populate it with meaningful values. In this case, we needed a context with a valid result. The alternative is to use NSubstitute but I think you'll be doing a lot of setup if you do that.

ChrisMaddock · 2017-05-08T20:30:06Z

Initial changes made, and ready for review. Thanks Charlie.

jnm2 · 2017-05-08T21:20:16Z

src/NUnitFramework/framework/Internal/Results/TestResult.cs

@@ -268,7 +283,7 @@ internal set
                InternalAssertCount = value;
            }
        }
-
+        


I really hate to nitpick whitespace. I like these tools:
VS: https://marketplace.visualstudio.com/items?itemName=MadsKristensen.TrailingWhitespaceVisualizer
VS Code: https://marketplace.visualstudio.com/items?itemName=shardulm94.trailing-spaces with "trailing-spaces.trimOnSave": true

@jnm2, LOL, nice to see other developers as OCD as me 😄

OTOH, if VS doesn't really care...

jnm2 · 2017-05-08T21:20:41Z

src/NUnitFramework/framework/Internal/Results/TestResult.cs

+        /// </summary>
+        /// <param name="targetNode">The target node.</param>
+        /// <returns>The new attachments element.</returns>
+        private TNode AddAttatchmentsElement(TNode targetNode)


Nit: should be AddAttachmentsElement

I never could spell that word! Thanks!

Spelling is not a nitpick. Just whitespace. 😄

jnm2 · 2017-05-08T21:21:39Z

src/NUnitFramework/framework/Internal/TestAttachment.cs

+        /// <summary>
+        /// Absolute filepath to attachment file
+        /// </summary>
+        public string Filepath { get; }


I've never seen this as one word. Can it be FilePath?

I Googled, and you're right. I'll change throughout - thanks!

jnm2 · 2017-05-08T21:23:05Z

src/NUnitFramework/framework/Internal/TestAttachment.cs

+        /// </summary>
+        /// <param name="filepath">Absolute filepath to attachment file</param>
+        /// <param name="description">Optional user specifed description of attachment</param>
+        public TestAttachment(string filepath, string description)


Same; I've never seen this as one word. Can it be filePath?

jnm2 · 2017-05-08T21:23:16Z

src/NUnitFramework/framework/Internal/TestExecutionContext.cs

+        /// </summary>
+        /// <param name="filepath">Filepath to attachment. Can be absolute, or relative to working directory.</param>
+        /// <param name="description">Optional description of attachment</param>
+        public void AddTestAttachment(string filepath, string description = null)


Same; I've never seen this as one word. Can it be filePath?

jnm2 · 2017-05-08T21:24:10Z

src/NUnitFramework/tests/Internal/Results/TestResultAttachmentTests.cs

+        [SetUp]
+        public void SetUp()
+        {
+            _result = TestUtilities.Fakes.GetTestMethod(this, "FakeMethod").MakeTestResult();


nameof(FakeMethod) 😀

jnm2 · 2017-05-08T21:25:00Z

src/NUnitFramework/tests/Internal/TestExecutionContextTests.cs


        [OneTimeSetUp]
        public void OneTimeSetUp()
        {
            _fixtureContext = TestExecutionContext.CurrentContext;
            _fixtureResult = _fixtureContext.CurrentResult.ResultState;
+            _tempFilePath = Path.Combine(TestContext.CurrentContext.WorkDirectory, "NUnitTests.tmp");


Replace "NUnitTests.tmp" with TempFileName?

Thanks. I had to do a bit of shuffling here to get things playing nice with PCL - must have missed that. 🙂

jnm2 · 2017-05-08T21:26:00Z

src/NUnitFramework/tests/Internal/TestExecutionContextTests.cs

+
+        public TestExecutionContext CreateFakeContext()
+        {
+            var fakeResult = TestUtilities.Fakes.GetTestMethod(this, "FakeMethod").MakeTestResult();


nameof(FakeMethod) 😀

ChrisMaddock · 2017-05-08T21:34:14Z

Thanks @jnm2. I'll wait for comments from others, then fix everything in one go. 😄

rprouse

I am approving, but requesting changes. Looks good except for the CDATA issue.

rprouse · 2017-05-08T23:35:49Z

src/NUnitFramework/framework/Internal/Results/TestResult.cs

@@ -268,7 +283,7 @@ internal set
                InternalAssertCount = value;
            }
        }
-
+        


@jnm2, LOL, nice to see other developers as OCD as me 😄

rprouse · 2017-05-08T23:39:53Z

src/NUnitFramework/framework/Internal/Results/TestResult.cs

+                var attachmentNode = attachmentsNode.AddElement("attachment", attachment.Filepath);
+
+                if (attachment.Description != null)
+                    attachmentNode.AddAttribute("description", attachment.Description);


Description is a user supplied string that may be long or require CDATA encoding. Based on that, we should probably make it a child element rather than an attribute and CDATA encode it.

var attachmentNode = attachmentsNode.AddElement("attachment"); attachmentNode.AddElement("filename", attachment.Filepath); if (attachment.Description != null) attachmentNode.AddElementWithCDATA("description", attachment.Description);

Tests will need to be changed accordingly.

Sure.

I did consider this - the current AddAttribute goes through an ExcapeXmlCharacters() - which should sufficiently encode any user string. If length is a concern for readability however, I can change it over. Will do that with the rest. 🙂

Amazing how things grow! I didn't imagine we would have a description at all at the start but if we have one I think @rprouse is right.

rprouse · 2017-05-08T23:42:20Z

src/NUnitFramework/framework/Internal/TestExecutionContext.cs

+
+            if (!Path.IsPathRooted(filepath))
+                filepath = Path.Combine(TestContext.CurrentContext.WorkDirectory, filepath);
+


Looks good to me, although I think Path.GetFullPath() will be available once I get the .NET Standard version working.

ChrisMaddock · 2017-05-09T08:18:38Z

Thanks both. @CharliePoole - you said earlier you were planning to come back to this - so I'll hold on for your feedback, then deal with everything in one go.

CharliePoole

Not sure these are really changes... some are questions. Most details are inline but in summary...

I don't see why the TestExecutionContext or its tests need to be changed.
I'm not sure we care if the path given to us is real and what it's absolute path equivalent is. The "standard" is that we store files relative to the WorkDirectory, but we are not storing anything... it's up to the user. If we didn't care about the path, testing becomes easier.
I would have tested this using a new class in the testdata-assembly. That would eliminate all worries about test context, execution context, etc. We would simply be looking at the result and it's fields.
There is no test of TestContext
There are no new tests of TestResult, particularly to verify that it creates XML correctly.

CharliePoole · 2017-05-10T02:17:41Z

src/NUnitFramework/framework/Internal/Results/TestResult.cs

+        /// TestAttachments attached to result.
+        /// Only exposed for testing.
+        /// </summary>
+        internal ICollection<TestAttachment> TestAttachments { get; } = new List<TestAttachment>();


Working my way down... I can't see why this would not be private readonly List. Maybe it will become clear as I go. 😄

Comment says "Only exposed for testing." but readonly sounds good.

Ah! I didn't notice the comment. So this field is not the private backing field for a public property then? That seems a bit odd, since it leaves a part of the TestResult inaccessible to clients.

You're right. I had been keeping this as an 'add-only' collection, but it would be more in-keeping with the rest of the TestResult class if this was exposed to the user in a ReadOnlyCollection. I'll expose this list and the TestAttachment type.

CharliePoole · 2017-05-10T02:19:49Z

src/NUnitFramework/framework/Internal/Results/TestResult.cs

+        /// </summary>
+        /// <param name="targetNode">The target node.</param>
+        /// <returns>The new attachments element.</returns>
+        private TNode AddAttatchmentsElement(TNode targetNode)


Spelling is not a nitpick. Just whitespace. 😄

CharliePoole · 2017-05-10T02:23:04Z

src/NUnitFramework/framework/Internal/Results/TestResult.cs

+                var attachmentNode = attachmentsNode.AddElement("attachment", attachment.Filepath);
+
+                if (attachment.Description != null)
+                    attachmentNode.AddAttribute("description", attachment.Description);


Amazing how things grow! I didn't imagine we would have a description at all at the start but if we have one I think @rprouse is right.

CharliePoole · 2017-05-10T02:31:58Z

src/NUnitFramework/framework/Internal/TestExecutionContext.cs

+
+            if (!Path.IsPathRooted(filepath))
+                filepath = Path.Combine(TestContext.CurrentContext.WorkDirectory, filepath);
+


Actually Path.GetFullPath() would be incorrect. We promise to store all output files relative to our WorkDirectory, which is not the current directory. The choice of name for the WorkDirectory, may have been a bad one, since it sounds a bit like the working directory, i.e. cwd, but I believe we are stuck with it.

A more basic question... why do we want to make this absolute at all? Why not just use what the user gives us?

CharliePoole · 2017-05-10T02:33:41Z

src/NUnitFramework/framework/Internal/TestExecutionContext.cs

+
+#if !PORTABLE
+            if (!File.Exists(filepath))
+                throw new FileNotFoundException("Test attachment filepath could not be found.", filepath);


Why do we care that the file doesn't exist? Maybe the user asked to add the attachment before saving it. Maybe some third party software is expected to add it later. If we are not doing anything with the file, why not just record what the user tells us to record?

This was a point I went back and forth on. 🙂 My thinking was that adding a non-existant file is far more likely to occur through user-error, rather than intention, and you'd want to know about that ASAP. I imagine a common use case may be where people only refer to attachments when the test has failed - so broken paths may only be discovered 6 months after writing, when the test first fails, and it's too late.

The opposite arguments e.g. 'adding an attachment before saving it', mostly seem like things that could be managed by the user. (e.g. Don't add an attachment before you've created it!) That seems like fairly bad practice anyway, what if the creation of the attachment later went wrong, and the file couldn't be written? We could always add a 'don't check file existence' overload in the future if such cases arose.

Not that it matters, but MSTest checks if the File Exists. @rprouse @jnm2 - what do you guys think?

I guess checking for existence makes sense in general.

Typically, if you're going to do anything else with the file other than verify that it exists, the existence check is meaningless because it's race condition prone. To have meaningful handling you'd have to catch FileNotFoundException because it's the only atomic API.

I imagine a common use case may be where people only refer to attachments when the test has failed - so broken paths may only be discovered 6 months after writing, when the test first fails, and it's too late.

But this is a great point and would be very helpful.

CharliePoole · 2017-05-10T02:41:45Z

src/NUnitFramework/framework/TestContext.cs

+        /// <param name="description">Optional description of attachment</param>
+        public static void AddTestAttachment(string filepath, string description = null)
+        {
+            TestExecutionContext.CurrentContext.AddTestAttachment(filepath, description);


Why the extra level of indirection? In other code, TestContext just accesses TestExecutionContext.CurrentContext.CurrentResult directly.

I can change that if this ways preferred. I get a little mixed up about logic should live between TestContext and TestExecutionContext - I'd followed how AddFormatter() was written.

This is also more possible now I'm no longer storing an intermediate list of attachments on TestExecutionContext. 😄

ChrisMaddock · 2017-05-10T09:03:36Z

Thanks @CharliePoole! I've replied to the inline comments, the remaining ones...

I would have tested this using a new class in the testdata-assembly. That would eliminate all worries about test context, execution context, etc. We would simply be looking at the result and it's fields.

I'll try it. 🙂

There is no test of TestContext

No - I'll move the tests currently in TestExecutionContext to use TestContext, once I move everything out of there.

There are no new tests of TestResult, particularly to verify that it creates XML correctly.

Is this not covered by TestResultAttachmentTests?

Will hopefully make the changes this weekend - thanks all!

ChrisMaddock · 2017-05-21T13:57:29Z

Review changes made - this is ready for a second look. 🙂

Only thing to note, is that this contains if #PORTABLE symbols, so either this or #2174 will need adjusting, depending which is merged first. I don't mind making the changes here, given the size of #2174 - although that one should probably needs to wait for further review, before merging.

oznetmaster · 2017-05-22T02:13:10Z

I commented on the commit. Probably the wrong place, again :(

ChrisMaddock added is:feature pri:normal labels May 7, 2017

ChrisMaddock added this to the 3.7 milestone May 7, 2017

ChrisMaddock self-assigned this May 7, 2017

ChrisMaddock mentioned this pull request May 7, 2017

Attaching files to the test result #1670

Closed

ChrisMaddock commented May 7, 2017

View reviewed changes

ChrisMaddock force-pushed the issue-1670 branch 2 times, most recently from 850b1d0 to f550823 Compare May 8, 2017 20:06

ChrisMaddock commented May 8, 2017

View reviewed changes

ChrisMaddock force-pushed the issue-1670 branch from f550823 to c3f22d7 Compare May 8, 2017 20:15

jnm2 reviewed May 8, 2017

View reviewed changes

rprouse approved these changes May 8, 2017

View reviewed changes

CharliePoole requested changes May 10, 2017

View reviewed changes

ChrisMaddock mentioned this pull request May 18, 2017

Plan the 3.7 Release #2167

Closed

ChrisMaddock added 2 commits May 20, 2017 22:36

Add TestResult attachment functionality

178686a

Initial review changes

f94a134

ChrisMaddock force-pushed the issue-1670 branch from c3f22d7 to 14213df Compare May 21, 2017 13:39

Review changes

696a338

ChrisMaddock force-pushed the issue-1670 branch from 14213df to 696a338 Compare May 21, 2017 13:54

CharliePoole approved these changes May 21, 2017

View reviewed changes

CharliePoole merged commit bc1309e into master May 21, 2017

CharliePoole deleted the issue-1670 branch May 21, 2017 18:15


		if (!Path.IsPathRooted(filepath))
		filepath = Path.Combine(TestContext.CurrentContext.WorkDirectory, filepath);

Allow attaching files to TestResults #2152

Allow attaching files to TestResults #2152

Conversation

ChrisMaddock commented May 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisMaddock commented May 7, 2017 • edited Loading

CharliePoole commented May 7, 2017

ChrisMaddock commented May 7, 2017

CharliePoole commented May 7, 2017

ChrisMaddock commented May 7, 2017

CharliePoole commented May 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisMaddock commented May 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisMaddock commented May 8, 2017

rprouse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisMaddock commented May 9, 2017

CharliePoole left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisMaddock commented May 10, 2017 • edited Loading

ChrisMaddock commented May 21, 2017 • edited Loading

oznetmaster commented May 22, 2017

ChrisMaddock commented May 7, 2017 •

edited

Loading

ChrisMaddock commented May 10, 2017 •

edited

Loading

ChrisMaddock commented May 21, 2017 •

edited

Loading