Skip to content

Delete TRX reordering logic from TestingPlatformAutoRegisteredExtensions#7454

Draft
Youssef1313 wants to merge 1 commit intomainfrom
dev/ygerges/remove-reorder
Draft

Delete TRX reordering logic from TestingPlatformAutoRegisteredExtensions#7454
Youssef1313 wants to merge 1 commit intomainfrom
dev/ygerges/remove-reorder

Conversation

@Youssef1313
Copy link
Member

@Youssef1313 Youssef1313 commented Feb 22, 2026

I cannot see a reason why we need this reordering.

Copilot AI review requested due to automatic review settings February 22, 2026 14:55
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR removes the special-case reordering logic in the MSBuild task that generates the SelfRegisteredExtensions source, so builder hooks are emitted in the order MSBuild provides them.

Changes:

  • Removed the well-known TRX hook GUID constant and the Reorder helper.
  • Updated code generation to use SelfRegisteredExtensionsBuilderHook directly (no reordering step).

@Youssef1313 Youssef1313 marked this pull request as ready for review February 22, 2026 15:55
Copy link
Member

@Evangelink Evangelink left a comment

Choose a reason for hiding this comment

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

It definitely was done on purpose but I don't recall the use case. Maybe it's now un-needed because of other refactorings.

@MarcoRossignoli do you recall why it was done?

@Youssef1313
Copy link
Member Author

Youssef1313 commented Feb 23, 2026

Maybe it's now un-needed because of other refactorings.

That's my guess, but I don't have the full history and not sure why it was needed in the past and what changed since then. For history, https://github.com/microsoft/testanywhere/issues/1529

@Evangelink
Copy link
Member

Hum I wonder if that's linked to the updates to the MessageBus, now we correctly wait for drains so even if TRX is not registered last it can still get all messages.

@MarcoRossignoli
Copy link
Contributor

Because the code coverage has to update the trx with the coverage file, so we need to ensure that the trx is already on disk when the coverage end session kick it from the out of process extension.

@Youssef1313
Copy link
Member Author

@MarcoRossignoli Hmm, that's a good point.

I wonder if we are actually already broken today. We have two modes for TRX, one that runs in-proc and one that runs out-of-proc. We run TRX out-of-proc as a TestHostController extension only if --crash-dump is enabled.

That means:

  • TestHost finishes, including TRX generation, without receiving the file artifact.
  • Then, TestHostController finishes, publishes file artifact, but that's not considered by TRX extension.

That's just a theory and I need to make more testing to validate what's broken and what's not broken.

But generally speaking, that will be problematic for almost any reporter extension. So I wonder if we should do one or both of the following:

  1. Have a more generic way of ordering via MSBuild instead of special casing well-known GUIDs.
  2. Instead of re-ordering TRX to be last, we re-order CC to be first. That means any other extensions needing the coverage file can operate correctly. But that also means Coverlet need to be special cased as well, which is why doing 1 will also be beneficial.

@Youssef1313 Youssef1313 marked this pull request as draft February 24, 2026 17:59
@Youssef1313
Copy link
Member Author

Then, TestHostController finishes, publishes file artifact, but that's not considered by TRX extension.

At least for MS CC, it's the TestHost that publishes the file artifact. There is some IPC that goes between the TestHost and TestHostController where the TestHost asks the TestHostController "give me the artifacts", and then TestHost itself publishes.

I need to validate the opposite case then, where both CC and TRX are running out-of-proc

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.

4 participants