-
Notifications
You must be signed in to change notification settings - Fork 100
[app_dart] Migrate to null safety #1246
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
Conversation
1cf175e to
7b117c8
Compare
b3588eb to
ccdbcd5
Compare
|
The current blocker on this is rolling http to the new version in the tests. I'm looking into how to fix |
b30dde3 to
3d647cb
Compare
3d647cb to
ab08417
Compare
147b228 to
297d713
Compare
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'm not sure why this is now surfacing as an issue in the new Dart version.
envelope.message.data has always been a base64 encoded string of the json, however it seems json decode removed the base64 decode aspect
|
There's 21 tests failures remaining that require non-trivial fixes.
|
Is this something that we can crowdsource? it is blocking already several changes to the backend. |
|
That would be great as I need to head out for an appointment. Feel free to add commits to this PR for the fixes. The remaining handlers can be crowdsourced:
Once these are done, we'll need to squash the PR into one commit, and rebase with tip of tree, fixing the issues there. Then we can format and do a prod test. |
|
There's one other issue, and I'm trying to find a solution: when(mock.methodThatReturnsStream).thenAnswer((_) => Stream.empty());Mockito will not return |
|
I'm working on the last issue regarding the mock generics. I was able to find https://github.com/dart-lang/mockito/blob/master/NULL_SAFETY_README.md#fallback-generators and i'm working to implement it now. |
|
I'm disabling the flaky issue tests as they're non-critical and blocking this PR. flutter/flutter#90139 |
|
I'm going to squash these commits to rebase on top of master (\cc @keyonghan @godofredoc feel free to review now) |
12c0fed to
d51e538
Compare
keyonghan
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.
I wouldn't expect this to break main features, but can we validate the PR in a test version before merging?
Yes. My expectation is the review will take longer than the prod traffic test, so I wanted to get early eyes on it. I'm also debugging issues through the local dart server. I added a preview link to the PR description |
|
Another AI is I need to revisit the model classes and mark everything as nullable, otherwise dart:mirrors doesn't work properly |
|
The only issue i'm noticing is I'm adding some logs to see what the message looks like, and see if there are any missing headers. |
After some more debugging, it's not always failing. For some reason it can pass on the exact same call (like refresh-chromebot-status querying all the same builds, but then fail on push-build-status-to-github) |
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 increased ci.yaml check failures to warnings to surface issues in the logs
|
I did a prod traffic test with service/buildbucket.dart using the original dart:io calls, and that is still returning a malformed response. I suspect there is an issue in the model/buildbucket.dart migration |
* [test_utilities] Move to Flutter master * update generated code * dart migrate * generate mocks * switch buildbucket types from int64 to string * fix services * check licenses skips mocks.mocks.dart * create github.postJSON fallback generator * fix foundation + request handling * fix request handlers * disable flaky issue tests
cbccc74 to
e6a6e48
Compare
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.
You'll need to transform the encoded json string to base64.
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.
This was it, thank you! I'm not sure why this regressed with this PR
* Disable github rate limit * Increase ci.yaml check failures as warnings in logs
a441bfa to
1b11094
Compare
|
I'm submitting this, but there's a known issue where the waiting for tree to green graphql queries can timeout ~20% of the time. I filed to flutter/flutter#90640 to track |
Fixes flutter/flutter#82705
Fixes flutter/flutter#88185 - ci is no longer using
flutter upgradeFixes flutter/flutter#89709
Changes
Cocoon
CirrusGraphQLClientGraphQLClientFakes
Mocks
google_apis
GraphQL
QueryResulterrors to the renamed exceptionssourcein testsDocumentNodehttp
Uripackage:httpmetrics_center
Test Plan
dart testpassesPreview
https://testchillers-dot-flutter-dashboard.appspot.com