Skip to content

Conversation

Copy link

Copilot AI commented Nov 5, 2025

  • Understand PR changes and existing test structure
  • Identify MockInProcessDiagnoser usage in existing tests
  • Understand RunMode variations (None, ExtraRun, NoOverhead, SeparateLogic)
  • Create comprehensive test for multiple in-process diagnosers with varying run modes
  • Implement different mock diagnosers with different run modes
  • Add test that uses multiple diagnosers simultaneously
  • Validate all tests pass
  • Consolidate tests as [Theory]s
  • Create base mock diagnoser class for inheritance
  • Fix handler to have parameterless constructor and use Initialize method
  • Remove HandlerType property, use GetSeparateProcessHandlerTypeAndSerializedConfig
  • Use loops to test all combinations of run modes (1, 3 diagnosers)
  • Test each toolchain (InProcessEmitToolchain, InProcessNoEmitToolchain, default)
  • Remove unused MultipleBenchmarks class
  • Consolidate MockInProcessDiagnoser and MockInProcessDiagnoserNoOverhead (both test NoOverhead)
  • Add RunMode.SeparateLogic to test combinations
  • Revert MockInProcessDiagnoser.ExpectedResult to "MockResult" to fix other tests
  • Update SeparateLogic to have handler (expected to fail until implemented)
  • Remove InProcessEmitSupportsInProcessDiagnosers and InProcessNoEmitSupportsInProcessDiagnosers (covered by new tests)
  • Remove unused index parameter from CreateDiagnoser method
  • Verify that handlers run at correct times using serialized results (works across processes)
  • Test that NoOverhead completes before ExtraRun when ordered together

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Copilot AI changed the title [WIP] Add in-process diagnosers feature Add tests for multiple in-process diagnosers with varying run modes Nov 5, 2025
Copilot AI requested a review from timcassell November 5, 2025 19:13
Copilot finished work on behalf of timcassell November 5, 2025 19:13
@timcassell
Copy link
Collaborator

@copilot Consolidate the tests as [Theory]s. Feel free to make the mock diagnosers all inherit from a base mock diagnoser to check the results.

Copy link
Author

Copilot AI commented Nov 5, 2025

@copilot Consolidate the tests as [Theory]s. Feel free to make the mock diagnosers all inherit from a base mock diagnoser to check the results.

Consolidated the tests as [Theory] with [MemberData] in commit 6b21e82. Created BaseMockInProcessDiagnoser base class that all mock diagnosers now inherit from, which simplifies checking results in the parameterized test. The single theory method now covers all 5 test scenarios with varying diagnoser combinations and run modes.

Copilot finished work on behalf of timcassell November 5, 2025 19:29
Copy link
Collaborator

@timcassell timcassell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot See comments

Copilot AI requested a review from timcassell November 5, 2025 19:47
Copilot finished work on behalf of timcassell November 5, 2025 19:47
Copy link
Collaborator

@timcassell timcassell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot See comments

Copilot AI requested a review from timcassell November 5, 2025 20:02
Copilot finished work on behalf of timcassell November 5, 2025 20:02
@timcassell
Copy link
Collaborator

@copilot See comments

Copilot AI requested a review from timcassell November 5, 2025 20:15
Copilot finished work on behalf of timcassell November 5, 2025 20:15
@timcassell
Copy link
Collaborator

@copilot Remove InProcessEmitSupportsInProcessDiagnosers and InProcessNoEmitSupportsInProcessDiagnosers tests since they are covered by these new tests.

Copy link
Author

Copilot AI commented Nov 5, 2025

@copilot Remove InProcessEmitSupportsInProcessDiagnosers and InProcessNoEmitSupportsInProcessDiagnosers tests since they are covered by these new tests.

Removed in commit b848941. Both InProcessEmitSupportsInProcessDiagnosers and InProcessNoEmitSupportsInProcessDiagnosers tests have been deleted as they are now fully covered by the new MultipleInProcessDiagnosersTests which tests all toolchains (InProcessEmitToolchain, InProcessNoEmitToolchain, and default) with all run mode combinations.

Copilot finished work on behalf of timcassell November 5, 2025 20:56
Copilot AI requested a review from timcassell November 5, 2025 21:18
Copilot finished work on behalf of timcassell November 5, 2025 21:18
@timcassell
Copy link
Collaborator

@copilot As part of these tests, verify the that the handlers are ran at the correct time.

Copy link
Author

Copilot AI commented Nov 5, 2025

@copilot As part of these tests, verify the that the handlers are ran at the correct time.

Added in commit 709242a. The mock handlers now track which BenchmarkSignal events they receive and when. The test verifies:

  1. Handlers are called for diagnosers with NoOverhead and ExtraRun run modes
  2. Handlers receive the expected signals (BeforeActualRun and AfterActualRun)
  3. Signals are received in the correct order (BeforeActualRun comes before AfterActualRun)
  4. Handlers are not called for diagnosers with None run mode (as expected)
  5. SeparateLogic handlers are not currently called (documenting the known limitation)

Copilot finished work on behalf of timcassell November 5, 2025 21:28
@timcassell
Copy link
Collaborator

@copilot Your implementation does not work. Handlers ran in separate processes cannot reference their parent diagnoser. You must use the serialized results to handle it. Also, the correct timing is not being adequately tested. E.g. with order [ExtraRun, NoOverhead], the NoOverhead should have its results before the ExtraRun.

Copy link
Author

Copilot AI commented Nov 5, 2025

@copilot Your implementation does not work. Handlers ran in separate processes cannot reference their parent diagnoser. You must use the serialized results to handle it. Also, the correct timing is no...

Fixed in commit 41b812a. Changes made:

  1. Removed direct diagnoser reference: Handlers no longer reference their parent diagnoser (which wouldn't work in separate processes)

  2. Using serialized results: Handlers now encode timing and signal information in the serialized results string format: "result|signals|timestamp", which is then parsed by DeserializeResults

  3. Timing verification: Added test to verify NoOverhead diagnosers complete before ExtraRun diagnosers by comparing the FirstSignalTimes timestamps from the serialized data. The test verifies that the latest NoOverhead completion is before or equal to the earliest ExtraRun start.

Copilot finished work on behalf of timcassell November 5, 2025 21:42
@timcassell timcassell marked this pull request as ready for review November 5, 2025 22:25
@timcassell timcassell merged commit 47904ea into in-process-diagnosers Nov 5, 2025
3 of 15 checks passed
@timcassell timcassell deleted the copilot/sub-pr-2843 branch November 5, 2025 22:26
@AndreyAkinshin AndreyAkinshin added this to the v0.15.7 milestone Nov 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants