-
Notifications
You must be signed in to change notification settings - Fork 6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[macOS] Delete FlutterCompositor tests #47527
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cbracken
requested review from
loic-sharma,
goderbauer,
dkwingsmt,
gspencergoog and
yaakovschectman
October 31, 2023 22:18
matanlurey
approved these changes
Oct 31, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM I like this change
cbracken
force-pushed
the
remove-compositor-tests
branch
from
October 31, 2023 22:40
224a97c
to
789da14
Compare
goderbauer
approved these changes
Oct 31, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
The tests for FlutterCompositor are not useful. The current tests test two things: 1. That the mocks we set up behave the way we set them up to behave. 2. That the implementation of FlutterCompositor is the current implementation of FlutterCompositor. As an example, consider FlutterCompositorTest.TestPresent: https://github.com/flutter/engine/blob/89e8de970cb99aa82e067bbdb4a8e927e53f0b28/shell/platform/darwin/macos/framework/Source/FlutterCompositorTest.mm#L107 Ostensibly, this test verifies that the onPresent callback configured in our fake FlutterViewProvider implementation is called when FlutterCompositor::Present() is called. However, taking a look at the mocking setup: https://github.com/flutter/engine/blob/89e8de970cb99aa82e067bbdb4a8e927e53f0b28/shell/platform/darwin/macos/framework/Source/FlutterCompositorTest.mm#L47-L85 We do the following: 1. Mock out FlutterSurfaceManager such that when a surface is requested, we hand back a mock surface. A little gross since we're relying on some knowledge of implementation details of the compositor, but let's take this as reasonable for now. 2. We mock out `FlutterSurface asFlutterMetalTexture` to return a mock texture. Again, we're getting a bit deep into implementation details that the test shouldn't know about, but let's assume this gets us somewhere. 3. We mock out `FlutterSurfaceManager present:notify:` to actually call the `onPresent` callback if it's passed in. In effect, we're testing that: 1. We configured our mock for `FlutterSurfaceManager present:notify:` to call onPresent. 2. That `FlutterCompositor::Present` actually calls `FlutterSurfaceManager present:notify:` despite that being a simple implementation detail of that call. This removes these tests. I have filed the following issue to track refactoring this class for testability and adding tests: flutter/flutter#137648 Encountered these tests as part of deflaking and cleaning up memory allocations throughout the macOS desktop tests. Issue: flutter/flutter#104789 Issue: flutter/flutter#127441 Issue: flutter/flutter#124840
cbracken
force-pushed
the
remove-compositor-tests
branch
from
October 31, 2023 23:12
789da14
to
a6b29cc
Compare
cbracken
added
the
autosubmit
Merge PR when tree becomes green via auto submit App
label
Oct 31, 2023
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Nov 1, 2023
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Nov 1, 2023
auto-submit bot
pushed a commit
to flutter/flutter
that referenced
this pull request
Nov 1, 2023
…137656) flutter/engine@db06c2e...a0ac6b4 2023-11-01 [email protected] [Impeller] Include cstdint everywhere that uint32_t is used. (flutter/engine#47533) 2023-11-01 [email protected] [Impeller] Fix nullopt access and simplify coverage computation in GetSubpassCoverage. (flutter/engine#47347) 2023-11-01 [email protected] [Impeller] OpenGLES: Ensure frag/vert textures are bound with unique texture units. (flutter/engine#47218) 2023-11-01 [email protected] Roll Fuchsia Linux SDK from LCfhx_lTRJI51G0zc... to _TyF0etsONe5aqCbM... (flutter/engine#47532) 2023-11-01 [email protected] [Impeller] stencil buffer record/replay instead of MSAA storage. (flutter/engine#47397) 2023-11-01 [email protected] [macOS] Delete FlutterCompositor tests (flutter/engine#47527) 2023-10-31 [email protected] [Impeller] Place Rect statics under the Rect template. (flutter/engine#47529) 2023-10-31 [email protected] Roll Skia from aaa225e0cc6d to 34ef20100acc (1 revision) (flutter/engine#47530) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from LCfhx_lTRJI5 to _TyF0etsONe5 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 [email protected],[email protected] 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The tests for FlutterCompositor are not useful. The current tests test two things:
As an example, consider FlutterCompositorTest.TestPresent:
engine/shell/platform/darwin/macos/framework/Source/FlutterCompositorTest.mm
Lines 107 to 137 in 89e8de9
Ostensibly, this test verifies that the onPresent callback configured in our fake FlutterViewProvider implementation is called when FlutterCompositor::Present() is called.
However, taking a look at the mocking setup:
engine/shell/platform/darwin/macos/framework/Source/FlutterCompositorTest.mm
Lines 47 to 85 in 89e8de9
We do the following:
FlutterSurface asFlutterMetalTexture
to return a mock texture. Again, we're getting a bit deep into implementation details that the test shouldn't know about, but let's assume this gets us somewhere.FlutterSurfaceManager present:notify:
to actually call theonPresent
callback if it's passed in.In effect, we're testing that:
FlutterSurfaceManager present:notify:
to call onPresent.FlutterCompositor::Present
actually callsFlutterSurfaceManager present:notify:
despite that being a simple implementation detail of that call.This removes these tests. I have filed the following issue to track refactoring this class for testability and adding tests: flutter/flutter#137648
Encountered these tests as part of deflaking and cleaning up memory allocations throughout the macOS desktop tests.
Issue: flutter/flutter#137648
Issue: flutter/flutter#104789
Issue: flutter/flutter#127441
Issue: flutter/flutter#124840
Pre-launch Checklist
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.