Pre-Submit Tryjobs for Flutter Gold #44474

Piinks · 2019-11-08T22:54:28Z

Description

This PR adds support for pre-submit tryjobs for Flutter Gold. This will allow for golden file tests to be run against baselines in pre-sbumit tests, generating visual diffs and enabling pre-submit triage of new or changed golden files.

Status:

https://skia-review.googlesource.com/c/buildbot/+/256018 + roll to CIPD needed for patchset_id
Update wiki

Related Issues

Fixes Better Triage Notification System for Gold #43995 somewhat in that triage will be handled before a change lands, removing the need to remember to visit the dashboard and triage.
Fixes Enable Tryjobs for Flutter Gold #45181

Tests

Updated related tests.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

No, this is not a breaking change.

Piinks · 2019-11-11T23:31:56Z

Blocked by

packages/flutter_goldens_client/lib/skia_client.dart

packages/flutter_goldens/lib/flutter_goldens.dart

Piinks · 2019-11-19T18:20:26Z

I'll have to make a framework change to actually trigger a framework test and see that this works... looking for a typo or something similar...

Piinks · 2019-11-19T19:11:12Z

packages/flutter/lib/src/animation/animation_controller.dart

-/// behavior.
+/// we reduce the duration and the corresponding number of frames for
+/// animations. This enum is used to allow certain [AnimationController]s to opt
+/// out of this behavior.


This change is a trivial 80 character line limit correction in keeping with the style guide, and to also trigger the relevant framework tests confirming tryjob execution.

Piinks · 2019-11-20T23:41:27Z

It looks like this is a bit flaky.
On the failing test shards, some tests have failed with this exception from

goldctl imgtest add ...

Exit code 1: Skia Gold tryjobAdd failure.
stdout:
stderr: Error running command: ''gold client not ready: Gold results fields invalid: Received nil
pointer for GoldResult. At jsonio.go:194 goldclient.go:452 goldclient.go:289 goldclient.go:279
cmd_imgtest.go:334 command.go:833 command.go:917 main.go:45 proc.go:203 asm_amd64.s:1357. At
goldclient.go:453 goldclient.go:289 goldclient.go:279 cmd_imgtest.go:334 command.go:833
command.go:917 main.go:45 proc.go:203 asm_amd64.s:1357''

When the exception was thrown, this was the stack:
#0      SkiaGoldClient.tryjobAdd (package:flutter_goldens_client/skia_client.dart:284:7)
<asynchronous suspension>
#1      FlutterPreSubmitFileComparator.compare (package:flutter_goldens/flutter_goldens.dart:313:23)
<asynchronous suspension>

The tryjob did show up on the dashboard here: https://flutter-gold.skia.org/changelists?page_size=50
Clicking on it results in a 404 error.

kjlubick · 2019-11-21T13:53:36Z

Around 22:07 UTC (last night), I see from the ingestion logs that files were uploaded to the right place and processed.
I see some entries in Firestore:

the ChangeList was made, and a Patchset 1 under it
6 cirrus TryJobs
oodles of TryJobResults

From that conclusion, data is (mostly) getting into Gold successfully.

The 404 is due to the frontend not linking properly to GitHub. If you click Triage that takes you to a page that should show the TryJobResults, but isn't - I'm investigating that.

kjlubick · 2019-11-21T13:58:00Z

Ah, the data in Firestore is slightly wrong - the TryJob and TryJobResults are incorrectly tagged as being "gerrit"

packages/flutter_goldens_client/lib/skia_client.dart

packages/flutter_goldens/lib/flutter_goldens.dart

dnfield · 2019-12-04T18:46:29Z

packages/flutter_goldens_client/lib/skia_client.dart

+    final File keys = workDirectory.childFile('keys.json');
+    final File failures = workDirectory.childFile('failures.json');


Is workDirectory in source control? Should we be putting this in some kind of temp directory that gets blown away after CI is done?

If I'm reading this right, it's coming from https://github.com/flutter/flutter/pull/44474/files#diff-259a52e89a62c0e0633f88f4307c22b7L142, which is not tracked by source control - is that right?

Is workDirectory in source control?

I am not sure what you mean by this.

I thought it was automatically blown away on ci since we start with a clean instance for each build. I can split it out further for ci/non-ci code paths, but I thought this would keep it from becoming a more complicated code path for those cases.

Yeah this should be fine. I was worried we were modifying something that the Flutter source control cared about with this.

dnfield · 2019-12-04T21:16:08Z

packages/flutter_goldens/lib/flutter_goldens.dart

+  /// The optional [suffix] argument is used by the
+  /// [FlutterSkiaGoldFileComparator] and the [FlutterPreSubmitFileComparator].
+  /// These [FlutterGoldenFileComparators] randomize their base directories to
+  /// maintain thread safety while using the `goldctl` tool.


Why aren't we just using createTempSync from package:file?

https://pub.dev/documentation/file/latest/file/Directory/createTempSync.html

When run locally, we want the files to be accessible to see diffs etc.

Ahh. I'm a bit curious about whether we'd hit collisions at some point using math.random, but it's probably fine.

I thought so too, but then realized we only run 2-3 processes concurrently on ci, and only ~130/~4500 tests are golden file tests, so I feel like a collision would be incredibly rare in how this is written.

dnfield

LGTM

Piinks · 2019-12-04T21:38:20Z

Thanks for the review @dnfield! I just need to update the wiki and this will be ready to go.

fluttergithubbot · 2019-12-04T23:13:01Z

This pull request is not suitable for automatic merging in its current state.

This pull request has changes requested. Please resolve those before re-applying the label.

Piinks · 2019-12-04T23:14:12Z

This pull request is not suitable for automatic merging in its current state.

