Few adjustments for test id #5936
Conversation
764720a to
6d2ac0e
Compare
6d2ac0e to
5eba464
Compare
test/UnitTests/Microsoft.Testing.Extensions.UnitTests/TrxTests.cs
Outdated
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
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
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
test/UnitTests/Microsoft.Testing.Extensions.UnitTests/TrxTests.cs
Outdated
Show resolved
Hide resolved
src/Platform/Microsoft.Testing.Extensions.TrxReport/Hashing/XxHash128.cs
Show resolved
Hide resolved
src/Platform/Microsoft.Testing.Extensions.TrxReport/Hashing/XxHashShared.cs
Show resolved
Hide resolved
src/Platform/Microsoft.Testing.Extensions.TrxReport/Hashing/XxHash128.cs
Show resolved
Hide resolved
src/Platform/Microsoft.Testing.Extensions.TrxReport/Hashing/XxHashShared.cs
Outdated
Show resolved
Hide resolved
src/Platform/Microsoft.Testing.Extensions.TrxReport/Hashing/XxHashShared.cs
Outdated
Show resolved
Hide resolved
src/Platform/Microsoft.Testing.Extensions.TrxReport/Hashing/XxHash128.cs
Show resolved
Hide resolved
src/Platform/Microsoft.Testing.Extensions.TrxReport/Hashing/XxHash128.cs
Outdated
Show resolved
Hide resolved
nohwnd
left a comment
There was a problem hiding this comment.
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.
| { | ||
| var idProvider = new TestIdProvider(); | ||
| byte[] hash = XxHash128.Hash(Encoding.Unicode.GetBytes(data)); | ||
| return new Guid(hash); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@nohwnd I'm not sure exactly what we should put into the "version" part of the Guid.
There was a problem hiding this comment.
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.
src/Adapter/MSTestAdapter.PlatformServices/ObjectModel/UnitTestElement.cs
Outdated
Show resolved
Hide resolved
src/Platform/Microsoft.Testing.Extensions.TrxReport/Hashing/BinaryPrimitives.cs
Show resolved
Hide resolved
test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/XxHash128Tests.cs
Show resolved
Hide resolved
|
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. |
Related to #1285 and #5762