-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Removed no-shuffle tag and fixed order dependency in daemon_test.dart #98970
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
Merged
Conversation
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
packages/flutter_tools/test/commands.shard/hermetic/daemon_test.dart
Outdated
Show resolved
Hide resolved
jonahwilliams
approved these changes
Feb 23, 2022
Contributor
jonahwilliams
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…shard/hermetic/daemon_test.dart Co-authored-by: Jonah Williams <[email protected]>
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Feb 23, 2022
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/plugins
that referenced
this pull request
Feb 23, 2022
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Feb 23, 2022
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Feb 23, 2022
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Feb 23, 2022
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Feb 23, 2022
clocksmith
pushed a commit
to clocksmith/flutter
that referenced
this pull request
Mar 8, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR fixed the problem that has prevented daemon_test.dart being shuffled. Part of #85160.
The issues
Two issues were found related to three tests. No test failed for me using the reported seed 1000. But it failed on seed 123.
Test A: 'notifyingLogger outputs trace messages in verbose mode'
Test B: 'notifyingLogger ignores trace messages in non-verbose mode'
Test C: 'notifyingLogger buffers messages sent before a subscription'
First Issue - Order Dependency
All three tests use the bufferLogger variable that is initialized inside group 'daemon'. As these
three tests are placed outside that group, they were failing if run before the group 'daemon'.
Second Issue - Broken Test and Order Dependency
As far as I can see, the test B has never been working as intended.
The line 'logger.printTrace('test');' should not be logged in bufferLogger in non-verbose mode. The test expected it to be logged.
It was working though because test A was logging the same string to bufferLogger and leaving the log behind for later tests. But if test B was run before test A, it would fail.
The fix
First issue
Create a new test group called 'notifyingLogger' to initialize bufferLogger and then clear it, after each of these tests. The bufferLogger variable was isolated in each group to avoid future mistakes.
Second Issue
Test B was changed to expect the bufferLogger to be empty.
Note
I could not reproduce any test failings on randomize ordering seed 1000. I found these issues using seed 123.
Any reviewer should consider testing seed 1000 to make sure there is no race condition that I cannot reproduce.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on [Discord].