This pull request has changes requested. Please resolve those before re-applying the label.

@dnfield I haven't seen this before. What does it mean?

dnfield · 2019-12-04T23:24:35Z

It's a bug :( It'ssupposed to be checking for reviews and refuse to merge if there are open change requests. This PR doesn't have any. Unfortunately the bot won't merge it currently.

Piinks · 2019-12-04T23:32:21Z

It's a bug :( It'ssupposed to be checking for reviews and refuse to merge if there are open change requests. This PR doesn't have any. Unfortunately the bot won't merge it currently.

No worries! I was curious. :)

skia-gold · 2019-12-18T14:39:11Z

test message from gold

Piinks · 2019-12-18T17:26:40Z

test message from gold

I read you gold leader. ⭐️ 😛

Validating Cirrus info

fe1cad6

Piinks added a: tests "flutter test", flutter_test, or one of our tests c: contributor-productivity Team-specific productivity, code health, technical debt. framework flutter/packages/flutter repository. See also f: labels. work in progress; do not review labels Nov 8, 2019

googlebot added the cla: yes label Nov 8, 2019

Piinks changed the title ~~Pre-Submit Tryjobs for Flutter Gold~~ [WIP] Pre-Submit Tryjobs for Flutter Gold Nov 8, 2019

Piinks added 9 commits November 15, 2019 12:01

Merge branch 'master' of github.com:flutter/flutter into goldTryjobs

8692295

++

b456402

++

37a93fa

++

384c42b

++

fd89364

++

5c8ad4e

++

dcd2a49

Adding tryjob flow to presubmit comparator

cf2a0dd

Commenting out cirrus testing

8dd9b4c

Piinks commented Nov 19, 2019

View reviewed changes

packages/flutter_goldens_client/lib/skia_client.dart Outdated Show resolved Hide resolved

packages/flutter_goldens_client/lib/skia_client.dart Outdated Show resolved Hide resolved

packages/flutter_goldens/lib/flutter_goldens.dart Show resolved Hide resolved

Analyzer therapy, doc change to trigger framework tests.

c842c61

Piinks added a: fidelity Matching the OEM platforms better a: images Loading, displaying, rendering images a: quality A truly polished experience labels Nov 19, 2019

Piinks added 2 commits November 19, 2019 10:39

Analyzer therapy

25bd67e

++

7a16e81

Piinks commented Nov 19, 2019

View reviewed changes

++

7d8952a

kjlubick requested changes Nov 22, 2019

View reviewed changes

packages/flutter_goldens_client/lib/skia_client.dart Outdated Show resolved Hide resolved

Piinks changed the title ~~Pre-Submit Tryjobs for Flutter Gold~~ [WIP] Pre-Submit Tryjobs for Flutter Gold Nov 22, 2019

Piinks added the work in progress; do not review label Nov 22, 2019

Piinks added 2 commits November 22, 2019 09:44

Merge branch 'master' of github.com:flutter/flutter into goldTryjobs

5f6e29f

Changing to patchset_id

ef219ee

Piinks mentioned this pull request Nov 26, 2019

Implement screenshot test for flutter web. #45530

Merged

Merge branch 'master' of github.com:flutter/flutter into goldTryjobs

2413496

Piinks changed the title ~~[WIP] Pre-Submit Tryjobs for Flutter Gold~~ Pre-Submit Tryjobs for Flutter Gold Nov 26, 2019

Piinks removed the work in progress; do not review label Nov 26, 2019

Kick bots

01c7667

Piinks mentioned this pull request Nov 27, 2019

Flutter Gold is not receiving image uploads #45699

Closed

Merge branch 'master' into goldTryjobs

7d837da

kjlubick reviewed Dec 3, 2019

View reviewed changes

packages/flutter_goldens/lib/flutter_goldens.dart Outdated Show resolved Hide resolved

Updating docs :)

73ac621

dnfield reviewed Dec 4, 2019

View reviewed changes

dnfield approved these changes Dec 4, 2019

View reviewed changes

Piinks added the waiting for tree to go green label Dec 4, 2019

fluttergithubbot removed the waiting for tree to go green label Dec 4, 2019

Piinks merged commit 9011cec into flutter:master Dec 4, 2019

dnfield mentioned this pull request Dec 5, 2019

[cocoon] check-waiting-pull-requests sometimes thinks there are changes requested when there are not #46131

Closed

Piinks mentioned this pull request Jan 13, 2020

Golden file changes originating from flutter/engine require a manual roll #48744

Closed

github-actions bot locked as resolved and limited conversation to collaborators Aug 2, 2021

Piinks deleted the goldTryjobs branch March 14, 2025 17:13

		final File keys = workDirectory.childFile('keys.json');
		final File failures = workDirectory.childFile('failures.json');

Pre-Submit Tryjobs for Flutter Gold #44474

Pre-Submit Tryjobs for Flutter Gold #44474

Uh oh!

Conversation

Piinks commented Nov 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

Piinks commented Nov 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Piinks commented Nov 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Piinks commented Nov 20, 2019

Uh oh!

kjlubick commented Nov 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kjlubick commented Nov 21, 2019

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

Piinks commented Dec 4, 2019

Uh oh!

fluttergithubbot commented Dec 4, 2019

Uh oh!

Piinks commented Dec 4, 2019

Uh oh!

dnfield commented Dec 4, 2019

Uh oh!

Piinks commented Dec 4, 2019

Uh oh!

skia-gold commented Dec 18, 2019

Uh oh!

Piinks commented Dec 18, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Piinks commented Nov 8, 2019 •

edited

Loading

Piinks commented Nov 11, 2019 •

edited

Loading

kjlubick commented Nov 21, 2019 •

edited

Loading