Skip to content

Conversation

@CaseyHillers
Copy link
Contributor

@CaseyHillers CaseyHillers commented Jun 10, 2021

Fixes flutter/flutter#82705
Fixes flutter/flutter#88185 - ci is no longer using flutter upgrade
Fixes flutter/flutter#89709

Changes

Cocoon

  • Remove CirrusGraphQLClient
    • The same as GraphQLClient
  • Migrate BuildBucket id from int to string
    • This is because json_serializable now exports it as a JSON number which is not safe + there's no use of it as an int
  • Created test entity_generators.dart with common utility functions

Fakes

  • FakeRequestHandler
  • FakeBigqueryService (rename of BigqueryServiceMock)

Mocks

  • Null safety requires generated the mock model code (this is under test/src/utilities/mocks.dart)
  • Any tests which were able to assume a null value returned were updated to include a stub

google_apis

  • Api dropped from various classes

GraphQL

  • Add lower level library to enable access to moved datastructures
  • Update QueryResult errors to the renamed exceptions
    • Add sourcein tests
  • Parse GraphQL strings to DocumentNode

http

  • Move string urls to Uri
  • Encourage use of package:http

metrics_center

  • Add commit timestamp

Test Plan

  • dart test passes
  • Local server passes all usable functionality (serves content, views dashboard, retries tasks)
  • 1% prod traffic test
  • 10% prod traffic test
  • 25% prod traffic test
  • 50% prod traffic test
  • Full roll out

Preview

https://testchillers-dot-flutter-dashboard.appspot.com

@google-cla google-cla bot added the cla: yes label Jun 10, 2021
@CaseyHillers CaseyHillers force-pushed the backend-nnbd branch 3 times, most recently from 1cf175e to 7b117c8 Compare August 13, 2021 18:34
@CaseyHillers CaseyHillers marked this pull request as ready for review August 13, 2021 18:47
@CaseyHillers
Copy link
Contributor Author

The current blocker on this is rolling http to the new version in the tests. I'm looking into how to fix fake_http.dart

@CaseyHillers CaseyHillers changed the title [app_dart] Migrate packages to null safety [app_dart] Migrate to null safety Aug 27, 2021
Copy link
Contributor Author

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

@CaseyHillers
Copy link
Contributor Author

There's 21 tests failures remaining that require non-trivial fixes.

  • 6 have to do with the flaky issues using a generic function on a mock, which isn't supported (it was able to rely on a null value)
  • 9 have to do with some weird mockito setup and not using a provided stub (github webhook)
  • 6 have to do with previously null bigquery values in refresh chromebot status

@godofredoc
Copy link
Contributor

There's 21 tests failures remaining that require non-trivial fixes.

  • 6 have to do with the flaky issues using a generic function on a mock, which isn't supported (it was able to rely on a null value)
  • 9 have to do with some weird mockito setup and not using a provided stub (github webhook)
  • 6 have to do with previously null bigquery values in refresh chromebot status

Is this something that we can crowdsource? it is blocking already several changes to the backend.

@CaseyHillers
Copy link
Contributor Author

CaseyHillers commented Sep 10, 2021

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:

  1. get-status DONE
  2. refresh-chromebot-status DONE
    3. github-webhook DONE
  3. file-flaky-issue SKIPPED
  4. Add mocks.mocks.dart to the license check DONE

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.

@CaseyHillers
Copy link
Contributor Author

There's one other issue, and I'm trying to find a solution:

when(mock.methodThatReturnsStream).thenAnswer((_) => Stream.empty());

Mockito will not return Stream.emtpy() in the tests, or store it as a stub. I asked in #hackers-test to get ideas, but from the documentation it sounds like this should work.

@CaseyHillers
Copy link
Contributor Author

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.

@CaseyHillers
Copy link
Contributor Author

I'm disabling the flaky issue tests as they're non-critical and blocking this PR. flutter/flutter#90139

@CaseyHillers
Copy link
Contributor Author

I'm going to squash these commits to rebase on top of master (\cc @keyonghan @godofredoc feel free to review now)

Copy link
Contributor

@keyonghan keyonghan left a 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?

@CaseyHillers
Copy link
Contributor Author

CaseyHillers commented Sep 15, 2021

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

@CaseyHillers
Copy link
Contributor Author

Another AI is I need to revisit the model classes and mark everything as nullable, otherwise dart:mirrors doesn't work properly

@CaseyHillers
Copy link
Contributor Author

The only issue i'm noticing is BuildBucketClient._postRequest is always failing with BuildBucketException: [400]: could not decode body: illegal base64 data at input byte 0.

I'm adding some logs to see what the message looks like, and see if there are any missing headers.

@CaseyHillers
Copy link
Contributor Author

The only issue i'm noticing is BuildBucketClient._postRequest is always failing with BuildBucketException: [400]: could not decode body: illegal base64 data at input byte 0.

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)

Copy link
Contributor Author

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

@CaseyHillers
Copy link
Contributor Author

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

@CaseyHillers CaseyHillers changed the base branch from master to main September 22, 2021 22:36
@CaseyHillers CaseyHillers changed the base branch from main to master September 22, 2021 22:44
* [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
@CaseyHillers CaseyHillers changed the base branch from master to main September 22, 2021 22:58
Copy link
Contributor

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.

Copy link
Contributor Author

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

Casey Hillers added 3 commits September 22, 2021 21:04
@CaseyHillers
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

3 participants