Add some checks for merged test groups#89521
Conversation
… convert to [Fact]
ghost
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue Detailsnull
|
| return 101; | ||
| } | ||
| public static void TestEntryPoint() | ||
| => Assert.Throws<DivideByZeroException>(() => Test(0)); |
There was a problem hiding this comment.
The general problem I have with these assert helpers is that they expand into a ton of unrelated code that I prefer not to see when I'm looking at the test. I stopped using these generally in JIT tests when I had to skip past tons of inlined Assert.Equals gunk while investigating a test at some point.
It's probably ok for this test since most interesting stuff goes on inside Test.
There was a problem hiding this comment.
Thanks for the feedback. This is interesting as I've had the opposite experience when looking at the source code (rather than debugging them) in that there is so much boilerplate in the way. That said, I don't plan on widespread changes to call Equals a bunch but just happened to notice how much boilerplate was needed for Throws while auditing recent test changes. I also came to the conclusion that Throws was "less" than Equals and friends.
It would be nice to have a helper that addresses both sides. I wonder if the xunit folks would accept NoInlining everywhere. Alternatively, we could build our own wrapper library, though it would be unfortunate to end up "not standard".
(Kind of related - [Theory] often ends up not saving much over a wrapper [Fact] method since you often end up needing to pass the expected output and explicitly check against it anyway. It would be nice to have the expected return in the [InlineData]/etc., but again it wouldn't be standard.
There was a problem hiding this comment.
It's also interesting how often tests define their own AssertEquals, etc.!
| } | ||
|
|
||
| return 100; | ||
| public static void TestEntryPoint() |
There was a problem hiding this comment.
Fact? Also, Task.Run(TestTask) should presumably be Task.Run(TestTask).Wait().
|
@trylek @jkoritzinsky This starts to add some checks to avoid mistakes in our tests. PTAL - feedback especially welcome on how to do things better with Roslyn! upcoming - #89550, #89554, and possibly other things that we find to check |
| new DiagnosticDescriptor( | ||
| "XUW1001", | ||
| "No explicit entry point", | ||
| "Projects in merged tests group should not contain entry points", | ||
| "XUnitWrapperGenerator", | ||
| DiagnosticSeverity.Warning, | ||
| isEnabledByDefault: true), |
There was a problem hiding this comment.
We should save this in a static readonly field (makes it easier to know which diagnostic ids are in use).
There was a problem hiding this comment.
Thanks. just fyi - I also noticed that I was generating SimpleRunners for all tests now and restricted the actual wrapper generation to ConsoleApplications (hopefully nothing is depending on libraries having entry points). I think I will run outerloop+extraplatforms to verify.
|
/azp run runtime-coreclr outerloop, runtime-extra-platforms |
|
Azure Pipelines successfully started running 2 pipeline(s). |
| var result = new TestResult(name, containingTypeName, methodName, duration, null, null, output); | ||
| _testResults.Add(result); | ||
|
|
||
| outTw.WriteLine($"{0:HH:mm:ss.fff} Passed test: {1}", System.DateTime.Now, name); |
There was a problem hiding this comment.
@markples This line results in printing "HH:mm:ssfff Passed test: 1" always -- presumably it shouldn't be a string interpolation.
There was a problem hiding this comment.
Ah, that's leftover from the original
builder.AppendLine($"System.Console.WriteLine(\"{{0:HH:mm:ss.fff}} Running test: {{1}}\", System.DateTime.Now, {test.TestNameExpression});");
but was for the outer level. It's weird though; I thought that I looked at the output.
ghost
locked as resolved and limited conversation to collaborators
3 participants
Uh oh!
There was an error while loading. Please reload this page.