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

[share] Fix bug on iPad where origin is null and enable XCUITests #3188

Merged
merged 31 commits into from
Oct 27, 2020

Conversation

cyanglaz
Copy link
Contributor

@cyanglaz cyanglaz commented Oct 22, 2020

Creates a XCUITests target in the share plugin.

Also fixes flutter/flutter#66485

@google-cla google-cla bot added the cla: yes label Oct 22, 2020
@cyanglaz cyanglaz changed the title Enable xcuitests Enable xcuitests with share plugin Oct 23, 2020
@cyanglaz cyanglaz changed the title Enable xcuitests with share plugin [share] Fix a small bug and enable XCUITests Oct 26, 2020
@cyanglaz cyanglaz changed the title [share] Fix a small bug and enable XCUITests [share] Fix bug on iPad where origin is null and enable XCUITests Oct 26, 2020
@@ -1,87 +0,0 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Member

Choose a reason for hiding this comment

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

Did you mean to remove this?

elementMatchingPredicate:[NSPredicate
predicateWithFormat:@"label == %@", @"Share With Empty Origin"]];
if (![shareWithEmptyOriginButton waitForExistenceWithTimeout:30]) {
NSLog(@"%@", app.debugDescription);
Copy link
Member

Choose a reason for hiding this comment

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

Remove the NSLog

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is beneficial so that when a failure happened and we can see what's in the a11y tree and why the particular item was not found in the a11y tree.

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 print it when it fails? Your test results will get clogged pretty fast with a bunch of debugging lines.

Copy link
Member

Choose a reason for hiding this comment

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

Also NSLog is soft deprecated, should be using os_log.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The log only happens if ![shareWithEmptyOriginButton waitForExistenceWithTimeout:kSecondsToWaitWhenFindingElements] and we send a XCTFail signal afterwards. So it doesn't print unless the test fails.

Thanks for the info of os_log, updated!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or did you mean to combine the "app.debugDescription" log within XCTFail?

XCUIElement* shareWithEmptyOriginButton = [app.buttons
elementMatchingPredicate:[NSPredicate
predicateWithFormat:@"label == %@", @"Share With Empty Origin"]];
if (![shareWithEmptyOriginButton waitForExistenceWithTimeout:30]) {
Copy link
Member

Choose a reason for hiding this comment

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

30 seconds seems really long.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We used the same waiting length on scenario app tests https://github.com/flutter/engine/blob/master/testing/scenario_app/ios/Scenarios/ScenariosUITests/GoldenPlatformViewTests.m#L11
I think the rational is that on ci, the animation might take some time to run and might cause false negative to happen.
This returns immediately after finding the item, so I think setting time-out to be longer should be fine?

predicateWithFormat:@"identifier == %@", @"ActivityListView"]];
if (![activityListView waitForExistenceWithTimeout:30]) {
NSLog(@"%@", app.debugDescription);
XCTFail(@"Failed due to not able to find activityListView with %@ seconds", @(30));
Copy link
Member

Choose a reason for hiding this comment

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

literals as @30

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Moved all the "30" out to a static

elementMatchingPredicate:[NSPredicate
predicateWithFormat:@"identifier == %@", @"ActivityListView"]];
if (![activityListView waitForExistenceWithTimeout:30]) {
NSLog(@"%@", app.debugDescription);
Copy link
Member

Choose a reason for hiding this comment

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

Remove

XCUIElement* activityListView = [app.otherElements
elementMatchingPredicate:[NSPredicate
predicateWithFormat:@"identifier == %@", @"ActivityListView"]];
if (![activityListView waitForExistenceWithTimeout:30]) {
Copy link
Member

Choose a reason for hiding this comment

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

Ditto, 30 seconds seems too long.

@@ -0,0 +1,22 @@
<?xml version="1.0" encoding="UTF-8"?>
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 unused?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the good catch! Removed.

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!

@cyanglaz cyanglaz merged commit dedeae0 into flutter:xctest Oct 27, 2020
cyanglaz pushed a commit that referenced this pull request Oct 27, 2020
…n the repo (#3210)

* enable xctest in .cirrus.yml (#3189)

* [share] Fix bug on iPad where `origin` is null and enable XCUITests (#3188)
FlutterSu pushed a commit to FlutterSu/flutter-plugins that referenced this pull request Nov 20, 2020
…n the repo (flutter#3210)

* enable xctest in .cirrus.yml (flutter#3189)

* [share] Fix bug on iPad where `origin` is null and enable XCUITests (flutter#3188)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants