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

[Impeller] backfilled golden tests from playground tests in aiks_unittests. #40770

Merged
merged 17 commits into from
Mar 30, 2023
Merged

[Impeller] backfilled golden tests from playground tests in aiks_unittests. #40770

merged 17 commits into from
Mar 30, 2023

Conversation

gaaclarke
Copy link
Member

@gaaclarke gaaclarke commented Mar 29, 2023 •
edited
Loading

issue: flutter/flutter#123790

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@gaaclarke gaaclarke force-pushed the aiks_golden branch 2 times, most recently from 780bead to 0162c6b Compare March 30, 2023 00:19
@chinmaygarde chinmaygarde changed the title [impeller] backfilled golden tests from playground tests in aiks_unittests [Impeller] backfilled golden tests from playground tests in aiks_unittests. Mar 30, 2023
@gaaclarke gaaclarke marked this pull request as ready for review March 30, 2023 16:38
@flutter-dashboard

This comment was marked as outdated.

Sign in to view
@flutter-dashboard
Copy link

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

Changes reported for pull request #40770 at sha 4375669

@gaaclarke gaaclarke requested a review from dnfield March 30, 2023 19:10
@gaaclarke
Copy link
Member Author

I've approved all changes since they look roughly right (modulo flutter/flutter#123789). It's worth just getting these landed and iterating on them as we want.

dnfield
dnfield reviewed Mar 30, 2023
View reviewed changes
impeller/aiks/BUILD.gn
impeller_component("aiks_unittests_golden") {
testonly = true

defines = [ "IMPELLER_GOLDEN_TESTS" ]
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: consider making a GN template that just defines this rather than defining it in each invocation of impeller_component that wants it.

dnfield
dnfield reviewed Mar 30, 2023
View reviewed changes
impeller/golden_tests/golden_playground_test_mac.cc
GTEST_SKIP_(
"GoldenPlaygroundTest doesn't support interactive playground tests "
"yet.");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be nice if we could somehow check if the test actually used ImGui or not. This will be brittle, but it's probably just temporary right?

Copy link
Member Author

@gaaclarke gaaclarke Mar 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I looked into it, the problem is that we have to call GTEST_SKIP inside of SetUp so there's no way (at runtime) to know if a test is going to use ImGui in SetUp =T

I think we should probably abolish any ImGui tests since they can't be automated. (edit: easily, automated easily)

Copy link
Contributor

Choose a reason for hiding this comment

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

Alternatively we move them into some other target that makes it clear they'r enot automated tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea I was thinking about something similar, maybe making the target something like impeller_playground. I'm not sure if we want to just move everything over to goldens though. In which case there would be no need for that. I'm still trying to feel out the problem.

@gaaclarke
Copy link
Member Author

I'm going to rebase this to capture brandon's latest fix which should change a golden.

@flutter-dashboard
Copy link

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

Changes reported for pull request #40770 at sha 11bf1ea

@gaaclarke gaaclarke added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 30, 2023
@auto-submit auto-submit bot merged commit d66f46c into flutter:main Mar 30, 2023
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Mar 30, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 30, 2023
@engine-flutter-autoroll
d66f46cdd [Impeller] backfilled golden tests from playground tests in…
c6dc648 
… aiks_unittests. (flutter/engine#40770)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
1 more reviewer

@dnfield dnfield dnfield approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@gaaclarke gaaclarke

Labels
autosubmit Merge PR when tree becomes green via auto submit App e: impeller will affect goldens
Projects
No open projects
Impeller
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gaaclarke @dnfield @chinmaygarde