Disable parallel test runs for Nancy.Tests

[blairconrad]

Several fixtures manipulate (and possibly more depend on) the static field
StaticConfiguration.CaseSensitive, causing interference.

Prerequisites

• I have written a descriptive pull-request title
• I have verified that there are no overlapping pull-requests open
• I have verified that I am following the Nancy code style guidelines
• I have provided test coverage for my change (where applicable)

Description

Disable test run parallelization for Nancy.Tests.
I noticed intermittent test failures in this module (even on master, on Windows) during my work on #2634. After further investigation performed as described in #2702, helped along with comments from @xt0rted and @jchannon, I see that both Nancy.Tests.Unit.RequestFixture and Nancy.Tests.Unit.Routing.DefaultRouteResolverFixture
setStaticConfiguration.CaseSensitive`. I figured this to be a primary cause of the failures.

I ran dotnet test -c Release in Nancy.Tests repeatedly on Windows on the master branch, then disabled test parallelization and repeated.

Here's what I found:
With parallelization enabled (as master has it), tests failed after various numbers of runs:
7, 3, 6, 1, 2, 2, 2, 3, 1, 2, 1, 3, 1, 1, 1, 8, 11, 4, 2, 1, 2, 1, 1, 12, 7, 3, 1, 1, 7, 1, 8, 15, 10, 1, 4, 3, 3, 1 , 2, 1, 1, 2, 1, 2, 1, 4, 14, 3, 14, 10, 3, 1, 1, 2, 3, 3, 1, 4, 5, 6, 1, 9, 7, 4, 12, 7, 1, 5, 3, 1, 6, 9, 6, 1, 6, 2, 2, 1, 1, 2, 2, 2, 6, 2, 1 , 5, 11, 4, 4, 2, 1, 3, 7, 3, 3, 6, 2, 10, 1, 1, 26, 2, 1, 7, 4, 10, 2, 1, 12, 2, 1, 1, 7, 1, 1, 4, 1, 7, 6, 2, 4, 1, 1, 4, 3, 2, 2, 1, 3, 1, 2, 3, 1, 2, 5, 2, 7, 7, 2, 6, 6, 1, 1, 1, 2, 4, 3, 6, 4, 2, 13, 1, 8, 2, 2, 10, 8, 1, 6, 5, 3, 6, 2, 1, 5, 1, 2, 1, 10, 9, 4, 3, 5, 6, 2, 9, 3, 1, 2 , 17, 1, 9, 3, 4, 5, 5, 1, 2, 5, 1, 4, 2, 1, 1, 1, 1, 8, 2, 2, 1, 5, 8, 2, 2, 2, 3, 3, 2, 2, 1, 1, 3, 4, 3, 3, 1, 3, 1, 2, 1, 1, 3, 3, 1

With parallelization disabled, the tests did not fail once in over 200 runs.

Here's a picture:

It's possible that a more sophisticated fix could be found, but in the interim, I propose disabling parallel test execution.

[thecodejunkie]

Copied this from #2702

@blairconrad It's not unlikely that StaticConfiguration is the cause of the problem. I've previously added a task to remove the last, remaining parts, of StaticConfiguration in #2659

A while back I also created StaticConfigurationContext to help mitigate race conditions, in tests, that used StaticConfiguration values. It looks like DefaultRouteResolverFixture does not use it.. would be interesting to see if adding it would solve the intermittent test failtures

[blairconrad]

@thecodejunkie, StaticConfigurationContext looks like it would help for serialized tests, but I think won't protect us when the tests are run in parallel. Unless I'm missing something, it performs no synchronization, so the other fixtures could still muck about with the setting, no?

[blairconrad]

I'm not sure what to make of the Travis failure. At work, so no time/ability to investigate.

[blairconrad]

… at least it's not the first time it's failed

[thecodejunkie]

I'm kind of tempted to think that it's also StaticConfiguration that might be causing it. I'm just quasi-guessing here, but LiteralNode is also using StaticConfiguration.CaseSensitive so maybe the parallel test execution screw it up sometimes so the BrowserFixture JSON test doesn't match a route.. 🤔

[blairconrad]

Interesting idea, @thecodejunkie. If you'd like, I could try disabling the parallel execution on Nancy.Testing.Tests as well. (But where will it all end!? 😉)

[thecodejunkie]

To be honest, we're working on removing StaticConfiguration so we can live with the intermittent failure for the time being

[blairconrad]

Fair enough. Should we close this PR then?

[jchannon]

Does that assemblyinfo change work for netcore tests?

[blairconrad]

Seems to. Test times jump from about 21–22 seconds to about 34–35 seconds when I add the AssemblyInfo.cs.

[jchannon]

In netcore you pass it to the runner eg dotnet test -parallel none plus .netcore doesnt know what assembly info is, it uses project.json

I think you'd have to amend the build.cake to pass that argument

[blairconrad]

@jchannon, I would be the first to admit that I don't fully understand .NET Core or XUnit, but I'm confused by your earlier comments.

In netcore you pass it to the runner eg dotnet test -parallel none

This seems to be a way to disable parallel test execution, but I don't think it's the only way.

.netcore doesnt know what assembly info is, it uses project.json

I don't know exactly what you're suggesting by "doesn't know what assembly info is". The file is built into the assembly. The attribute does appear when I reflect the assembly. And as I explain below, it seems to make a difference. Also, in a separate project, I'm including an equivalent file in the build, specifying assembly version numbers, and they are respected.

I think it would be up to XUnit to interpret the attribute once it's there; this wouldn't be something directly controlled by .NET Core. I took a peek at the XUnit source, and it seems to be treating the CollectionBehavior attribute the same way whether compiled under .NET Core or not, so I'd expect it to work. And consider my evidence:

1. when I include the attribute, Nancy.Tests execution time rises from about 21 seconds to about 34. This is repeatable over many test runs
2. when I run dotnet test -parallel none, the time also rises to about 34 seconds per run, which is pretty suggestive
3. ( I also seem to get more test failures without the attribute, but I've not repeated my original experiment, so at this point, my evidence is anecdotal)

[blairconrad]

Oh, and is the whole discussion moot, since you're trying to get rid of StaticConfiguration, and that may obviate the need for serializing the tests?

[blairconrad]

In case we're keeping track, we've another failed build.

But at this I'm more interested in knowing whether

1. we should close the PR because there's no interest, or
2. if there's interest, what more you'd like done before a merge can happen

Thanks!

[thecodejunkie]

@NancyFx/most-valued-minions @NancyFx/owners I think we should pull this in for now.. it seems that it's causing test failures in most PRs after we enabled more test projects to run on core. Any objections?

[jchannon]

nope do it

…

On 14 March 2017 at 12:52, Andreas Håkansson ***@***.***> wrote: @NancyFx/most-valued-minions <https://github.com/orgs/NancyFx/teams/most-valued-minions> @NancyFx/owners <https://github.com/orgs/NancyFx/teams/owners> I think we should pull this in for now.. it seems that it's causing test failures in most PRs after we enabled more test projects to run on core. Any objections? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2703 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGapjW5IYYfXcQW7HlrbqYtA6O4sSJGks5rlo2rgaJpZM4L8OHl> .

[blairconrad]

Want a rebase?

[blairconrad]

Rather, object to a rebase?

[blairconrad]

Ah, to heck with it. I'm rebasing.

[blairconrad]

Thanks, @thecodejunkie!

[thecodejunkie]

@blairconrad thank you! 👍