Add basic instrumentation tests for union declarations - #84637
Conversation
|
Azure Pipelines: Successfully started running 1 pipeline(s). 1 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
Adds initial coverage ensuring C# union-generated members interact correctly with various emit-time instrumentation modes (stack overflow probing, module cancellation, local state tracing, and dynamic analysis/test-coverage instrumentation).
Changes:
- Add
UnionDeclaration_01tests to multiple instrumentation test suites to validate which synthesized union members are/aren’t instrumented. - Update local
CompileAndVerifyhelpers in the Emit2 instrumentation suites to acceptCSharpTestSource, enabling collection-expression sources while still applying the suite’sEmitOptions. - Add a dynamic analysis test asserting union-generated members remain uninstrumented under test-coverage instrumentation in both Release and Debug.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Compilers/CSharp/Test/Emit2/Emit/RuntimeProbing/StackOverflowProbingTests.cs | Adds a union declaration test validating stack overflow probing instrumentation behavior. |
| src/Compilers/CSharp/Test/Emit2/Emit/RuntimeProbing/ModuleCancellationTests.cs | Adds a union declaration test validating module cancellation instrumentation behavior; also updates helper signature. |
| src/Compilers/CSharp/Test/Emit2/Emit/LocalStateTracing/LocalStateTracingTests.cs | Adds a union declaration test validating local state tracing instrumentation behavior; updates helper signatures. |
| src/Compilers/CSharp/Test/Emit/Emit/DynamicAnalysis/DynamicInstrumentationTests.cs | Adds a union declaration test asserting union-generated members are not test-coverage instrumented (Release + Debug). |
|
@RikkiGibson, @jjonescz Please review |
| .True("_ = (TestUnion)123;") | ||
| .True("Microsoft.CodeAnalysis.Runtime.Instrumentation.FlushPayload();"); | ||
|
|
||
| var verifier = CompileAndVerify(source, references: [comp0Ref], options: TestOptions.ReleaseExe); |
There was a problem hiding this comment.
Should we be using the locally defined CompileAndVerify helper which passes
emitOptions: EmitOptions.Default.WithInstrumentationKinds(ImmutableArray.Create(InstrumentationKind.TestCoverage))instead of the base class helper?
There was a problem hiding this comment.
Good catch! Overloading base helpers is fragile because the new overloads might not be used by a specific call site.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/Compilers/CSharp/Test/Emit2/Emit/LocalStateTracing/LocalStateTracingTests.cs:210
CompileAndVerifyFailstakes anexpectedOutputparameter but then always passesexpectedOutput: nullto the underlyingCompileAndVerify, so the parameter is effectively ignored (and can be misleading when trying to diagnose failures). Since there are no call sites in the repo, consider removing the parameter (and the conditional exe/dll option selection) to make the helper’s intent unambiguous.
// Only used to diagnose test verification failures (rename CompileAndVerify to CompileAndVerifyFails and rerun).
public CompilationVerifier CompileAndVerifyFails(CSharpTestSource source, string? ilVerifyMessage = null, string? expectedOutput = null)
=> CompileAndVerify(
source,
options: (expectedOutput != null) ? TestOptions.UnsafeDebugExe : TestOptions.UnsafeDebugDll,
emitOptions: s_emitOptions,
verify: Verification.Fails,
targetFramework: s_targetFramework,
expectedOutput: null);
Microsoft Reviewers: Open in CodeFlow