Runtime: Improve Unit Testing experience in CoreFX

[willdean]

I don't want this to be a mass hate-in on xUnit, but I made some unguarded comments about xUnit on another CoreFx thread, and there does seem to be some shared feeling on the subject. @karelz suggested (#4571) in a separate thread to discuss it. If the consensus is 'shove off to the xUnit repo and do your moaning there', then that's fine, though I think my complaints are perceived by some to be positive features of xUnit, so I doubt I'd get any traction on my own.

The xUnit issues I have found made testing harder than it needed to be on CoreFx are:

• Inability to consistently add messages to assertion methods - you can add a message to Assert.True, but you can't to Assert.Equal. Two workaround recommendations seem to get made: The first is to use Assert.True and put your condition in the bool argument - that then means the test runner can put no useful info into the trace. The other alternative is to use a better assertion library (there was some consistency of support for Fluent Assertions, though I haven't used it myself, and my heart sinks at yet another dependency). Every assertion method should have an optional message. People planning to advance the defence at this point that there should only be one assertion in a test and so you shouldn't need messages can expect short shrift from me...

• Obvious gaps in the assertion support - e.g. no 'Fail' method, so we end up with stuff like Assert.True(false, "Message") or writing a helpers to avoid this sort of thing. This would be easy to add.

• No proper trace from tests. A .NET test framework/runner should collect Console and Trace output and report/log it sensibly (i.e. not co-mingled with other test output). No ifs, no buts, no excuses about async, nothing. This is the one that really winds me up, because it leads to CoreFx having to have a policy against tracing in tests largely because the test outputs gets sprayed across each other so they make no sense. Couple this with the pain some of us have with trying to debug into tests, and the result is miserable - CI post-mortem shouldn't have to be a guessing game.

I'm aware that xUnit is somehow the MS-insider's choice, and it was probably the only framework with Core support at the time it was chosen, so we cannot rewrite history, but if the Core projects have any influence with xUnit, could they ask for these improvements?

[danmosemsft]

Some of the extensions we already have, below, could be potentially rolled into XUnit if they're generally interesting.
https://github.com/dotnet/corefx/blob/master/src/Common/tests/System/AssertExtensions.cs
https://github.com/dotnet/buildtools/tree/master/src/xunit.netcore.extensions

+1 for having a good way to log that goes into the xunit log and not merely undifferentiated console output. ITestOutputHelper may be it, but I remember there are some issues with it - tests have to be instance, and some issue with it not appearing (?). Why not just have Assert.Log ?

As well as having messages in all asserts, I would like the built in messages to be improved. For example Assert.Equals will only log a small fragment of the strings around the point they diverge.

@stephentoub do you have others to add to the list above.

[danmosemsft]

@AtsushiKan posted https://github.com/dotnet/buildtools/issues/1538 extracted here:

Ask 1: xunit.console.netcore.exe should support response files (including multi-line response files.)
Perfect for when you have a list of methods to focus on – “sed “s/^/-method /g” > foo.rsp; xunit.console.netcore.exe @foo.rsp

Ask 2: xunit.console.netcore.exe should automatically read from a default response file (xunit.console.netcore.rsp next to the .exe) RunTests.cmd (or whatever) should create xunit.console.netcore.rsp and invoke “xunit.console.netcore.exe” without the 100+ character command line it does today.

Ask 3: Human-friendly command line switches for browsing and selecting tests.

[danmosemsft]

Nit: from command line, I have often wanted to pass a particular pattern to match test methods to execute. Right now I can do -method The.Full.Namespace.Class.Method but it would be so much easier to do -pattern *foo*. I used to do this on my old team with a different harness.

[danmosemsft]

We should make it easier to run xunit directly rather than msbuild /t:test. Just to speed things up when I want to. Right now I have to copy and paste bits I see in the build log, and open up a file named like C:\git\corefx\bin/Windows_NT.AnyCPU.Debug/System.Diagnostics.Process.Tests/netstandard//RunTests.cmd to extract other bits, then do something like

pushd C:\git\corefx\bin\Windows_NT.AnyCPU.Debug\System.Diagnostics.Process.Tests\netstandard
C:\git\corefx\bin\testhost\netcoreapp-Windows_NT-Debug-x64\dotnet.exe xunit.console.netcore.exe  System.Diagnostics.Process.Tests.dll  -xml testResults.xml -notrait Benchmark=true -notrait category=nonnetcoreapptests -notrait category=nonwindowstests  -notrait category=OuterLoop -notrait category=failing

[ghost]

Regarding xunit output, I opened an issue about this on coreeng but since not everyone can see that,

I want all notifications about skipped tests to go away unless I specifically request them. Skipped tests are already known be tracked by issues or not relevant to the platform you're running. They are uninteresting by definition.

[ghost]

Another note regarding xunit output: I would prefer it if the "all tests pass" output does not include the substring "Failed: 0".

Think about the poor sod who's reading through a 5000 line log output from some remote job running 200 test projects and trying to grep for "Failed:".

[willdean]

@danmosemsft I guess if Console/Trace really can't be made to work properly (NCrunch seems to cope OK running nUnit tests in parallel, but perhaps their units of parallelisation/asynchrony are different), then some other test-specific tracing methods ("Assert.Log") would be better than nothing.

Every other .NET test runner I've used in the last umpteen years has been able to collect up all the trace from tests and structure it appropriately - personally I think the design decision in xUnit to forgo that in exchange for overlapping tests was a bad one, if it really was a hard either/or decision.

[danmosemsft]

@willdean Presumably XUnit could wrap Console.Out and differentiate output from the test threads themselves but if tests spawn threads I don't think it could tell which one was responsible for the thread pool thread doing Console.Log. The only fix I can think of is to spawn child processes to get parallelism. Which would be fine as long as it's easy to switch off and/or debug a particular test.

[karelz]

paging in xUnit maintainers for insights, ideas, options, etc.: @onovotny @bradwilson

More contributors to xUni I know: @hughbe @citizenmatt @akoeplinger

As was said above: If we need to move (parts of) the discussion into xunit repo, that would be fine. Let's decide based on the discussion here.

[safern]

Assert.All only works for generic collections. In example we can do an Assert.All to IDictionary<TKey, TValue> but not to non-generic IDictionary. So we have some cases like this one: https://github.com/safern/corefx/blob/4775884841d3e47d658adedeb56e033e01d30d3a/src/Common/tests/System/Collections/IDictionary.NonGeneric.Tests.netcoreapp.cs#L22-L35

where we can't just do an Assert.All and we have to do a foreach loop to do the testing.

[tmat]

We use test helpers in Roslyn as well as other repos we own.
The main helpers there are Asserts that compare long strings (e.g. IL, XML), sequences (byte[], etc.) displaying diffs when the comparison fails.

[mconnew]

One issue that isn't caused by xunit itself but by the build tools created for corefx is the inability to use the test explorer in VS. There seems to be some confusion around this too. In the windows specific build instructions it says:

Running tests from using the VS test explorer does not currently work after we switched to running on CoreCLR. We will be working on enabling full VS test integration but we don't have an ETA yet.

So the documentation is clearly saying the intention of issue dotnet/runtime#14412 is to enable VS test explorer functionality and states this will be worked on at some point in the future but hasn't been planned for yet. @weshaggard commented this early in the bug:

We currently do not have support for hosting and running xunit within VS. That actually requires a fair amount of work but we can use this issue to track that work.

But bug dotnet/runtime#14412 was closed by @mellinoe with the following comment:

Enabling the test explorer window is a large piece of work that should be tackled as its own effort.

So I'm confused about the status? Is this still planned? If so, why was the bug used to track this closed? If it has been decided that this isn't going to happen, why do the docs still say it will?

[willdean]

@mconnew Much blood is being let on pretty much exactly that subject in dotnet/runtime#22080 - as you say, it's not exclusively an xUnit issue

[mconnew]

