Add some checks for merged test groups #89521

markples · 2023-07-26T19:09:01Z

CLRTest.Execute.Batch.targets
- Fix CLRTestExecutionArguments (allows passing a filter argument to .cmd
- Fix indenting in generated script
XUnitWrapperGenerator
- Move some generated code into (shared) ITestInfo.cs
- Also display exception on stdout on failure
- Add check that projects in merged test groups don't contain entry points
  - Can be a sign of code that is expected to run but won't be
  - Or can be a [Fact]/etc-less entry point in a ReqProcIso project but would then need to be fixed if ReqProcIso were removed
Directory.Build.targets
- Run XUnitWrapperGenerator on test projects in merged groups
- Can't simply replace the conditional because we have XUnit-style tests outside of merged groups
Update tests to conform to above checks plus a bit of opportunistic cleanup

…ve whitespace

… convert to [Fact]

…rary

ghost · 2023-07-26T19:09:11Z

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

null

Author:	markples
Assignees:	markples
Labels:	`area-CodeGen-coreclr`
Milestone:	-

jakobbotsch · 2023-07-26T19:15:11Z

src/tests/JIT/Regression/JitBlue/Runtime_75832/Runtime_75832.cs

-        return 101;
-    }
+    public static void TestEntryPoint()
+        => Assert.Throws<DivideByZeroException>(() => Test(0));


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.

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.

It's also interesting how often tests define their own AssertEquals, etc.!

jakobbotsch · 2023-07-26T19:18:41Z

src/tests/JIT/opt/Devirtualization/box2.cs


-        return 100;
+    public static void TestEntryPoint()


Fact? Also, Task.Run(TestTask) should presumably be Task.Run(TestTask).Wait().

thanks - fixed

…merged-checks

…85576)" Also fix test path and update issue link This reverts commit d75e811.

markples · 2023-07-27T19:07:01Z

@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!
cc @dotnet/jit-contrib

upcoming - #89550, #89554, and possibly other things that we find to check

jkoritzinsky · 2023-07-27T23:11:29Z

src/tests/Common/XUnitWrapperGenerator/XUnitWrapperGenerator.cs

+                new DiagnosticDescriptor(
+                    "XUW1001",
+                    "No explicit entry point",
+                    "Projects in merged tests group should not contain entry points",
+                    "XUnitWrapperGenerator",
+                    DiagnosticSeverity.Warning,
+                    isEnabledByDefault: true),


We should save this in a static readonly field (makes it easier to know which diagnostic ids are in use).

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.

markples · 2023-07-27T23:50:53Z

/azp run runtime-coreclr outerloop, runtime-extra-platforms

azure-pipelines · 2023-07-27T23:51:10Z

Azure Pipelines successfully started running 2 pipeline(s).

markples · 2023-07-28T18:38:48Z

runtime is clean. outerloop and extraplatforms seem to be #89545, #88808, and completely unrelated jobs

jakobbotsch · 2023-08-09T10:13:25Z

src/tests/Common/XUnitWrapperLibrary/TestSummary.cs

@@ -133,8 +140,10 @@ public void WriteFooterToTempLog(StreamWriter tempLogSw)
        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);


@markples This line results in printing "HH:mm:ssfff Passed test: 1" always -- presumably it shouldn't be a string interpolation.

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.

markples added 10 commits July 24, 2023 18:52

First attempt at first rules

baba1b4

Fix CLRTestExecutionArguments in CLRTest.Execute.Batch.targets, impro…

0caf13d

…ve whitespace

XUWG: Display exceptions in stdout, factor a bit

453e637

JIT/opt/Devirtualization/box2: Use Task.Run to de-async Main and then…

8c7bc27

… convert to [Fact]

hfa_nested_f64_interop_cpp.csproj: No RequiresProcessIsolation on lib…

e0f9528

…rary

Runtime_82535: Use Fact, remove tabs

b2cbac0

JIT/Methodical/delegate: Use Fact, leave exception for harness

11d7bd2

b08046cs: Remove Environment.ExitCode, improve logic

305808c

Runtime_75832: Use Assert.Throws

c79f25e

Factor XUWG

0d5f37f

dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jul 26, 2023

ghost assigned markples Jul 26, 2023

typos

fa6db9c

jakobbotsch reviewed Jul 26, 2023

View reviewed changes

markples added 5 commits July 26, 2023 12:19

Make XUW1001 a warning

4df2ebf

Fix box2.cs

40753c8

Reinstate RefXUWG disjuncts in D.B.t

3dd47f2

Merge remote-tracking branch 'origin/start-merged-checks' into start-…

d6b3353

…merged-checks

Add flush for consistency

41a8dcc

markples mentioned this pull request Jul 27, 2023

Remove unnecessary "return 100"s from tests #89550

Merged

markples added 3 commits July 27, 2023 01:09

Limit new checks to merged test groups

31e281f

box2 - make class public

b9eeedb

Remove check that any tests exist

2b0269f

markples mentioned this pull request Jul 27, 2023

Add check that tests are public #89554

Merged

build-analysis bot mentioned this pull request Jul 27, 2023

Tracking issue for CI build timeouts #76454

Closed

markples mentioned this pull request Jul 27, 2023

JIT: Runtime_82535 crashes on osx x64 #89585

Closed

markples added 2 commits July 27, 2023 10:59

Revert "Reenable a test which doesn't seem to fail any more. (dotnet#…

985534e

…85576)" Also fix test path and update issue link This reverts commit d75e811.

Merge remote-tracking branch 'dotnet/main' into start-merged-checks

a5bab7b

markples marked this pull request as ready for review July 27, 2023 19:07

jkoritzinsky approved these changes Jul 27, 2023

View reviewed changes

static for Descriptor; only write entrypoints for apps

7969174

markples merged commit 6f27090 into dotnet:main Jul 28, 2023
181 of 211 checks passed

jakobbotsch reviewed Aug 9, 2023

View reviewed changes

ghost locked as resolved and limited conversation to collaborators Sep 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add some checks for merged test groups #89521

Add some checks for merged test groups #89521

markples commented Jul 26, 2023 •

edited

Loading

ghost commented Jul 26, 2023

jakobbotsch Jul 26, 2023 •

edited

Loading

markples Jul 26, 2023

markples Jul 26, 2023

jakobbotsch Jul 26, 2023

markples Jul 26, 2023

markples commented Jul 27, 2023

jkoritzinsky Jul 27, 2023

markples Jul 27, 2023

markples commented Jul 27, 2023

azure-pipelines bot commented Jul 27, 2023

markples commented Jul 28, 2023

jakobbotsch Aug 9, 2023

markples Aug 9, 2023

Add some checks for merged test groups #89521

Add some checks for merged test groups #89521

Conversation

markples commented Jul 26, 2023 • edited Loading

ghost commented Jul 26, 2023

jakobbotsch Jul 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markples commented Jul 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markples commented Jul 27, 2023

azure-pipelines bot commented Jul 27, 2023

markples commented Jul 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markples commented Jul 26, 2023 •

edited

Loading

jakobbotsch Jul 26, 2023 •

edited

Loading