Validate with `-testUsedAssemblies` in a CI leg by jcouv · Pull Request #60052 · dotnet/roslyn

jcouv · 2022-03-09T06:14:06Z

This identified a few issues tests that assert or throw when GetEmitDiagnostics() is called: VoidEvent, NullCoalescingOperatorWithTupleNames, ExternAliases4, CrossAssemblySupportingAndNotSupportingCovariantReturns, TestNullCoalesce_Dynamic

Verified the validation fails as expected with a canary.

Fixes #60086

jcouv · 2022-03-10T04:26:11Z

eng/build.ps1

+    $env:ROSLYN_TEST_EMITDIAGNOSTICS = "true"
+  }
+
  if ($testUsedAssemblies) {


@AlekseyTs Should we enable testUsedAssemblies in a CI leg? Seems like it's unused at the moment. #Closed

Should we enable testUsedAssemblies in a CI leg? Seems like it's unused at the moment.

I think it would be good to add a separate CI leg similar to the one used for IOperation testing.

Okay. Let's keep that separate (there's a few questions, such as choosing a leg, how to deal with potential failures that might have snuck in).
Filed tracking issue.

Towards that goal, this PR fixes an issue with TestRunner that makes sure we copy environment variables to Helix machines where tests end up running.

I think we should consider whether to group this set of "additional validation" flags into a single leg or to have separate legs. Recently we are trying to be sensitive about number of test legs introduced because:

Each time we additionally run the tests increases the rate of spurious failures.

Cost of provisioning the additional machines.

See #57313.

jcouv · 2022-03-10T17:57:29Z

@cston @RikkiGibson @dotnet/roslyn-compiler for review. Thanks

RikkiGibson

Implementation looks good, but perhaps consideration from the team is required to decide whether to do this validation in the existing job or in a new job.

RikkiGibson · 2022-03-10T17:57:54Z

src/Tools/Source/RunTests/TestRunner.cs

+
+            static void transferEnvironmentVariable(StringBuilder command, string setEnvironmentVariable, string variableName)
+            {
+                if (Environment.GetEnvironmentVariable(variableName) is string iop)


Consider inlining. #Resolved

RikkiGibson · 2022-03-10T17:59:50Z

src/Compilers/CSharp/Test/Semantic/Semantics/OperatorTests.cs

 }
 ";
-            var compilation = CreateCompilation(source).VerifyDiagnostics();
+            // We use VerifyDiagnosticsOnly until this issue is fixed:


Should we get emit diagnostics anyway and assert about the nature of the failure? I'm concerned that emit bugs may be inadvertently fixed without related test baselines getting updated, resulting in either introduction of redundant tests or reduced test coverage of the fix. Similar to the issue that comes up when 'Skip' is used in tests. #WontFix

I agree that skipped and conditional tests could drift, but conditional tests on NoUsedAssembliesValidation and NoIOperationValidation are how we deal with problematic tests in those CI legs at the moment. I'd rather not revisit that for purpose of this PR

RikkiGibson · 2022-03-10T18:03:23Z

eng/build.ps1

+    $env:ROSLYN_TEST_EMITDIAGNOSTICS = "true"
+  }
+
  if ($testUsedAssemblies) {


I think we should consider whether to group this set of "additional validation" flags into a single leg or to have separate legs. Recently we are trying to be sensitive about number of test legs introduced because:

Each time we additionally run the tests increases the rate of spurious failures.

Cost of provisioning the additional machines.

See #57313.

jcouv · 2022-03-10T21:18:41Z

FYI @jmarolf From discussion with Rikki and team, we're adding a new CI leg (debug, core, compiler tests only, with some additional validation added to every compiler test for used assemblies and emit diagnostics).

jaredpar · 2022-03-10T21:22:20Z

azure-pipelines.yml

    configuration: Debug
    testArguments: -testCoreClr -testIOperation -testCompilerOnly

+template: eng/pipelines/test-windows-job.yml


template: eng/pipelines/test-windows-job.yml

Consider adding a comment here explaining why this job is diff than others. #Resolved

jaredpar · 2022-03-10T21:22:46Z

eng/build.ps1


-    if ($testUsedAssemblies) {
-      Remove-Item env:\ROSLYN_TEST_USEDASSEMBLIES
-    }


Why was this removed?

jcouv · 2022-03-10T22:25:42Z

/azp run

azure-pipelines · 2022-03-10T22:26:01Z

Azure Pipelines successfully started running 3 pipeline(s), but failed to run 1 pipeline(s).

azure-pipelines.yml

jaredpar · 2022-03-15T17:03:16Z

src/Compilers/CSharp/Test/Emit/PDB/PDBUsingTests.cs

        }

-        [Fact]
+        [ConditionalFact(typeof(NoUsedAssembliesValidation))] // Tracked by https://github.com/dotnet/roslyn/issues/60045


Why do you use comments here vs. putting the URL into the Reason property? #Resolved

Not much reason. Either one works

One appears in the test logs, one does not 🙂.

Co-authored-by: Jared Parsons <jaredpparsons@gmail.com>

RikkiGibson · 2022-03-15T17:52:48Z

azure-pipelines.yml

+- template: eng/pipelines/test-windows-job.yml
+  parameters:
+    testRunName: 'Test Windows CoreCLR TestUsedAssemblies Debug'
+    jobName: Test_Windows_CoreClr_TestUsedAssemblies_Debug


