Skip to content

TeamCity service message should have assembly name as a part of test name. See #669#671

Closed
NikolayPianikov wants to merge 12 commits intonunit:masterfrom
NikolayPianikov:TeamCityTestNameWithAssembly
Closed

TeamCity service message should have assembly name as a part of test name. See #669#671
NikolayPianikov wants to merge 12 commits intonunit:masterfrom
NikolayPianikov:TeamCityTestNameWithAssembly

Conversation

@NikolayPianikov
Copy link
Copy Markdown
Member

Fixes #669

@rprouse
Copy link
Copy Markdown
Member

rprouse commented Jun 1, 2015

@chris-smith-zocdoc, could you give me a second code review on this pull request? You are much more familiar with Team City than I am, so I would appreciate your input. Thanks.

@chris-smith-zocdoc
Copy link
Copy Markdown
Contributor

@rprouse Sure.

@chris-smith-zocdoc
Copy link
Copy Markdown
Contributor

You can see the results of this working in the new TC setup. https://teamcity.jetbrains.com/viewLog.html?buildId=491864&buildTypeId=NUnit_NUnit3_BuildAndTest&tab=testsInfo

image

@chris-smith-zocdoc
Copy link
Copy Markdown
Contributor

@NikolayPianikov TC is showing 109 new tests, are all of these because of the duplicate name grouping? If I switch the test view to "tests without grouping by name" there are 10 new tests which are the ones you introduced in this PR.

image

@chris-smith-zocdoc
Copy link
Copy Markdown
Contributor

This looks good to me

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit of code duplication here. Remove this one and then make GetTestAssembly on TestResult internal.

@rprouse
Copy link
Copy Markdown
Member

rprouse commented Jun 1, 2015

Looks good to me too. @NikolayPianikov, can you make the one requested change? I will merge when you are done.

…t of test name - remove code dup, fix tests ResultReporterTests
…t of test name - add tests to TestUtilityTests
@NikolayPianikov
Copy link
Copy Markdown
Member Author

@chris-smith-zocdoc, the number of tests increased because we added prefix to test's name - name of assembly. Now we believe tests in different assembly as different tests and not just as the test's double running. You could compare:
image
and
image

But when we have similar names of assemblies, it doesn't work as we would like

@CharliePoole
Copy link
Copy Markdown
Member

Note that the Engine tests should not actually be part of the .NET 4.5 run, however.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to avoid adding this attribute to the XML if possible. Every test is eventually nested under a suite of type "Assembly", which has the full name of the assembly. Putting it on every test is an extreme level of duplication in the result file for all users, even though it is only used for TeamCity.

Also, every test has an id, the first part of which (before the hyphen) identifies the assembly uniquely. It could be considered to use this as a part of the name, perhaps as a prefix, when creating TC messages.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I will fix

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've created new PR #684

@CharliePoole
Copy link
Copy Markdown
Member

I added a line note because I was surprised to see that this PR is changing the framework itself. We anticipated - or at least I did - that it was only a small local change to the Console runner. If we are going to change the output of the framework, we want to give it a careful review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TeamCity service message should have assembly name as a part of test name.

4 participants