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

[video_player] Fix XCUITest based on the new tooltip accessibility label #5426

Merged
merged 5 commits into from
Apr 28, 2022
Merged

[video_player] Fix XCUITest based on the new tooltip accessibility label #5426

merged 5 commits into from
Apr 28, 2022

Conversation

cyanglaz
Copy link
Contributor

@cyanglaz cyanglaz commented Apr 27, 2022 •
edited
Loading

The accessibility label for tool tip seems change in one of the recent flutter updates(flutter/flutter#87684), which results failure in this particular XCUITest. This is blocking the flutter to plugins roll.

Fixes: flutter/flutter#102698

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 relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/plugins repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
  • I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
  • 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.

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

@cyanglaz cyanglaz requested a review from hellohuanlin as a code owner April 27, 2022 22:25
@cyanglaz
Copy link
Contributor Author

cyanglaz commented Apr 27, 2022 •
edited
Loading

Interesting, the test is still failing on master. It passed on my local.
We might have to do something for the test to pass on stable too.

@jmagman
Copy link
Member

jmagman commented Apr 27, 2022

@chunhtai this failure was caused by flutter/flutter#87684, is the swapping of label text like this expected?

@cyanglaz looks like this needs to be included in a manual framework->plugin roll so the tests pass, on top of a roll like #5425

@cyanglaz
Copy link
Contributor Author

cyanglaz commented Apr 27, 2022 •
edited
Loading

Just to add to @jmagman's comment, the "Playback speed" is the tooltip name, and "1.0x" is the tooltip current value.
The accessibility value used to be {tooltipname}\n{tooltipvalue}
After flutter/flutter#87684, the accessibility value becomes {tooltipvalue}\n{tooptipname}

@chunhtai
Copy link

yes, previously the tooltips are appended to the label before multiple semantics node merges, but now tooltips are appended after multiple semantics node merges.

It looks like the change surface an accessibility issue, too. tooltip should be announced after the label not before.

stuartmorgan-g
stuartmorgan-g reviewed Apr 27, 2022
View reviewed changes
packages/video_player/video_player_avfoundation/example/ios/RunnerUITests/VideoPlayerUITests.m Outdated
@@ -30,15 +30,15 @@ - (void)testPlayVideo {
XCTAssertTrue([playButton waitForExistenceWithTimeout:30.0]);
[playButton tap];

XCUIElement *playbackSpeed1x = app.staticTexts[@"Playback speed\n1.0x"];
XCUIElement *playbackSpeed1x = app.staticTexts[@"1.0x\nPlayback speed"];
Copy link
Contributor

Choose a reason for hiding this comment

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

You'll need to have both the old and new version, and wait for either one to show up, because the tests run on both stable and master.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, will do that

Copy link
Member

@jmagman jmagman Apr 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Alternatively you could just look for 1.0x which is the spirit of this check.
( I don't think app.staticTexts[@"1.0x"] will work since the string needs to exactly match, but you could check if any contains that).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I missed this comment earlier. I like this better. Updated the PR with only looking for 1.0x and 5.0x

@cyanglaz cyanglaz mentioned this pull request Apr 28, 2022
Open
fix test
72d4b90 
update changelog

add debug info

revert

also check for old accessiblity value
@cyanglaz cyanglaz force-pushed the video_player_xcuitest branch from 87b00ba to 72d4b90 Compare April 28, 2022 16:55
jmagman
jmagman approved these changes Apr 28, 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.

Really small nit, you can also just land ASAP to unblock the tree

packages/video_player/video_player_avfoundation/example/ios/RunnerUITests/VideoPlayerUITests.m Outdated
XCUIElement *playbackSpeed5x = app.staticTexts[@"Playback speed\n5.0x"];
XCTAssertTrue([playbackSpeed5x waitForExistenceWithTimeout:30.0]);
NSPredicate *find5xButton = [NSPredicate predicateWithFormat:@"label CONTAINS '5.0x'"];
XCUIElement *playbackSpeed5x = [app.staticTexts containingPredicate:find5xButton].element;
Copy link
Member

Choose a reason for hiding this comment

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

You could use `[app.staticTexts elementMatchingPredicate:userAgentPredicate];

Suggested change
XCUIElement *playbackSpeed5x = [app.staticTexts containingPredicate:find5xButton].element;
XCUIElement *playbackSpeed5x = [app.staticTexts elementMatchingPredicate:find5xButton];

@cyanglaz cyanglaz added the waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. label Apr 28, 2022
@cyanglaz cyanglaz merged commit 4b687c9 into flutter:main Apr 28, 2022
@cyanglaz cyanglaz deleted the video_player_xcuitest branch April 28, 2022 21:16
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Apr 29, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 29, 2022
@engine-flutter-autoroll
4b687c9fa [video_player] Fix XCUITest based on the new tooltip access…
d0036ca 
…ibility label (flutter/plugins#5426)
CaseyHillers pushed a commit to flutter/flutter that referenced this pull request Apr 29, 2022
@engine-flutter-autoroll
Roll Plugins from ac3167f83402 to 160c714e75f3 (9 revisions) (#102824)
ad1d300 
* c09ef582e [flutter_plugin_tools] Remove UWP (flutter/plugins#5432)

* 3876f54bf [ci.yaml] Remove explicit caches (flutter/plugins#5434)

* 0605d876d [webview_flutter_android] Updates `pigeon` version to support null safety (flutter/plugins#5395)

* 4b687c9fa [video_player] Fix XCUITest based on the new tooltip accessibility label (flutter/plugins#5426)

* e777e515b Roll Flutter from 1b58a59 to 4cea9af (147 revisions) (flutter/plugins#5437)

* 656e8c443 Roll Flutter from 4cea9af to 5b71314 (4 revisions) (flutter/plugins#5441)

* f689280bf [flutter_plugin_tools] Validate code blocks in readme-check (flutter/plugins#5436)

* 9c41c6895 Roll Flutter from 5b71314 to 7a74222 (1 revision) (flutter/plugins#5442)

* 160c714e7 Roll Flutter from 7a74222 to 2eed8cb (1 revision) (flutter/plugins#5443)
egramond pushed a commit to egramond/flutter that referenced this pull request May 5, 2022
@engine-flutter-autoroll @egramond
Roll Plugins from ac3167f83402 to 160c714e75f3 (9 revisions) (flutter…
f9739a2 
…#102824)

* c09ef582e [flutter_plugin_tools] Remove UWP (flutter/plugins#5432)

* 3876f54bf [ci.yaml] Remove explicit caches (flutter/plugins#5434)

* 0605d876d [webview_flutter_android] Updates `pigeon` version to support null safety (flutter/plugins#5395)

* 4b687c9fa [video_player] Fix XCUITest based on the new tooltip accessibility label (flutter/plugins#5426)

* e777e515b Roll Flutter from 1b58a59 to 4cea9af (147 revisions) (flutter/plugins#5437)

* 656e8c443 Roll Flutter from 4cea9af to 5b71314 (4 revisions) (flutter/plugins#5441)

* f689280bf [flutter_plugin_tools] Validate code blocks in readme-check (flutter/plugins#5436)

* 9c41c6895 Roll Flutter from 5b71314 to 7a74222 (1 revision) (flutter/plugins#5442)

* 160c714e7 Roll Flutter from 7a74222 to 2eed8cb (1 revision) (flutter/plugins#5443)
mauricioluz pushed a commit to mauricioluz/plugins that referenced this pull request Jan 26, 2023
@mauricioluz
[video_player] Fix XCUITest based on the new tooltip accessibility la…
29554ad 
…bel (flutter#5426)

The accessibility label for tool tip seems change in one of the recent flutter updates(flutter/flutter#87684), which results failure in this particular XCUITest. This is blocking the flutter to plugins roll.

Fixes: flutter/flutter#102698
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jmagman jmagman jmagman approved these changes

@hellohuanlin hellohuanlin hellohuanlin approved these changes

@stuartmorgan-g stuartmorgan-g Awaiting requested review from stuartmorgan-g

Assignees
No one assigned
Labels
p: video_player platform-ios waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[video_player] UITests failure when rolling flutter into plugin
5 participants
@cyanglaz @jmagman @chunhtai @stuartmorgan-g @hellohuanlin