Fix the flakiness of new BinaryFormatter tests#103476
Merged
adamsitnik merged 6 commits intodotnet:mainfrom Jun 17, 2024
Merged
Fix the flakiness of new BinaryFormatter tests#103476adamsitnik merged 6 commits intodotnet:mainfrom
adamsitnik merged 6 commits intodotnet:mainfrom
Conversation
…ont as it enumerates all properties and may change the contents of the fields
…ches to be the same object, as we want to compare exactly the same object on purpose
…uated SortedList<TKey, TValue>.keyList and valueList
…e it which may cause hard to repro failures based on the order of executed tests
adamsitnik
added
the
binaryformatter-migration
ghost
added
the
needs-area-label
adamsitnik
added
area-System.Resources
and removed
needs-area-label
Contributor
|
Tagging subscribers to this area: @dotnet/area-system-resources |
This was referenced Jun 14, 2024
stephentoub
approved these changes
Jun 14, 2024
Member
stephentoub
left a comment
stephentoub
left a comment
There was a problem hiding this comment.
We just got "lucky" that we didn't hit this before in existing tests?
Member
Author
These tests were not caching the input for all tests in a static field, so the mutation could not affect other test invocations |
Member
Author
|
I am going to close and re-open the PR hoping that the CI leg that should have been cancelled is going to change the status:
|
This was referenced Jun 15, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Recently I've noticed that the new BF tests fail from time to time, claiming that the blobs stored in the tests became outdated.
Sample log entry:
Details
It was very hard to reproduce this problem, so my guess was that it was dependent on test execution order.
I did some debugging and it turned out that
EqualityExtensions.CheckEqualsenumerates over all properties of given type.Some of the properties might be lazy evaluated and stored in a field. BF is storing all the fields. It means that serializing given object instance before and after passing it to
EqualityExtensions.CheckEqualsmay produce different blobs!The fix was to call that method up-front and update the blob and moreover stop caching the test input, as it may clearly get mutated by the tests (this explains why the original BF tests were not flaky).
fixes #103367
fixes #103478