-
Notifications
You must be signed in to change notification settings - Fork 183
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
Upgrade xunit to 2.4.0, switch to dotnet test #1445
Upgrade xunit to 2.4.0, switch to dotnet test #1445
Conversation
<PackageReference Include="xunit.core" Version="2.3.0-beta2-build3683" /> | ||
<PackageReference Include="xunit.runner.visualstudio" Version="2.3.0-beta2-build1317" /> | ||
<PackageReference Include="Microsoft.TestPlatform.TestHost" Version="15.0.0" /> | ||
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="15.8.0" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should have been using this instead of Microsoft.TestPlatform.TestHost
. Anyway, xUnit's "getting started" section uses this package.
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="15.8.0" /> | ||
<PackageReference Include="xunit.core" Version="2.4.0" /> | ||
<PackageReference Include="xunit.runner.visualstudio" Version="2.4.0" /> | ||
<PackageReference Include="XunitXml.TestLogger" Version="2.0.0" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no built-in XML logger. This is the one recommended on this page. However I see 2 issues with it:
- it's not an official implementation (https://github.com/spekt/xunit.testlogger)
- it doesn't take the framework into account, so there's only one result file, which seems to contain results only for the last framework run.
Alternative options:
- Do we really need those files? Me, I never looked at them. Maybe we could just disable reporting.
- Do we really need them to be in the xUnit format? We could use the built-in trx logger
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it doesn't take the framework into account, so there's only one result file, which seems to contain results only for the last framework run.
The trx logger has the same problem. So if we want per-framework result files, we have to run the tests per framework, as @adamralph did in #1442...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's true. On the other hand, I'm not against submitting an issue/PR to spekt/xunit.testlogger.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about dropping the result files completely? Do you see any value in them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, I don't have them in any of my projects, and I don't miss them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not against submitting an issue/PR to spekt/xunit.testlogger.
Sure, but there are 2 PRs open since July that didn't even receive a comment... not a very good sign.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's true. And the project is an absolute mess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mostly enjoy the results files when the console output is too hard to read. Seems like we're making that easier, so I'm game to try without them.
@@ -8,7 +8,7 @@ | |||
|
|||
public class Program | |||
{ | |||
private const string TestsDirectory = "artifacts/tests"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't really like that name, it was misleading
tools/FakeItEasy.Build/Program.cs
Outdated
var xml = Path.GetFullPath(Path.Combine(TestsDirectory, Path.GetFileName(testDirectory) + ".TestResults.xml")); | ||
Run("dotnet", $"xunit -configuration Release -nologo -nobuild -noautoreporters -notrait \"explicit=yes\" -xml {xml}", testDirectory); | ||
var xml = Path.GetFullPath(Path.Combine(TestResultsDirectory, Path.GetFileName(testDirectory) + ".TestResults.xml")); | ||
Run("dotnet", $"test -c Release --no-build --logger:\"xunit;LogFilePath={xml}\"", testDirectory); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are many dotnet xunit
options that don't have equivalent in dotnet test
-nologo
: I spent a bit of time trying to find it, but apparently there's no way to suppress the tool output header. I also didn't succeed in reducing the verbosity, even with--verbosity quiet
(the default seems to beminimal
, which produces the same outputquiet
)-noautoreporters
: no equivalent, as far as I can tell. The tests are reported to AppVeyor again, and I don't know how to disable it.-notrait
: there is a--filter
option, but--filter "explicit!=yes
doesn't work, even though the docs mention it's possible. We're not using this at the moment anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the first problem, I'd found microsoft/vstest#1701.
Ah, I forgot to check the auto reporters. Good find.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the first problem, I'd found microsoft/vstest#1701.
Yes, but it doesn't provide a solution...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, no. But it's nice to know that someone's aware of it.
@thomaslevesque, I generally like the way this is going (except for I agree we could drop the logger. For me the only remaining issue of concern would be the inability to disable autoreporters. That was a real pain point for the AppVeyor builds. |
Out of interest, what is this about? I don't do anything special around "autoreporters" in other projects and the Appveyor builds are fine. |
Well, I tend to prefer the short form, but I can change it to
Tests are reported to AppVeyor and appear in the "Tests" tab of the build. This looks like a good idea, but it has several issues:
|
I usually like the explicit version in scripts, but I'm not too bent out of shape. Vote with your ❤️. |
@adamralph, @thomaslevesque, for interest, I checked the latest build for this PR, and the autotest reporting added at least 2:12 to the build time (measured from time of last line of the console output to the reported build time near the top of the page). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love it, @thomaslevesque!
tools/FakeItEasy.Build/Program.cs
Outdated
Run("dotnet", $"test -c Release --no-build --logger:\"xunit;LogFilePath={xml}\"", testDirectory); | ||
} | ||
private static void RunTests(string testDirectory) => | ||
Run("dotnet", $"test --configuration Release --no-build -- RunConfiguration.NoAutoReporters=true", testDirectory); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The $
is bonus now.
Oh! You put the --configuration
in. How kind. I guess after you saved all those "testresult" characters, why not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
$
is bonus now.
Oops. I'll remove it. Usually R# tells me about these things, but I did everything in VS Code...
c61d5ed
to
d178354
Compare
@thomaslevesque, looks great. Is there a reason we're still [WIP]? |
No, I just forgot to remove it.
I'm on my phone now, so I can't do it easily. Feel free to rename!
Le mar. 11 sept. 2018 à 17:48, Blair Conrad <notifications@github.com> a
écrit :
… @thomaslevesque <https://github.com/thomaslevesque>, looks great. Is
there a reason we're still [WIP]?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1445 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABxyWQweO3eOJB_4OWQDXJ3xD2RhRumQks5uZ9tDgaJpZM4WjORY>
.
|
Ouch! I can understand the reason for suppressing it!
Yes, this is very frustrating. I've been chiming in on appveyor/ci#239 about that. |
Thanks, @thomaslevesque! |
Thanks for the merge!
Le mar. 11 sept. 2018 à 19:18, Blair Conrad <notifications@github.com> a
écrit :
… Thanks, @thomaslevesque <https://github.com/thomaslevesque>!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1445 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABxyWYSb6JHtLIbFt3aaMPHd-XMNjI2dks5uZ_CAgaJpZM4WjORY>
.
|
Fixes #1444