nit: would change TestUsedAssemblies to UsedAssemblies here and in 'testRunName'. #Resolved

RikkiGibson · 2022-03-15T17:58:22Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTupleTest.cs

        }

-        [Fact]
+        [ConditionalFact(typeof(NoUsedAssembliesValidation))] // Tracked by https://github.com/dotnet/roslyn/issues/60046


I see that 'VerifyDiagnosticsOnly' was removed and replaced with ConditionalFact. I would prefer if we didn't skip tests or make them conditional. Ideally, if the underlying bug were fixed here, this test would start failing, forcing us to update, which will end up preventing test duplication.

We could consider capturing the fact that an assertion fails in debug mode with the following pattern:

comp.VerifyDiagnosticsOnly(...); #if DEBUG Assert.Throws<InvalidOperationException(() => comp.VerifyEmitDiagnostics()); #endif

jcouv self-assigned this Mar 9, 2022

ghost added the Area-Compilers label Mar 9, 2022

jcouv force-pushed the emit-diagnostics branch from 39b11b7 to f78c9ed Compare March 9, 2022 07:59

runfoapp bot mentioned this pull request Mar 9, 2022

Timeout on Microsoft.VisualStudio.LanguageServices.CSharp.UnitTests.dll tests #55638

Open

This was referenced Mar 9, 2022

[Flaky test] Roslyn.VisualStudio.IntegrationTests.CSharp.CSharpRename.VerifyRenameInStandaloneFiles #60068

Closed

[Flaky test] GoToImplementationOpensProvisionalTabIfDocumentNotOpen #60071

Closed

Pull on emit diagnostics in tests in a CI leg

cd86d1a

jcouv force-pushed the emit-diagnostics branch from 1830c32 to cd86d1a Compare March 10, 2022 00:34

Julien Couvreur added 2 commits March 9, 2022 18:48

Transfer environment variables to Helix

292f236

Remove canary

9be4f2d

jcouv marked this pull request as ready for review March 10, 2022 04:24

jcouv requested review from a team as code owners March 10, 2022 04:24

jcouv commented Mar 10, 2022

View reviewed changes

jcouv mentioned this pull request Mar 10, 2022

Add -testUsedAssemblies checks to a CI leg #60086

Closed

RikkiGibson approved these changes Mar 10, 2022

View reviewed changes

jcouv marked this pull request as draft March 10, 2022 20:28

jcouv changed the title ~~Pull on emit diagnostics in tests in a CI leg~~ Validate with -testUsedAssemblies in a CI leg Mar 10, 2022

jaredpar reviewed Mar 10, 2022

View reviewed changes

jcouv force-pushed the emit-diagnostics branch from d68b30d to 31074bc Compare March 10, 2022 21:29

Create new leg with testUsedAssemblies

5406f87

jcouv force-pushed the emit-diagnostics branch from 31074bc to 5406f87 Compare March 10, 2022 21:33

tweaks

c117d2a

jcouv force-pushed the emit-diagnostics branch 2 times, most recently from 415a6d8 to 5c16f6d Compare March 11, 2022 06:46

Remove canary

0b2e200

jcouv force-pushed the emit-diagnostics branch from 5c16f6d to 0b2e200 Compare March 11, 2022 08:34

jcouv marked this pull request as ready for review March 11, 2022 17:14

jcouv requested review from AlekseyTs, RikkiGibson and jaredpar March 11, 2022 17:14

jaredpar approved these changes Mar 15, 2022

View reviewed changes

Update azure-pipelines.yml

cc08ed1

Co-authored-by: Jared Parsons <jaredpparsons@gmail.com>

RikkiGibson reviewed Mar 15, 2022

View reviewed changes

jmarolf approved these changes Mar 15, 2022

View reviewed changes

Address feedback

95c9322

RikkiGibson approved these changes Mar 16, 2022

View reviewed changes

jcouv enabled auto-merge (squash) March 16, 2022 01:08

jcouv merged commit 839fe5e into dotnet:main Mar 16, 2022

ghost added this to the Next milestone Mar 16, 2022

allisonchou modified the milestones: Next, 17.2.P3 Mar 28, 2022

Conversation

jcouv commented Mar 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv Mar 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv commented Mar 10, 2022

Uh oh!

RikkiGibson left a comment

Choose a reason for hiding this comment

Uh oh!

RikkiGibson Mar 10, 2022 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RikkiGibson Mar 10, 2022 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv commented Mar 10, 2022

Uh oh!

jaredpar Mar 10, 2022 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

template: eng/pipelines/test-windows-job.yml

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv commented Mar 10, 2022

Uh oh!

azure-pipelines bot commented Mar 10, 2022

Uh oh!

Uh oh!

jaredpar Mar 15, 2022 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RikkiGibson Mar 15, 2022 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

jcouv commented Mar 9, 2022 •

edited

Loading

jcouv Mar 10, 2022 •

edited

Loading

RikkiGibson Mar 10, 2022 •

edited by jcouv

Loading

RikkiGibson Mar 10, 2022 •

edited by jcouv

Loading

jaredpar Mar 10, 2022 •

edited by jcouv

Loading

jaredpar Mar 15, 2022 •

edited by jcouv

Loading

RikkiGibson Mar 15, 2022 •

edited by jcouv

Loading