Skip to content
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

Build 2015 (vNext) fails when a test is ignored using [Ignore("No reason given")] #92

Open
jessehouwing opened this issue Dec 18, 2015 · 17 comments
Milestone

Comments

@jessehouwing
Copy link

This is with the latest NUnit 2.6 version and the latest NUnit runner (2.0), both fetched from Nuget.

All projects are .NET 4.0

2015-12-18_17-31-29

@OsirisTerje OsirisTerje self-assigned this Dec 18, 2015
@OsirisTerje
Copy link
Member

I'll set up a repro

@OsirisTerje
Copy link
Member

What is happening here is that the test(s) with Ignore are in fact being skipped, but the Ignore statement including a reason text causes this message to be output in addition. This happens only on the TFS Build vNext, not on the old Xaml build and not on standalone VS.
I can't see that the adapter is adding this message explicitly either, so it might come from NUnit itself.
@CharliePoole : Does NUnit itself add the reason text to some outputs ?

@OsirisTerje
Copy link
Member

This should be a warning text in vNext, as it is in Visual Studio. The issue has been reported to the MS PG, and we'll close it here, as we can't do anything.

@CharliePoole
Copy link
Contributor

@OsirisTerje How about something like vs-followup as a label (or other words that make sense to you) That would eliminate strangeness (to me) of having both a closed: and a status: on the same issue.

@jessehouwing
Copy link
Author

In this case it may make sense to link it to an issue in the
vso-agent-tasks project...
On Dec 21, 2015 20:00, "CharliePoole" [email protected] wrote:

@OsirisTerje https://github.com/OsirisTerje How about something like
vs-followup as a label (or other words that make sense to you) That would
eliminate strangeness (to me) of having both a closed: and a status: on the
same issue.


Reply to this email directly or view it on GitHub
#92 (comment)
.

@prawalagarwal
Copy link

Whatever is shown on the console in VNext builds is the output coming out from the vstest exe (the same executable which runs test for VS IDE too), which in turn is showing the output from nunit adapter. Here the ignore message comes out in the stderr stream from the nunit adapter and hence the red color. Running from CLI also prints the message but CLI is not color coding the stderr. It just color codes exit message if the exit code is non 0 from the adapter.

VS IDE handles the stderr stream and shows it as a warning message.

@jessehouwing
Copy link
Author

Since this is informational text, should it not be pushed to the StdOut instead of StdErr?

@prawalagarwal
Copy link

Yes it should be pushed to stdout. Will be interesting to find out why nunit adapter was implemented this way.

@OsirisTerje
Copy link
Member

The test adapter just converts whatever comes from NUnit itself, the test outcome from an Ignored test is TestOutcome.Skipped. Then the message is added to the Result.ErrorMessage property, there is no other message properties. The convertion to stderr and stdout is not in the adapter. There is a collection of TestResultMessages which is not being set. The purpose of this collection is not known to us. Is this something that should be used, and if so, where does this information end up in the test explorer ?

@OsirisTerje
Copy link
Member

@OsirisTerje
Copy link
Member

I see that @cklutz has a fork where he has used the message collection for that purpose, separating between stdout and stderr. I assume that is the way to go. It would be nice if he could add a PR to get that code back into the project.
Using the collection separates between the types, but what is then the ErrorMessage property used for ?

@OsirisTerje OsirisTerje reopened this Jan 19, 2016
@OsirisTerje OsirisTerje added this to the 2.2 milestone Jan 19, 2016
@CharliePoole
Copy link
Contributor

The adapter does not write to stderr or stdout. A quick look at the code will show that and it's obvious that it can't since it's not a console program. What the adapter does is provide TestResult objects to the controlling program - vstest in this case - which then decides what to do with it.

I think what @prawalagarwal actually means is that messages can be provided as a part of the result output using categories, two of which use StdErr and StdOut in their names. AFAIK this is undocumented stuff - at least it was undocumented when I wrote the adapter.

That's why we originally marked this as not fixable by us. It looks as if @cklutz has leveraged the messages collection of the TestResult to provide a better result. We should do the same, after investigating where the various messages show up in all three of the environments we use.

[Note that the adapter was written for use in the IDE. Features that emphasize TFS or the VS console runner were tacked on later.]

@OsirisTerje Personally, I would keep this issue closed. If we want to completely change how we handle messages, it's a new feature and we should track it separately.

@CharliePoole
Copy link
Contributor

@cklutz I sent you an invitation to become a contributor to NUnit. It looks like you have already done several things in your fork, which are outstanding issues in this adapter and/or the v3 adapter. I hope you'll join us.

@cklutz
Copy link

cklutz commented Jan 20, 2016

@CharliePoole Thanks! I did. Although no promises about my (current) availability ;-)

@OsirisTerje
Copy link
Member

@cklutz Any chance for you to move the relevant code up to a branch here - so we can have a go at it ? If you can get it up, I can start do the relevant testing on it. Would also be great to have your email, mine is up in my profile.
@CharliePoole : I'll create a feature issue to track that work, but would like to keep this bug open until fixed. I'll also remove the MS followup since it seems we can handle it.

@CharliePoole
Copy link
Contributor

@cklutz If you are willing to take this issue on, I'll assign it to you. You said you don't have much time, but I notice you already have the code in your fork. We could copy it, but I think it will be better done if you do it yourself.

@CharliePoole
Copy link
Contributor

@cklutz Whoops! I posted on the wrong issue - reposted the comment on #97!

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

No branches or pull requests

5 participants