Skip to content

Add a flutter app that can resize itself as integration test app. #112297

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Sep 26, 2022
Merged

Add a flutter app that can resize itself as integration test app. #112297

merged 7 commits into from
Sep 26, 2022

Conversation

@a-wallen
Copy link
Contributor

@a-wallen a-wallen commented Sep 23, 2022 •
edited
Loading

Screen.Recording.2022-09-23.at.9.30.14.AM.mov

Resizing a macos window after runApp was called twice caused the engine to hang. In order to test the fix, four approaches were possible.

  1. Write a fixture test in the engine.

not possible as the test would require the flutter framework and there cannot be a circular dependency

  1. Write a native unit XCTest

I couldn't get the FlutterViewController to display the FlutterDartProject that would reproduce this bug

  1. Write a native XCUITest

The UI test required an Apple Script to resize the window with sudo permissions... not a favorable approach

  1. Write a flutter integration test

Approach 4 seems reasonable as it was the easiest to implement.

This commit contains the application and the integration tests that make sure the app works. I plan to follow up on this PR with another that implements the test that checks whether an app hangs after resizing.

partial fix to #92673

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], including [Features we expect every widget to implement].
  • I signed the [CLA].
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is [test-exempt].
  • All existing and new tests are passing.

@flutter-dashboard flutter-dashboard bot added the c: contributor-productivity Team-specific productivity, code health, technical debt. label Sep 23, 2022
@a-wallen a-wallen force-pushed the resize_integration_test branch from 4cedb17 to 7def58e Compare September 23, 2022 19:14
@a-wallen a-wallen changed the title Add flutter app that can resize itself as integration test app. Add a flutter app that can resize itself as integration test app. Sep 23, 2022
@a-wallen a-wallen force-pushed the resize_integration_test branch 2 times, most recently from 6ee591f to 4950b8d Compare September 23, 2022 19:36
a-wallen
a-wallen commented Sep 23, 2022
View reviewed changes
TESTOWNERS Outdated
Copy link
Contributor Author

@a-wallen a-wallen Sep 23, 2022

Choose a reason for hiding this comment

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

yak-shave: sort TESTOWNERS lines alphabetically

@a-wallen a-wallen force-pushed the resize_integration_test branch from 4950b8d to dc4849f Compare September 23, 2022 19:41
@a-wallen a-wallen marked this pull request as ready for review September 23, 2022 20:17
@a-wallen a-wallen requested a review from keyonghan as a code owner September 23, 2022 20:17
jmagman
jmagman reviewed Sep 23, 2022
View reviewed changes
Copy link
Member

@jmagman jmagman left a comment

Choose a reason for hiding this comment

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

Can you use an existing integration test for window resize instead of making a new one?

yaakovschectman
yaakovschectman approved these changes Sep 23, 2022
View reviewed changes
Copy link
Contributor

@yaakovschectman yaakovschectman left a comment

Choose a reason for hiding this comment

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

lgtm save other comments

@a-wallen
Copy link
Contributor Author

a-wallen commented Sep 23, 2022

@jmagman I can.

channels is likely the best candidate, but the app tests the method channel itself, not platform functionality.
platform_view is also close, but it's an example and it would be weird if we had a resize implementation just floating around for macos.

I think the reason I did it this way is to separate concerns of the integration tests. Regardless, if you want, I can add the test to either of those.

@jmagman
Copy link
Member

jmagman commented Sep 23, 2022

@jmagman I can.

channels is likely the best candidate, but the app tests the method channel itself, not platform functionality. platform_view is also close, but it's an example and it would be weird if we had a resize implementation just floating around for macos.

I think the reason I did it this way is to separate concerns of the integration tests.

That doesn't scale well, we don't need a new integration test project for every test. How about ui?

@a-wallen
Copy link
Contributor Author

a-wallen commented Sep 23, 2022

@jmagman is this what you mean 613d794 ?

a-wallen
a-wallen commented Sep 23, 2022
View reviewed changes
dev/integration_tests/ui/integration_test/resize_integration_test.dart Outdated
Copy link
Contributor Author

@a-wallen a-wallen Sep 23, 2022

Choose a reason for hiding this comment

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

^

@jmagman
Copy link
Member

jmagman commented Sep 23, 2022

@jmagman is this what you mean 613d794 ?

Exactly, resize a window for an existing integration test instead of making a new one. Windows and Linux should likely have similar tests.

@jmagman
Copy link
Member

jmagman commented Sep 23, 2022

Windows and Linux should likely have similar tests.

(not that you should implement them, just a comment)

@a-wallen a-wallen force-pushed the resize_integration_test branch from 613d794 to 901f093 Compare September 24, 2022 18:48
@a-wallen
Copy link
Contributor Author

a-wallen commented Sep 24, 2022

@jmagman thanks for the feedback. This PR is much cleaner now thanks to you 👍 .

@a-wallen a-wallen force-pushed the resize_integration_test branch from 901f093 to 109442f Compare September 24, 2022 20:11
@a-wallen a-wallen force-pushed the resize_integration_test branch from 109442f to ce1433d Compare September 24, 2022 20:14
@a-wallen
Copy link
Contributor Author

a-wallen commented Sep 24, 2022

IMG_2423

jmagman
jmagman approved these changes Sep 26, 2022
View reviewed changes
Copy link
Member

@jmagman jmagman left a comment

Choose a reason for hiding this comment

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

LGTM

@a-wallen a-wallen added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 26, 2022
@auto-submit auto-submit bot merged commit b22388e into flutter:master Sep 26, 2022
@a-wallen a-wallen deleted the resize_integration_test branch September 26, 2022 17:46
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Sep 27, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Sep 27, 2022
@engine-flutter-autoroll
b22388e Add a flutter app that can resize itself as integration test …
99970e3 
…app. (flutter/flutter#112297)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Sep 27, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 27, 2022
@engine-flutter-autoroll
b22388e Add a flutter app that can resize itself as integration test …
f1ebaf3 
…app. (flutter/flutter#112297)
@jmagman jmagman mentioned this pull request Jan 20, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jmagman jmagman jmagman approved these changes

@keyonghan keyonghan Awaiting requested review from keyonghan

@cbracken cbracken Awaiting requested review from cbracken

@loic-sharma loic-sharma Awaiting requested review from loic-sharma

+1 more reviewer

@yaakovschectman yaakovschectman yaakovschectman approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

autosubmit Merge PR when tree becomes green via auto submit App c: contributor-productivity Team-specific productivity, code health, technical debt.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@a-wallen @jmagman @yaakovschectman