-
Notifications
You must be signed in to change notification settings - Fork 40
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
AF-3801 fix reporter runtime error in Dart 2.1.0 #287
Conversation
Security InsightsNo security relevant content was detected by automated scans. Action Items
Questions or Comments? Reach out on Slack: #support-infosec. |
lib/src/reporter.dart
Outdated
// This may look like a strange conditional, but please leave it. | ||
// Somehow when using the tearoff `reporter.log` the `quiet` variable | ||
// is null, which causes a runtime error when running tests in Dart 2.1.0 | ||
if (quiet == true && shout != true) return; |
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.
Could it be that Reporter
isn't getting passed both arguments during intiialization?
dart_dev/lib/src/reporter.dart
Line 22 in 7eee6da
Reporter reporter = new Reporter(); |
Based on the existing code,
dart_dev/lib/src/reporter.dart
Lines 30 to 35 in a3b36d0
bool color = true; | |
bool quiet = false; | |
Reporter({bool this.color, bool this.quiet}); | |
String colorBlue(String message) => _color(_blue, message); |
, I would expect
new Reporter()
to have both fields be null.
I feel like it should just be:
bool color;
bool quiet;
Reporter({bool this.color = true, bool this.quiet = false});
Could it be that it's only showing up in testing because this is being run in checked mode?
- default constructor values - extra null checks
lib/src/reporter.dart
Outdated
@@ -30,7 +30,11 @@ class Reporter { | |||
bool color = true; | |||
bool quiet = false; | |||
|
|||
Reporter({bool this.color, bool this.quiet}); | |||
Reporter({this.color: true, this.quiet: false}) { |
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.
#nit these defaults aren't necessary if we have null-awares below
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.
I left them in so that when calling the constructor, the IDE would tell you what the defaults are.
lib/src/reporter.dart
Outdated
@@ -30,7 +30,11 @@ class Reporter { | |||
bool color = true; | |||
bool quiet = false; |
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.
#nit these defaults aren't necessary
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.
Totally. I'll remove them.
lib/src/reporter.dart
Outdated
if (quiet && !shout) return; | ||
// Ensure that even if quiet or shout are null, the conditional | ||
// will result in a boolean | ||
if (quiet == true && shout != true) { |
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.
I don't think we should have to worry about this case; these should never be null unless someone explicitly sets them to null, in which case the program should probably fail.
+1 |
QA +1
@Workiva/release-management-p |
Overview
Under Dart 2.1.0, when running tests that use
reporter.log
. Thequiet
variable would be null somehow and cause a runtime error in the conditional check in the_log
method.I imagine this is caused by something like checked mode when running tests. I could not reproduce this runtime error when running a similar thing on the command line (prod mode).
An example failing build due to this error: https://ci.webfilings.com/build/1585120
Changes
Testing