Skip to content

Conversation

@Piinks
Copy link
Contributor

@Piinks Piinks commented Mar 5, 2020

  • Noticed in the logs that there was some unnecessary work going on (no need to query cirrus if the last known gold status was completed), rearranged the logic to make it leaner.
  • Also updated the messaging, Switching to Gold Status Check flutter#51968 is my validator and the comment wasn't as clear as it could be.
  • Moves cron job to every 5 minutes instead of 3
  • Re-introduces considering what tests are running to reduce gold checks
    • In Adding gold status check to framework PRs #651 this was considered liable to break, but I have implemented it differently now. Instead of only checking framework tests, it is just using the presence of a framework test to indicate that golden file tests are executing for this PR
  • Reduced comment noise after initial feedback

lastUpdate.head == pr.head.sha) {
log.debug('Completed status already reported for this commit.');
// We have already seen this commit and it is completed.
continue;
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 is the case for an overwhelming number of PRs.

@Piinks Piinks requested review from dnfield and keyonghan March 5, 2020 23:58
@Piinks Piinks requested a review from CaseyHillers March 6, 2020 00:28
@Piinks
Copy link
Contributor Author

Piinks commented Mar 6, 2020

I have added a few more changes after touching base with @kjlubick about the amount of traffic on the gold endpoint
@dnfield had some concerns earlier (#651) about checking for framework tests, I've reintroduced this in a different way that is more reliable.
I've also moved the cron job to 5 minutes from 3.

PTAL. :)

@Piinks
Copy link
Contributor Author

Piinks commented Mar 6, 2020

I have to fix the formatting, but oddly dartfmt is different on my laptop. 😖

Comment on lines +81 to +85
final List<dynamic> cirrusChecks =
await queryCirrusGraphQL(pr.head.sha, cirrusClient, log, 'flutter');
for (dynamic check in cirrusChecks) {
final String status = check['status'];
final String taskName = check['name'];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is reminding me that we should turn off avoid_as and set implicit-casts: false.

I was going to suggest some ways to avoid dynamic typing here but we can save that for a PR that just changes those lints around.

Copy link
Contributor

@dnfield dnfield left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Piinks
Copy link
Contributor Author

Piinks commented Mar 6, 2020

This is all fixed now. Sorry for the noise. :)

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants