Skip to content

Few adjustments for test id#5936

Merged
Youssef1313 merged 20 commits intorel/4.0from
dev/ygerges/id
Jul 9, 2025
Merged

Few adjustments for test id#5936
Youssef1313 merged 20 commits intorel/4.0from
dev/ygerges/id

Conversation

@Youssef1313
Copy link
Copy Markdown
Member

@Youssef1313 Youssef1313 commented Jul 4, 2025 •
edited
Loading

Related to #1285 and #5762

@Youssef1313 Youssef1313 added this to the 4.0.0 milestone Jul 4, 2025
Youssef1313
Youssef1313 commented Jul 4, 2025
View reviewed changes
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jul 4, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 24.73246% with 633 lines in your changes missing coverage. Please review.

Project coverage is 62.14%. Comparing base (9beecce) to head (124493c).
Report is 47 commits behind head on rel/4.0.

Files with missing lines Patch % Lines
...sting.Extensions.TrxReport/Hashing/XxHashShared.cs 20.95% 445 Missing ⚠️
....Testing.Extensions.TrxReport/Hashing/XxHash128.cs 29.06% 183 Missing ⚠️
...er.PlatformServices/ObjectModel/UnitTestElement.cs 33.33% 4 Missing ⚠️
...ting.Extensions.TrxReport/Hashing/BitOperations.cs 66.66% 1 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff             @@
##           rel/4.0    #5936      +/-   ##
===========================================
- Coverage    63.15%   62.14%   -1.02%     
===========================================
  Files          574      578       +4     
  Lines        31462    32262     +800     
===========================================
+ Hits         19869    20048     +179     
- Misses       11593    12214     +621
Flag Coverage Δ
Debug 62.14% <24.73%> (-1.02%) ⬇️
integration 62.14% <24.73%> (-1.03%) ⬇️
production 62.14% <24.73%> (-1.02%) ⬇️
unit 62.14% <24.73%> (-1.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...g.Extensions.TrxReport/Hashing/BinaryPrimitives.cs 100.00% <100.00%> (ø)
...ft.Testing.Extensions.TrxReport/TrxReportEngine.cs 90.94% <100.00%> (-0.16%) ⬇️
...ting.Extensions.TrxReport/Hashing/BitOperations.cs 66.66% <66.66%> (ø)
...er.PlatformServices/ObjectModel/UnitTestElement.cs 65.97% <33.33%> (ø)
....Testing.Extensions.TrxReport/Hashing/XxHash128.cs 29.06% <29.06%> (ø)
...sting.Extensions.TrxReport/Hashing/XxHashShared.cs 20.95% <20.95%> (ø)

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Youssef1313
Youssef1313 commented Jul 7, 2025
View reviewed changes
nohwnd
nohwnd reviewed Jul 8, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@nohwnd nohwnd left a comment

Choose a reason for hiding this comment

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

I would like to see way more tests, especially some that compare guids based on real system.io.hashin.xx128hash with our patched version, both for making sure that the hashing function is not broken with the code changes, and for future reference, because same as sha1 hashes, the way the hashes are generated should not change among the stated version in guid, to not break testId consumers.

src/Adapter/MSTestAdapter.PlatformServices/ObjectModel/UnitTestElement.cs
{
var idProvider = new TestIdProvider();
byte[] hash = XxHash128.Hash(Encoding.Unicode.GetBytes(data));
return new Guid(hash);
Copy link
Copy Markdown
Member

@nohwnd nohwnd Jul 8, 2025

Choose a reason for hiding this comment

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

This should form a v8 uuid https://www.rfc-editor.org/rfc/rfc9562.html#name-uuid-version-8 and I think it would be reasonable to reserve 4 bits for the version / origin.

Copy link
Copy Markdown
Member Author

@Youssef1313 Youssef1313 Jul 8, 2025

Choose a reason for hiding this comment

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

@nohwnd I'm not following here.

Copy link
Copy Markdown
Member

@nohwnd nohwnd Jul 8, 2025

Choose a reason for hiding this comment

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

You get "random" bytes and mash them into a guid. That is not what should be done, even though the current implementation in vstest and mstest does that.

Instead it should form a correct v8 guid that uses the data and embeds our version, so we can recognize in the future by which hashing algorithm we generated that given guid, when replacing it with newer version for example. Or with new way of generating the guid.

Copy link
Copy Markdown
Member Author

@Youssef1313 Youssef1313 Jul 9, 2025

Choose a reason for hiding this comment

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

@nohwnd I'm not sure exactly what we should put into the "version" part of the Guid.

Copy link
Copy Markdown
Member

@nohwnd nohwnd Jul 9, 2025

Choose a reason for hiding this comment

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

right now that would be 1, then once we make changes to the data that we put into the guid it should be 2, or when we change hash. basically a way for us to map the bits to some table and be able to tell that they are not the same version, so in theory azdo can map them from 1 to the other.

Copy link
Copy Markdown
Member

@nohwnd nohwnd Jul 9, 2025

Choose a reason for hiding this comment

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

moved to #5957

@nohwnd nohwnd mentioned this pull request Jul 9, 2025
Closed
@nohwnd
Copy link
Copy Markdown
Member

nohwnd commented Jul 9, 2025

Approved, feel free to merge before the guid change if you are okay with the info on the unresolved discussions.

Created a new Issue for that #5957.

@Youssef1313 Youssef1313 merged commit 53db8f8 into rel/4.0 Jul 9, 2025
8 checks passed
@Youssef1313 Youssef1313 deleted the dev/ygerges/id branch July 9, 2025 15:15
This was referenced Jul 10, 2025
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nohwnd nohwnd nohwnd approved these changes

+1 more reviewer

@jkotas jkotas jkotas left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Breaking ‼️

Projects

None yet

Milestone

4.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@Youssef1313 @codecov-commenter @nohwnd @jkotas