Failing Unit Tests in nunit.framework.tests After New Clone

[aolszowka]

I just grabbed a fresh clone of nunit and popped open the solution in Visual Studio 2017 (Community 15.7.4) with the latest NUnit 3 Test Adapter (3.10 with Run Tests in Parallel) running on Windows 10 Professional and selected The "Debug-AnyCPU" and hit build.

After reading the BUILDING.md it seems like I should be ignoring any failures that don't come from nunit.framework.tests-* and nunitlite.tests-*

However out of the gate I get 2 failures from nunit.framework.tests-*

First Error:

Test Name:	TestCaseSourceCanAccessWorkDirectory("C:\\Users\\ace.olszowka\\source\\nunit\\bin\\Debug\\net20")
Test FullName:	NUnit.Framework.TestContextTests.TestCaseSourceCanAccessWorkDirectory("C:\\Users\\ace.olszowka\\source\\nunit\\bin\\Debug\\net20")
Test Source:	C:\Users\ace.olszowka\source\nunit\src\NUnitFramework\tests\TestContextTests.cs : line 110
Test Outcome:	Failed
Test Duration:	0:00:00.001

Result StackTrace:	at NUnit.Framework.TestContextTests.TestCaseSourceCanAccessWorkDirectory(String workDirectory) in C:\Users\ace.olszowka\source\nunit\src\NUnitFramework\tests\TestContextTests.cs:line 112
Result Message:	
Expected string length 34 but was 50. Strings differ at index 34.
  Expected: "C:\\Users\\ace.olszowka\\source\\nunit"
  But was:  "C:\\Users\\ace.olszowka\\source\\nunit\\bin\\Debug\\net20"
  -------------------------------------------------^

Looking at the source I don't see how this was possible, as far as I can tell these values should have been identical (_workDirectory and the test data are both set to TestContext.CurrentContext.WorkDirectory) my only guess is some type of race condition, maybe due to bad configuration on my end?

Second Error:

Test Name:	StackTracesAreFiltered("WarningInBeginInvoke",4)
Test FullName:	NUnit.Framework.Assertions.WarningTests.StackTracesAreFiltered("WarningInBeginInvoke",4)
Test Source:	C:\Users\ace.olszowka\source\nunit\src\NUnitFramework\tests\Assertions\WarningTests.cs : line 292
Test Outcome:	Failed
Test Duration:	0:00:00.004

Result StackTrace:	at NUnit.Framework.Assertions.WarningTests.StackTracesAreFiltered(String methodName, Int32 maxLineCount) in C:\Users\ace.olszowka\source\nunit\src\NUnitFramework\tests\Assertions\WarningTests.cs:line 310
Result Message:	
Multiple failures or warnings in test:
  1) (Warning message)
  2) Expected the number of lines to be no more than 4, but it was 5:

 1. at NUnit.TestData.WarningFixture.<>c__DisplayClass45_0.<WarningInBeginInvoke>b__0()
 2. at System.Runtime.Remoting.Messaging.StackBuilderSink._PrivateProcessMessage(IntPtr md, Object[] args, Object server, Object[]& outArgs)
 3. at System.Runtime.Remoting.Messaging.StackBuilderSink.AsyncProcessMessage(IMessage msg, IMessageSink replySink)
 4. at System.Runtime.Remoting.Proxies.AgileAsyncWorkerItem.ThreadPoolCallBack(Object o)
 5. at System.Threading.QueueUserWorkItemCallback.WaitCallback_Context(Object state)
(end)

I get lost in what this test is attempting to do here; but based on my limited understanding it seems like this code is really sensitive towards any changes in the call stack, could this be due to a recent change in the .NET Framework?

I also get every single test case failing in NUnitLite.Tests.CommandLineTests I would be happy to dig on there if this is unexpected.

Seeing as the build is passing in CI these are probably just configuration issues on my end, but nothing was mentioned in BUILDING.md about this so figured a report was worth it.

[jnm2]

Thanks for the report! I've confirmed that TestCaseSourceCanAccessWorkDirectory and CommandLineTests don't work in Test Explorer. We need to come up with a way to make them independent of runner and parallelism. @nunit/framework-team Any ideas?

For the second error, that's a smell test. It makes sense to bump that to 5.

[CharliePoole]

In the past, I just told people to not use the test explorer for the NUnit tests. Other team members have pushed back against that saying that test explorer has become, for many people, the default way to run tests.

There is certainly ample precedent for a particular runner not being useful when testing a testing framework. Testing frameworks under test are a pretty special situation and not one encountered by most users. OTOH, if the team wishes, maybe you will make it a goal to be able to successfully run under test explorer through the adapter.

Whatever you do, I think you should probably put out some documentation that tells people that running NUnit tests under the test explorer does not currently work. You can tell them in that context either that it's a goal to make it work or that it's not a priority, whichever is decided.

As the first author of the adapter and a many-year contributor to the framework, I have always decried the use of the test explorer in the development of NUnit. That's still my view on the issue. Heck, I don't even like that we use the console to test the framework in our CI now!!!

Whatever the decision, I think you are going to have to keep sending people back to the actual NUnit runners (either NUnitLite or the console runner) when issues come up that may or may not be due to the adapter. The adapter remains fundamentally equivalent to a third-party runner, even if it's under the NUnit Project.

[aolszowka]

FWIW from the outside looking in: It really does not matter to me which way it cuts so long as its documented as such.

Out of the box I as a developer will try to do what I would normally do when I encounter any new project which is use the same practices I use internally:

1. Clone the Code
2. Read the README.md/BUILD.md/HACKING.md
3. Attempt a Build (without changes)
4. Run the Unit Tests (via the integrated Runner).
5. If everything works start playing with stuff.

In the past this would have been ReSharper's dotCover but we've been trying to wean ourselves off it as licensing costs are just crazy for small shops/individual developers when there are Free/Open Source Software Alternatives that "mostly work".

There is a lot (in my opinion) to the Integrated Development Environment concept, not having to pop out of the workflow is super useful which is why we do it.

However if we're expected to use the Console Runner (or some other Runner) that's fine in my mind, just document it.

[CharliePoole]

If you're developing on NUnit, working on the framework, I think that mixing in any runners that do things in special ways (R#, our own adapter) can confuse the issue. I like to know that the framework works correctly in isolation before testing it with runners. So in my own work, I use nunitlite to test as I develop and then run the CI locally. If a person wanted to develop on NUnit in Test Explorer, I'd say they should still run CI locally and should also fall back to nunitlite or nunit3-console when anything looks dodgy.

For folks just running the nunit tests from a particular release, I think there's lots more latitude.

[jnm2]

We should make CI canonical, probably with NUnitLite for all framework tests and then the Console, VSTest adapter and a UWP runner for end-to-end tests.

Assuming CI is solid, it seems okay with me if there are subtle differences between Test Explorer, ReSharper, NCrunch, and our CI script. Saying these in-IDE tools should be ignored is something I see as a barrier to contribution and false since that's not even my workflow. My ideal workflow:

1. Locate existing tests or write new tests
2. Pin that test fixture to make it quick to rerun the tests while typing (even better, start continuous testing)
3. Implement a change
4. Before creating a Git commit, run .\build.ps1 -t test to make sure the passes and failures are as expected
5. If there are any surprises, find and pin the affected tests I didn't know about and go to step 3

In the rare scenario where the tests are runner-sensitive, I'm not sure it's challenging to make them runner-insensitive. It can't be worse than the integration tests I wrote for ILMerge which I made resilient to ReSharper and NCrunch shadow copies.

[jnm2]

@aolszowka I 100% agree with your point that whatever we do, we should be respectful of contributors' time by keeping the contribution docs easy to consume and up-to-date.

[CharliePoole]

@jnm2 I like your "canonical" approach. I'm dubious that it will be easy to make everything work smoothly in Test Explorer, but it's worth trying.

It's still useful, it essential, for anyone working on nunit to know when to switch to a lower level of testing.

Biggest thing we could do to move this idea forward IMO is to establish separate system and maybe integration tests.

[jnm2]

This may be the root cause. While writing tests to get XML from FrameworkController, I noticed that TestContext.WorkDirectory was getting changed to C:\Program Files (x86)\Microsoft Visual Studio\2017\Professional\Common7\IDE:

nunit/src/NUnitFramework/framework/Api/DefaultTestAssemblyBuilder.cs

Line 137 in 81fcc7c

TestContext.DefaultWorkDirectory = Directory.GetCurrentDirectory();

Call stack:

>	nunit.framework.dll!NUnit.Framework.Api.DefaultTestAssemblyBuilder.Build(System.Reflection.Assembly assembly, string suiteName, System.Collections.Generic.IDictionary<string, object> options) Line 137	C#
 	nunit.framework.dll!NUnit.Framework.Api.DefaultTestAssemblyBuilder.Build(string assemblyNameOrPath, System.Collections.Generic.IDictionary<string, object> options) Line 114	C#
 	nunit.framework.dll!NUnit.Framework.Api.NUnitTestAssemblyRunner.Load(string assemblyNameOrPath, System.Collections.Generic.IDictionary<string, object> settings) Line 154	C#
 	nunit.framework.dll!NUnit.Framework.Api.FrameworkController.LoadTests() Line 204	C#

