Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@dnfield
Copy link
Contributor

@dnfield dnfield commented Oct 18, 2023

This removes skips for the golden tests in //testing/dart/canvas_test.dart and instead passes them up to Skia gold.

Adds a utility class for dealing with Skia gold from these tests, as well as the existing fuzzy identical image comparison for tests that just want to do in memory comparison of images generated from the same test.

Removes the old golden files that were in tree.

Part of flutter/flutter#53784

@dnfield
Copy link
Contributor Author

dnfield commented Oct 18, 2023

(rather than trying to use Platform.script, I'm adding a dart define for a skia gold working directory)

@chinmaygarde chinmaygarde changed the title [Impeller] Skia gold for flutter_tester dart tests [Impeller] Skia gold for flutter_tester dart tests. Oct 19, 2023
@dnfield
Copy link
Contributor Author

dnfield commented Oct 20, 2023

There's a few things that need to be fixed still:

  • Need to add a dimension for single and multi-threaded
  • Need to differentiate based on PID or something for when multiple tests are run concurrently
  • Need to actually get Gold to process these changes. Something still isn't entirely right there. There seems ot be a strange interaction between some of the async work that gets done for goldctl and litetest though.

Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

LGTM

@flutter-dashboard
Copy link

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

Changes reported for pull request #47066 at sha 48f28b5

@dnfield dnfield added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 31, 2023
#
# See: https://bugs.python.org/issue26903
max_processes = multiprocessing.cpu_count()
max_processes = 1 # multiprocessing.cpu_count()
Copy link
Member

Choose a reason for hiding this comment

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

Is this intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I'll revert this. This was to try to isolate an issue on ci.

return imageBytes;
}

String _getEngineCheckoutPath() {
Copy link
Member

Choose a reason for hiding this comment

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

Can you compute this using this.workDirectory and the Engine.findWithin constructor in https://github.com/flutter/engine/blob/main/tools/pkg/engine_repo_tools/lib/engine_repo_tools.dart#L87.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TIL, yes this should work nicely. Trying it.

@zanderso
Copy link
Member

Removing autosubmit for my questions.

@zanderso zanderso removed autosubmit Merge PR when tree becomes green via auto submit App Work in progress (WIP) Not ready (yet) for review! labels Oct 31, 2023
@github-actions github-actions bot added the platform-web Code specifically for the web engine label Nov 1, 2023
@flutter-dashboard
Copy link

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #47066 at sha 8db70f3

@dnfield dnfield added autosubmit Merge PR when tree becomes green via auto submit App and removed will affect goldens labels Nov 2, 2023
@auto-submit auto-submit bot merged commit 9d4c951 into flutter:main Nov 2, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 2, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 2, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 2, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Nov 2, 2023
…137789)

flutter/engine@b11e318...1e2eee0

2023-11-02 skia-flutter-autoroll@skia.org Roll Skia from 2b3472da9888 to 306543797674 (5 revisions) (flutter/engine#47610)
2023-11-02 15619084+vashworth@users.noreply.github.com Run tests on either macOS 12 or 13 (flutter/engine#47606)
2023-11-02 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from 32ukjtetQl4pbXfTw... to bhSlAQy4VM3Plrlh4... (flutter/engine#47611)
2023-11-02 dnfield@google.com [Impeller] Skia gold for flutter_tester dart tests. (flutter/engine#47066)

Also rolling transitive DEPS:
  fuchsia/sdk/core/mac-amd64 from 32ukjtetQl4p to bhSlAQy4VM3P

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC rmistry@google.com,zra@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
@dnfield dnfield deleted the gold branch November 3, 2023 15:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

autosubmit Merge PR when tree becomes green via auto submit App e: impeller platform-web Code specifically for the web engine

Projects

No open projects
Archived in project

Development

Successfully merging this pull request may close these issues.

5 participants