-
Notifications
You must be signed in to change notification settings - Fork 319
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Writes the test properties, CSSProjectStructure, CSSIteration, WorkItemIds and Description to the TRX log #1801
Conversation
…nce with the vstst.xsd schema
…o written as properties
@dotMorten , will this change existing trx schema? I'll take a deeper look once I'm back on Monday. |
No this is just populating the properties already declared in the schema. I validated the output and it parses against the schema. There's nothing in there that isn't declared in the schema already |
src/Microsoft.TestPlatform.Extensions.TrxLogger/ObjectModel/TestElement.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the private field |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any tests for these changes. Can we please add.
src/Microsoft.TestPlatform.Extensions.TrxLogger/ObjectModel/TestPropertyItems.cs
Show resolved
Hide resolved
src/Microsoft.TestPlatform.Extensions.TrxLogger/ObjectModel/TestPropertyItems.cs
Outdated
Show resolved
Hide resolved
testElement.ExecutionId = new TestExecId(executionId); | ||
testElement.ParentExecutionId = new TestExecId(parentExecutionId); | ||
testElement.WorkItemIds = GetWorkItemIds(rockSteadyTestCase); | ||
|
||
foreach (var trait in rockSteadyTestCase.Traits) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Description, WorkItem, CssIteration and CssProjectStructure, are they part of the Traits collection ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are all traits - however in the TRX report they have their own specific locations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dotMorten These are specific to MSTest framework, aren't they ? If yes, can we try avoid these hardcoded checks.
Also, these have not been part of the Traits on MSTestv1, but started showing up in MSTestv2 only and we have already fixed that issue in the adapter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we try avoid these hardcoded checks
These values were already hardcoded (apart from the new ones I added). Look for instance in the GetPriority(…)
and GetOwner(…)
methods.
these have not been part of the Traits on MSTestv1, but started showing up in MSTestv2 only and we have already fixed that issue in the adapter.
I'm not sure what you're getting at here or what you want me to do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Principle we are trying to follow is TestPlatform should have minimal MSTest framework specific code.
- These properties (Description, WorkItem, CssIteration and CssProjectStructure) should not have been part of the Traits collection, that was a bug in the adapter and has been fixed.
- I really like the idea of Testproperty name-value available in the Trx. For these properties, I'd rather like them to be available as name value pairs like any other test property.
- I agree we have Priority and Owner as first class nodes, but these are legacy and have been there in older framework (backward compatibility).
- Considering Description for first class node still does make sense to me, as it can be extended to other testing frameworks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not clear on how these are MSTest specific in any way. The trait names are rather generic. However there's an existing xml schema that defines all these properties as specific non-trait fields. These aren't the only properties that are pulled out from the traits collection and saved to the TRX in their own fields. I'm following the TRX schema 1:1 here. If another test framework wants to use for instance "Description" all they have to do is report a trait as "Description".
Whether non-traits should have been stored as traits in the first place is a moot point - that ship has already sailed, and this PR merely builds on what is already there and couldn't really change without breaking anyone.
These properties (Description, WorkItem, CssIteration and CssProjectStructure) should not have been part of the Traits collection, that was a bug in the adapter and has been fixed
As I had pointed out in the PR related to that, that change was a significant breaking change that I'm assuming you'll be rolling back before the next release before you break anyone. This specific PR was done this way to avoid changing any existing behavior and follow the exact patterns that were already established, so I have a hard time understand what the problem here is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Updating the comment]
Thanks for the explanation.
My main concern was around use of Traits to get these properties.
And microsoft/testfx#482 This change is much needed as it broke the experience for multiple customers and these unwanted traits started showing up in the TestExplorer while grouping via Traits.
I know currently these properties are available only through traits and not as part of Store property (which is dictionary of testproperty and value) available in the TestCase object.
But, we can fix that in the adapter to start populating these properties.
Also, frameworks like Nunit today populate traits as TestProperty.
What I am proposing is we first fix the adapter to start populating these properties as part of the store, and not as Traits. Then consume these to populate the trx element
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have raised a PR for these test properties to be available in the TestCase object. Here is the PR: microsoft/testfx#538
It's not clear to me how you test the output of a trx report. Please advise/help |
Regarding the tests, We can add a few test cases in ConverterTests for verifying that we are able to convert to relevant properties in TestElement. |
I would really like to help to get output of traits implemented in trx. Can I create a new PR using @dotMorten's code that only outputs traits as key/value elements as he proposed, and thus skipping special handling of Description, WorkItem, CssIteration and CssProjectStructure? I will add some tests as mentioned in @singhsarab's last comment. |
@agehrke I am sure @dotMorten would like that. |
This part of the resulting XML seems to be wrong/other than former MSTest.exe created it. VS2019 is not able to show the ...
<WorkItemIDs>
<WorkItem id="1234" />
</WorkItemIDs>
... Expected: ...
<Workitems>
<Workitem>1234</Workitem>
</Workitems>
... |
Apart from my former comment, what's the status of this PR? We look forward to get support for the work items in trx file. |
Description
Writes the description, CSS*, test properties and workitem ids to the TRX log in accordance with the vstst.xsd schema
Related issue
The missing traits were discussed in this issue: #98
This PR addressing the lack of CssProjectStructure, CssIteration, Description, Properties/traits and WorkItemIds. The generated TRX has been validated against the schema.
Here's an example of TRX output from a test with 4 properties, where 3 of them have special places to go in the TRX:
TRX: