-
Notifications
You must be signed in to change notification settings - Fork 29.5k
[framework] dont null assert in _debugVerifyIllFatedPopulation #96551
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
[framework] dont null assert in _debugVerifyIllFatedPopulation #96551
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
@Hixie for lack of tests.... |
| assert(() { | ||
| Map<GlobalKey, Set<Element>>? duplicates; | ||
| for (final Element element in _debugIllFatedElements!) { | ||
| for (final Element element in _debugIllFatedElements ?? <Element>{}) { |
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.
const?
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.
done
|
If you want profile builds to be testable, I strongly recommend actually testing it. Right now they only work by pure chance (and sometimes don't work because we assume e.g. that kDebugMode matches when assert() is called). The only reason they work at all right now is that Google's internal tests rely on it in a couple of places. Several times we've broken those tests by mistake. If those tests go away (and I think that's quite possible) then the next time we break it we may not notice. |
|
Fair enough - I think the plan might be (@dnfield ) to remove support for profile mode with asserts. I think the challenge with testing this configuration is that we may not have any tooling in flutter/flutter that supports building profile with asserts enabled. (at least not since I last looked at the tool..) |
|
I filed #96552 to track |
|
I could write a devicelab test that asserts that it works, then @dnfield could later change it to assert failure. |
|
actually I don't need devicelab, I can use a similar hack to test_release. tests added |
|
Without the change to framework.dart, the test fails with: |
dnfield
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 until we can have a better way to enforce what modes unit tests are run in.
|
Mac_android run_release_test is hitting an infra failure, known issue? |
|
@CaseyHillers or @keyonghan might know (https://ci.chromium.org/ui/p/flutter/builders/try/Mac_android%20run_release_test/18/overview). I triggered a rerun. |
I filed #96672 to the ticket queue. If you don't need Mac_android coverage, feel free to ack it and submit it to see what CI says. I'll look into getting a few extra devices added to the try pool so one device doesn't control presubmit. |
|
cool, thank you @CaseyHillers ! |
If you run the tests in profile mode, this will fail due to the null check. Since we don't explicitly disallow profile mode in tests, I would prefer to keep it working - and if we decided to stop supporting it that can be an intention rather than accidental decision