It's because TestContext.WorkDirectory is static mutable state:

nunit/src/NUnitFramework/framework/TestContext.cs

Lines 96 to 101 in 81fcc7c

	/// <summary>
	/// Static DefaultWorkDirectory is now used as the source
	/// of the public instance property WorkDirectory. This is
	/// a bit odd but necessary to avoid breaking user tests.
	/// </summary>
	internal static string DefaultWorkDirectory;

That means it's shared by all tests (!), including concurrent ones. Any test that relies on WorkDirectory must be marked [NonParallelizable] in order to be correct.

[oznetmaster]

I noted this problem a long time ago. There is an issue about it somewhere.

NonParallelizable is not enough. If any test modifies WorkDirectory (as many of the NUnit tests do), there is a reasonable probability that other tests that rely upon it will fail.

It is all dependent on the order the tests are executed in, which is undefined in NUnit.

[CharliePoole]

Intention is that Work directory should be set once and remain unchanged for the run. Anything else US a bug.

[jnm2]

Is there a reason that we can't allow an independent work directory for each execution context?

@CharliePoole How would we make that a reality? We're in a special position since the framework tests test FrameworkController, a controller run within a controller run.

[CharliePoole]

Leaving aside our tests, the meaning of WorkDirectory is "the directory set by the user to receive all output files for the run." So a test should not be able to change it.

For our own tests, we need to be able to set it, but that setting should not affect other tests if we do it right. We probably aren't doing it right. 😜

In a pinch, we could make it unchangeable and stop testing it. <ducks>

[jnm2]

That's actually a cool idea- track whether the static field has been set and skip setting it thereafter? That way it's unchangeable and I'm not seeing any tests that check whether DefaultTestAssemblyBuilder sets it, so we should be good!

[oznetmaster]

There are NUnit tests that do change it, and if it is made unchangeable, they might fail.

[jnm2]

@oznetmaster I searched but cannot find any NUnit tests that change it except accidentally. Do you have any handy?

[oznetmaster]

I would have to search my archives.

I have already fixed the problem in my CF build by doing what has been discussed. Only allowing the value to be set once. I check for it being null, and if it is, then allow it to be set.

				if (options.ContainsKey (FrameworkPackageSettings.WorkDirectory))
					TestContext.DefaultWorkDirectory = options[FrameworkPackageSettings.WorkDirectory] as string;
				else
					if (TestContext.DefaultWorkDirectory == null)
						TestContext.DefaultWorkDirectory = Directory.GetCurrentDirectory ();

[oznetmaster]

The tests that were the problem were those that would call DefaultTestAssemblyBuilder.Build with options that did not include FrameworkPackageSettings.WorkDirectory, causing the TestContext.DefaultWorkDirectory to be overwritten by the CurrentDirectory. This meant that if the WorkDirectory had been set in the top level execution, it would be overwritten by the test, and never restored.

[jnm2]

Sounds like we'd be good to take the same approach then?

[oznetmaster]

Works for me. My CF build passes 100% of the NUnit tests.

[jnm2]

@OmicronPersei If you're around in the next few days, can you give @oznetmaster's fix a spin?

[OmicronPersei]

Yes! Will try this evening

[OmicronPersei]

I can't really reproduce this bug because of nunit3-vs-adapter#528 causing other issues. Ideas?

[jnm2]

@OmicronPersei Oh. I don't know. Would using a .runsettings with WorkDirectory set correctly be a good workaround?

[OmicronPersei]

Okay I've tried that. TestCaseSourceCanAccessWorkDirectory succeeds but StackTracesAreFiltered fails with the same message/stack trace in the OP.

[jnm2]

I think StackTracesAreFiltered should be bumped to 5. It's a smell test so that we notice if new frames are added so we can make sure it hasn't gotten out of control.

[OmicronPersei]

What about TestCaseSourceCanAccessWorkDirectory, or is that still in scope of this PR?

[jnm2]

ForTestCaseCanAccessWorkDirectory, does @oznetmaster's fix solve it? If not, we'll have to investigate further.

[OmicronPersei]

I can't reproduce the problem on my machine, the test in question passes for me.

[jnm2]

No worries, someone else can do that part. I like having smaller PRs, too.

[aolszowka]

Just trying to do house cleaning; is there any reason that this issue should still be open or can we close it with some type of resolution? (Even if it is WONTFIX).

[rprouse]

No, thanks for pointing it out. Closing.