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

Migrate from xUnit to NUnit #79

Merged
merged 38 commits into from
May 11, 2017
Merged

Migrate from xUnit to NUnit #79

merged 38 commits into from
May 11, 2017

Conversation

leasunhy
Copy link
Contributor

@leasunhy leasunhy commented Apr 28, 2017

This PR deprecates xUnit in favor of NUnit.

  • Update the test project template - .NET Framework.
  • Update the test project template - .NET Core.
  • Migrate existing tests.
  • Edit the root msbuild script files - test.csproj.
  • Edit the root msbuild script files - test_coreclr.csproj.
  • Create a little script to help test project migration.

@msftclas
Copy link

@leasunhy,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by Microsoft. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

@yatli
Copy link

yatli commented Apr 28, 2017

Looking good, please do some testing with coreclr. Thanks!

s@InlineData@TestCase@g
s@Fact@Test@g
s@using Xunit@using NUnit.Framework@g
s@Assert.Equal@Assert.AreEqual@g
Copy link

Choose a reason for hiding this comment

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

TODO add Assert.True -> Assert.That

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@leasunhy
Copy link
Contributor Author

leasunhy commented Apr 30, 2017

Support for coreclr blocked due to nunit/nunit3-vs-adapter#313.
According to that PR, hopefully we will be able to support coreclr in a few days.

@yatli
Copy link

yatli commented May 1, 2017

@leasunhy are you sure about this? IIRC nunit has always been working with dotnet test. The PR you referenced is for the visual studio adapter (vstest.exe, or to run the tests within VS).

{
if (args.Length < 2)
{
Console.Error.WriteLine("Usage: runner <runner-path> <test-assembly> <result-dir> [nunitlite-arg ..]");
Copy link

Choose a reason for hiding this comment

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

what is runner-path?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The testing process for one test assembly is:

  1. Meta-runner scans the test assembly and get the list of tests.
  2. Meta-runner starts a test-runner for each test in the test assembly.
  3. (TODO) Merge the result XML files into one.

... where, the path of the test-runner is specified in runner-path.

var assemblyPath = Path.GetFullPath(args[2]);
var remainingOptions = args.Skip(3).ToArray();

// NOTE(leasunhy): this may fail silently if some of the dependencies are not preloaded!
Copy link

Choose a reason for hiding this comment

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

no exceptions?

Copy link
Contributor Author

@leasunhy leasunhy May 2, 2017

Choose a reason for hiding this comment

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

Yes, in our scenario, when NUnit is not preloaded, it will throw an exception, but when Trinity is not preloaded, it will succeed but no tests can be discovered.


foreach (var testName in allTestNames)
{
var process = CreateProcessForTest(runnerPath, assemblyPath, resultDirPath, remainingOptions, testName);
Copy link

@yatli yatli May 2, 2017

Choose a reason for hiding this comment

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

TODO we'd better add a timeout limit (for example 30mins). IIRC NUnitLite has this feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@leasunhy
Copy link
Contributor Author

leasunhy commented May 4, 2017

The --timeout option of NUnitLite under .NET Core behaves strangely. That option will block the test runner even after the tests should have finished.
When NUnitLite is used with .NET Framework, the problem is gone.

@leasunhy
Copy link
Contributor Author

leasunhy commented May 4, 2017

OK, actually NUnit on .NET Core does not support Timeout at all. Even TimeoutAtttribute does not exist.
The cause is rooted in .NET Core: it does not provide Thread.Abort()...
I guess we will have to do Timeout in the meta runner.
How do you think? @yatli

@yatli
Copy link

yatli commented May 4, 2017

@leasunhy agreed. A bit of a hack, but see: https://github.com/dotnet/coreclr/blob/master/tests/src/baseservices/threading/regressions/threadex.cs

In the inner meta runner process, setup a timer event, which sends abort to record the test results.
Also, how about Thread.Interrupt()? Since we want the main thread of the test engine kept running, we should only inject an exception into it, not to abort it.

@@ -47,7 +51,7 @@ public static int Main(string[] args)

foreach (var testName in allTestNames)
{
var process = CreateProcessForTest(runnerPath, assemblyPath, resultDirPath, remainingOptions, testName);
var process = CreateProcessForTest(runnerPath, assemblyPath, resultDirPath, runnerOptions, testName);
process.WaitForExit();
Console.WriteLine(process.StandardOutput.ReadToEnd());
// TODO(leasunhy): check the exit status of the process and regard the test as a failure
Copy link

Choose a reason for hiding this comment

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

Just don't forget to add this feature :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, which feature?

Copy link

Choose a reason for hiding this comment

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

exit code

@yatli yatli merged commit 9b7545a into microsoft:test May 11, 2017
@leasunhy leasunhy deleted the test-nunit branch April 18, 2018 15:16
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.

3 participants