For the logging of tests, the WCF team have implemented our own FactAttribue, FactDiscoverer, IXunitTestCase and test discoverer classes to significantly improve the logging. WCF internally has it's own ETW event source. We instantiate an event listener and register a callback to save the events. We then run the test and if the test fails, we output the events which happened during the test run to Console.WriteLine which xunit saves to the logs. This keeps our logs empty when everything passes so keeps log storage size low, and allows us to have very verbose logging if a test fails. You can find all the code here, the actual meat of the functionality is here. Something similar could be done more generically with a logger made available to the individual tests instead of relying on product code using an EventSource. Our EventSource method requires only one test executing at once, but a logger being used in test code doesn't need to have that limitation.

[bradwilson]

• Inability to consistently add messages to assertion methods - you can add a message to Assert.True, but you can't to Assert.Equal. Two workaround recommendations seem to get made: The first is to use Assert.True and put your condition in the bool argument - that then means the test runner can put no useful info into the trace. The other alternative is to use a better assertion library (there was some consistency of support for Fluent Assertions, though I haven't used it myself, and my heart sinks at yet another dependency). Every assertion method should have an optional message. People planning to advance the defence at this point that there should only be one assertion in a test and so you shouldn't need messages can expect short shrift from me...

• Obvious gaps in the assertion support - e.g. no 'Fail' method, so we end up with stuff like Assert.True(false, "Message") or writing a helpers to avoid this sort of thing. This would be easy to add.

These are both addressed by either using a third party assertion library, or using our source-based assertion library (as a package or a Git submodule) and making the changes you want.

We have no intention to revisit either of these decisions.

• No proper trace from tests. A .NET test framework/runner should collect Console and Trace output and report/log it sensibly (i.e. not co-mingled with other test output). No ifs, no buts, no excuses about async, nothing. This is the one that really winds me up, because it leads to CoreFx having to have a policy against tracing in tests largely because the test outputs gets sprayed across each other so they make no sense. Couple this with the pain some of us have with trying to debug into tests, and the result is miserable - CI post-mortem shouldn't have to be a guessing game.

We made this decision when going to 2.0 to stop capturing things that didn't belong to us (we captured console, trace and debug in 1.x). This was not just the matter of understanding which unit test happened to be writing to the console (when there could be hundreds of them running at once), but also because it's just bad hygiene. When we take over these things, it makes testing things which use and manipulate them that much more difficult.

We are also not planning to revisit this decision.

[bradwilson]

Ask 1: xunit.console.netcore.exe should support response files (including multi-line response files.)
Perfect for when you have a list of methods to focus on – “sed “s/^/-method /g” > foo.rsp; xunit.console.netcore.exe @foo.rsp

Ask 2: xunit.console.netcore.exe should automatically read from a default response file (xunit.console.netcore.rsp next to the .exe) RunTests.cmd (or whatever) should create xunit.console.netcore.rsp and invoke “xunit.console.netcore.exe” without the 100+ character command line it does today.

xunit.console.netcore.exe is not something that comes from the xUnit.net team.

Ask 3: Human-friendly command line switches for browsing and selecting tests.

Feel free to open issues (and after discussion on design, PRs) for command line switches you'd like to see added. Note that this will only affect our command line runners, not xunit.console.netcore.exe, since that's not our code.

[bradwilson]

The main helpers there are Asserts that compare long strings (e.g. IL, XML), sequences (byte[], etc.) displaying diffs when the comparison fails.

If you feel these would be of general interest to the xUnit.net community, please open issues and PRs for them.

[bradwilson]

One issue that isn't caused by xunit itself but by the build tools created for corefx is the inability to use the test explorer in VS.

Our runner (xunit.runner.visualstudio) does indeed support Visual Studio for .NET Core. Docs: https://xunit.github.io/docs/getting-started-dotnet-core.html#run-tests-visualstudio

[danmosemsft]

These are both addressed by either using a third party assertion library, or using our source-based assertion library (as a package or a Git submodule) and making the changes you want.
We have no intention to revisit either of these decisions.

Why's that @bradwilson ? Sounds like folks are just asking for some useful overloads.

[bradwilson]

Regarding messages, two reasons:

1. New APIs are not free.
2. We believe that assertions should already be able to tell you what's wrong. If your assertion doesn't tell you what's wrong, then you should probably write a custom assertion, not use messages.

We left True & False with messages as the sole way out for when assertions could not be self-describing, but the developer was unwilling to write a custom assertion.

As for new assertions, feel free to open issues and start a discussion. We've already rejected Fail for the same reason that we reject the notion that assertions should generally have messages (after all, Assert.Fail is a cop-out to writing the correct assertion).

[hughbe]

(after all, Assert.Fail is a cop-out to writing the correct assertion

From my perspective, and I've written a lot of tests, there are situations where things go wrong and we need an Assert.True(false). Just look across the corefx codebase for examples.
It's all well and good trying to create a pure and perfect library with an ideology that shifts responsibility onto the consumer, but that's an unrealistic expectation IMO, as you can see from the complaints in this thread

[danmosemsft]

There are maybe 50K CoreFX test cases and with all the jobs and distros and configurations and branches with CI and full runs each runs hundreds of times a day on lab boxes alone. When there's a random failure I'm unlikely to be familiar with the code and sometimes there aren't symbols. Tests run concurrently and there's several different logs. So I tend to have a pragmatic bias towards making the logging as helpful as possible. It sounds like we have a philosophical difference that's unlikely to be bridged. It is easy for us to replace Assert though as @bradwilson points out.

[bradwilson]

I reject any claim that features must exist in xUnit.net simply because someone wants it.

We have unapologetically declared xUnit.net to be very opinionated software. We designed it to be focused narrowly at developer-written and -maintained unit tests. We drew from our experience of many years and many tends of thousands of unit tests written to infuse our personal opinion of best practices into the framework. That includes things like "custom assertion messages indicate a failure to find the right assertion", that's from hard fought experience. Our ongoing experience has not convinced us that we were wrong.

We offer extensibility points -- significantly more than any other test framework -- precisely because we intend people to be able to mold the framework in the way they feel matches their opinions rather than ours. It is not unreasonable to say "We aren't interested in doing this, so here's the way you can do it yourself". It's also not unreasonable to say "We prefer another framework over yours, because they're willing to implement things you aren't".

[bradwilson]

BTW, it's worth noting (something @hughbe knows well first hand) that we have added new assertions and new overloads over the 10 year life of xUnit.net. I'm not trying to say we will never add anything; I'm saying: open individual issues in xUnit.net so it can be discussed in the appropriate forum.

[hughbe]

That's a fair point. There's nothing stopping us from adding our own extensibility points to corefx. I like xunit's opinionated style compared to other test libraries - but by the definition of "opinionated" not everything can make everyone happy. I admire how adamantly you're saying no.

I'm interested also in the limitations of xunit's parallel test implementation. This is something that caused many problems (test hangs) for HttpListener tests which use Async code:
See https://github.com/dotnet/corefx/pull/20189
See https://github.com/dotnet/corefx/issues/20103
But this problem could be due to the tests rather than xunit.

[FransBouma]

@danmosemsft

Nit: from command line, I have often wanted to pass a particular pattern to match test methods to execute. Right now I can do -method The.Full.Namespace.Class.Method but it would be so much easier to do -pattern foo. I used to do this on my old team with a different harnes

You can with the trait attribute, where you specify the string which is specified as an argument when running the tests. This is one of the things which I see as a cumbersome system: if you want to test some group of methods, you first have to add the traits, then go to the comand line and specify them. Want to test another group? Add other traits, and potentially remove the ones you added previously.

I get that there is machinery in place to accomplish what a user want (using traits), so why bother adding a more userfriendly experience as an alternative, but that's academic: xunit is a tool and should be as easy to use as possible. If there are 2 alternatives to do ABC, one being overly verbose and cumbersome but 'pure' and another easy and simple, I don't see why that's the problem. Especially the default path should be as simple as possible.

[willdean]

We have no intention to revisit either of these decisions.

We are also not planning to revisit this decision.

And we're done...

[bradwilson]

@hughbe:

I'm interested also in the limitations of xunit's parallel test implementation.

@stephentoub's assessment of the situation is correct: the deadlock that you're experiencing is because of our sync context which limits tests in flight. Any code which is susceptible to deadlocks in the face of sync contexts (like calling .Wait or .Result on an incomplete Task) will, sooner or later, cause you to deadlock. If you want to track these down, you should enable long running test notification, and then set the maximum number of threads to 1. These tests should immediately deadlock themselves. (We run our own tests in CI with max threads set to 1 precisely to ensure that we have no internal deadlocks.)

As an alternative to fixing the broken tests, you can also ask for "unlimited" threads, which doesn't put the sync context into play at all. Your tests will run against the default scheduler for tasks, the one that uses the .NET thread pool, and thus your deadlocks should disappear (as the thread pool is allowed to grow as needed to accommodate the deadlocked threads).

(Note: there is a second sync context that is always in play which we use to allow async void unit tests, but this is unaffected by any usage of .ConfigureAwait and does not have any thread affinity.)

[stephentoub]

you should enable long running test notification

@bradwilson, how does someone do this? I was actually planning on adding such a thing as I couldn't find one. Must have just missed it.

[clairernovotny]

@stephentoub we added that feature in xUnit 2.2 you need to turn it on like this:
https://github.com/Reactive-Extensions/Rx.NET/blob/develop/Rx.NET/Source/tests/Tests.System.Reactive/xunit.runner.json#L2-L4

I forget ATM if the diagnosticMessages settings is also required to see it...it might be.

[willdean]

Is this reopened to track the enabling of the long running test notification in the corefx build process? If so, would it be worth changing the title?

[mconnew]

@willdean, I don't understand why this issue was closed. Just because the xunit team said they won't make changes in 2 specific areas doesn't mean this issue is resolved as nothing to see here. I take "xunit improvements" to also include extensions to xunit in the corefx build tools. For example, the corefx build tools could add the Assert.Fail() assert if it's something the community of developers working on corefx would like to see. Xunit is very extensible so there's very little that can't be done by corefx to get the behavior from xunit that we want. If there's anything we strongly feel should be a part of xunit directly, then that discussion can be had to move that functionality to xunit but that isn't a blocker to have that functionality available to corefx.

[hughbe]

from @AtsushiKan

Think about the poor sod who's reading through a 5000 line log output from some remote job running 200 test projects and trying to grep for "Failed:".

Was just re-reading this thread. I couldn't agree more. I've tried Ctrl-F-ing for "Failed: " only to find this is useless as we get hundreds of hits. Instead I go: "Failed: 1".... "Failed: 2".... but that's grim
I'm not sure if xunit will take this - this didn't get a response from @bradwilson and co. I say this as I don't know how many separate test projects the average library has.

[willdean]

@mconnew I closed it (I opened it, too) because it seems that the concerns divide as follows:

• Things in xUnit which aren't going to be fixed
• Things which turn out to be not in xUnit, so don't belong in an xUnit-improvement issue

(Of course xUnit-improvement issues don't belong in corefx either, as I acknowledged when I opened it)

There wasn't intended to be any rage-quitting tone to it - I just recognise a dead-end when I see one.

@danmosemsft 's reopening is worth a hundred of my closings, so you're safe to carry on now.

[bradwilson]

I've tried Ctrl-F-ing for "Failed: " only to find this is useless as we get hundreds of hits.

Assuming you're speaking if the post-it footer, we could deal with this via a new reporter (we could call it something like "minimal"). Open an issue and let's discuss what output you'd want for it.

[danmosemsft]

I made the title more general since it seems like there's a mixture of xunit suggestions and stuff we can do.

To lead this thread towards some productive improvements -- can folks prioritize what the, say, top 3-5 most important things to fix are? Some would be on us eg better unit test projects for VS use, some may be suggestions for Xunit (we can do PR's if they agree)

[danmosemsft]

Throwing another idea in the pile. @wfurt suggested it would be useful to have an attribute one could adorn a method with [AfterTestFailureAttribute] and it would be called each time a test failed and could gather various possibly helpful diagnostic info. Eg., for networking tests certain system logs and configuration data. I can't find anything in XUnit to do this - I see [BeforeAfterTestAttribute] but it runs on success, apparently.

[danmosemsft]

To keep this discussion going I'm going to take a shot at putting suggestions in descending order of importance:

1. Make test explorer work for CoreFX tests (are there any other VS integration issues?)
2. Add Assert overloads that have messages (apparently this will have to be in our own repo)
3. Reduce xunit output noise - skipped, failed 0 (filter perhaps in our wrapper exe or contribute a new "reporter" to xunit as suggested above)
4. Easy flag to filter tests (either in xunit or in xunit.console.netcore)
5. Response file support (either in xunit or in xunit.console.netcore)
6. Easier to log outside of asserts (make it easier to use ITestOutputHelper perhaps by making all test classes non static and derive from a helper base class or reusing what WCF did or something else)
7. Experiment with configuring "long running test" flag to find hanging tests
8. Add new assert flavors (probably reuse what Roslyn did)
9. Attribute to indicate callback after test failure

Generally if a change would be accepted into XUnit we should do it there in preference.

Does this seem like a reasonable rough prioritization?

[hughbe]

Another thing that we need is the ability to skip an entire test class. An example is skipping System.Drawing.Icon tests on Windows Nano

[davidsh]

Another thing that we need is the ability to skip an entire test class.

ActiveIssue or SkipOnTargetFramework attributes can be applied already to a test class instead of a test method. cc: @safern

[safern]

ActiveIssue or SkipOnTargetFramework attributes can be applied already to a test class instead of a test method. cc: @safern

Yup, the attributes' usage is documented in: https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/developer-guide.md

[hughbe]

But this doesn't apply to Nano right? Nano support is not a target framework or an active issue. It just doesn't work on Nano if I understand.

[safern]

Nano is not a TargetFramework so it would be more like a TestPlatform, so I really think we would need to add Nano to the TestPlatforms and then modify the PlatformSpecificDiscoverer to support skipping that platform. I could take care over the weekend :)

PlatformSpecificAttribute is already supported for test class

[hughbe]

@safern you're awesome! No pressure, but shall I hold off on dotnet/corefx#20811 until this?

[weshaggard]

Nano is not a TargetFramework so it would be more like a TestPlatform,

We need to be careful here because we might be able to get away with marking tests to be excluded on Nano, but we don't have a story in our packages to state that we don't support these on Nano. There is no target framework or runtime specific to Nano so we might want to consider actually doing something in the code to detect and fallback to something else, if possible.

[danmosemsft]

@weshaggard put another way, you mean "we should have some tests that verify that Nano product behavior is reasonable". I agree, but that may be doable with just a few Nano specific tests.

[wtgodbe]

@ViktorHofer is this still actionable?

[ViktorHofer]

Let me comment on the list created by @danmosemsft:

1. Make test explorer work for CoreFX tests - https://github.com/dotnet/corefx/issues/34314
2. Add Assert overloads that have messages - I don't think we should introduce a custom assert library just for this.
3. Reduce xunit output noise - skipped, failed 0 (filter perhaps in our wrapper exe or contribute a new "reporter" to xunit as suggested above). - We removed the logo and other unnecessary configurable output details. Also for CI it makes sense to have these details in the log.
4. Easy flag to filter tests (either in xunit or in xunit.console.netcore) - we have /p:xunitoptions="-method ... -class ..." for that.
5. Response file support (either in xunit or in xunit.console.netcore) - don't see a necessity atm.
6. Easier to log outside of asserts (make it easier to use ITestOutputHelper perhaps by making all test classes non static and derive from a helper base class or reusing what WCF did or something else) - unsure what we can do here?
7. Experiment with configuring "long running test" flag to find hanging tests - enabled in corefx. can be configured here: https://github.com/dotnet/arcade/blob/master/src/Microsoft.DotNet.CoreFxTesting/build/assets/xunit.runner.json#L3 and by setting /p:XUnitShowProgress=true you get additional details.
8. Attribute to indicate callback after test failure - don't see a necessity atm.

[wtgodbe]

That's great! It sounds like there isn't anything else pressing here, so I'll close this.