Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add instance id to protocol with MTP #47764

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

mariam-abdulla
Copy link
Member

@mariam-abdulla mariam-abdulla commented Mar 20, 2025

This pull request introduces the InstanceId property across various classes and serializers in the dotnet-test project. The primary goal is to enhance the identification and tracking of test instances. The most important changes include updates to class definitions, record types, and serializers to incorporate the new InstanceId property.

Introduction of InstanceId Property:

  • CliConstants.cs: Added InstanceId constant to the HandshakeMessagePropertyNames class.
  • CustomEventArgs.cs: Added InstanceId property to DiscoveredTestEventArgs, TestResultEventArgs, and FileArtifactEventArgs classes. [1] [2]
  • DiscoveredTestMessages.cs, FileArtifactMessages.cs, TestResultMessages.cs: Updated record types to include InstanceId. [1] [2] [3]

License Header Updates:

  • Updated license headers in multiple files to reflect the .NET Foundation's licensing agreement. [1] [2] [3] [4] [5] [6]

Serializer Updates:

Object Field ID Updates:

  • ObjectFieldIds.cs: Added InstanceId field IDs for DiscoveredTestMessagesFieldsId, TestResultMessagesFieldsId, and FileArtifactMessagesFieldsId. [1] [2] [3]

Relates to #45927

@Copilot Copilot bot review requested due to automatic review settings March 20, 2025 13:07
@mariam-abdulla mariam-abdulla requested a review from a team as a code owner March 20, 2025 13:07
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-dotnet test untriaged Request triage from a team member labels Mar 20, 2025
Copy link
Contributor

@Copilot 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 adds a new InstanceId field to protocol messages for the .NET test infrastructure.

  • Updates constants and record definitions in IPC models.
  • Modifies both serializers and deserializers to handle InstanceId.
  • Updates event arguments, logging, and test application event handling to include InstanceId.

Reviewed Changes

Copilot reviewed 14 out of 15 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/Cli/dotnet/commands/dotnet-test/IPC/ObjectFieldIds.cs Reassigned field IDs to include InstanceId.
src/Cli/dotnet/commands/dotnet-test/CustomEventArgs.cs Added InstanceId properties to event args.
src/Cli/dotnet/commands/dotnet-test/IPC/Serializers/FileArtifactMessagesSerializer.cs Updated payload writing and reading to include InstanceId (note incorrect assignment in deserialization).
src/Cli/dotnet/commands/dotnet-test/IPC/Models/* Updated records to include InstanceId.
src/Cli/dotnet/commands/dotnet-test/IPC/Serializers/DiscoveredTestMessagesSerializer.cs Updated to read and write InstanceId in discovered tests payload.
src/Cli/dotnet/commands/dotnet-test/IPC/Models/TestResultMessages.cs and Serializers/TestResultMessagesSerializer.cs Updated to support InstanceId in test result messages.
src/Cli/dotnet/commands/dotnet-test/TestApplication*.cs Updated event handling and logging to incorporate InstanceId.
src/Cli/dotnet/commands/dotnet-test/CliConstants.cs Added new constant for InstanceId.
src/Cli/dotnet/commands/dotnet-test/TestingPlatformCommand.cs and TestApplicationEventHandlers.cs Removed ExecutionIdReceived event and updated logging for InstanceId.
Files not reviewed (1)
  • eng/Versions.props: Language not supported

@mariam-abdulla mariam-abdulla enabled auto-merge (squash) March 20, 2025 13:27
@mariam-abdulla mariam-abdulla removed the untriaged Request triage from a team member label Mar 20, 2025
@mariam-abdulla mariam-abdulla changed the title Aadd instance id to protocol with MTP Add instance id to protocol with MTP Mar 20, 2025
@mariam-abdulla mariam-abdulla disabled auto-merge March 20, 2025 16:31
Copy link
Contributor

@KalleOlaviNiemitalo KalleOlaviNiemitalo left a comment

Choose a reason for hiding this comment

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

It's not clear from the PR description what the InstanceId identifies and what creates it. I guess it's the same InstanceId in https://github.com/microsoft/testfx/blob/66b23c38620c1ad74ce3f42d3555defdd2703177/src/Platform/Microsoft.Testing.Platform/ServerMode/DotnetTest/DotnetTestConnection.cs#L30, i.e., it is generated by Microsoft.Testing.Platform and identifies the process and AppDomain that is running the tests; it is not unique for each test in the process.

@Youssef1313
Copy link
Member

Youssef1313 commented Mar 21, 2025

@KalleOlaviNiemitalo InstanceId identifies the test host (test app) that we are communicating with. It will play a role in a follow-up PR where we combine the use of ExecutionId/InstanceId/TestNodeUid to fix the behavior with Retry extension. Basically, consider you have a test app where you enable the retry extension, and the test fails on the first run but passes on the second run.

The first run and second run are sharing the same ExecutionId, but are having different InstanceId. We will use this information to understand that the test was retried. So, whenever we get a test result we will check "is it the same ExecutionId and same Uid, but different InstanceId? then that's a retry"

@mariam-abdulla mariam-abdulla enabled auto-merge (squash) March 21, 2025 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants