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

[iOS ] Fix errors in unittest and scenario tests running against iOS 17 simulators (details in the description) #45391

Merged
auto-submit merged 1 commit into from
Sep 5, 2023
Merged

[iOS ] Fix errors in unittest and scenario tests running against iOS 17 simulators (details in the description) #45391

auto-submit merged 1 commit into from
Sep 5, 2023

Conversation

@cyanglaz
Copy link
Contributor

@cyanglaz cyanglaz commented Sep 1, 2023 •
edited
Loading

  1. New Mac version seems to have updated the locale data, to match the mac change, we can dynamically read the data when testing instead of hard coding.

  2. "we are sending a UIImage type as parameter to XCTAttachment attachmentWithScreenshot:, which takes a XCUIScreenshot as parameter, this is not supposed to pass in old iOS versions." In this PR we updated the parameter to use the XCUIScreenshot type.

Fixes flutter/flutter#133783

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 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.

@cyanglaz cyanglaz marked this pull request as ready for review September 1, 2023 19:01
@cyanglaz cyanglaz requested a review from vashworth September 1, 2023 19:01
vashworth
vashworth reviewed Sep 1, 2023
View reviewed changes
testing/scenario_app/ios/Scenarios/ScenariosUITests/LocalizationInitializationTest.m Outdated
NSString* expectedIdenntifier = [NSString stringWithFormat:@"[%@]", localeDart];
XCUIElement* textInputSemanticsObject =
[self.application.textFields matchingIdentifier:@"[en]"].element;
[self.application.textFields matchingIdentifier:expectedIdenntifier].element;
Copy link
Contributor

@vashworth vashworth Sep 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

What is the value for the expectedIdenntifier? Curious how to differs. Also, is that difference going to be a problem in other part of our code other than just this test?

Copy link
Contributor Author

@cyanglaz cyanglaz Sep 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It becomes en_US instead of en. Based on the old comment, I thought it should have always been en_US, I'm not sure why it passed before.

Copy link
Contributor Author

@cyanglaz cyanglaz Sep 2, 2023

Choose a reason for hiding this comment

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

When I ran with older XCode it shows en_US for me, I think the value change might be due to MacOS update rather than simulator version.

@cyanglaz cyanglaz mentioned this pull request Sep 2, 2023
Closed
@vashworth
Copy link
Contributor

vashworth commented Sep 5, 2023 •
edited
Loading

I ran these changes against Xcode 15 on a CI bot, and it fixes the -[LocalizationInitializationTest testNoLocalePrepend] test. It also gives a different error for the GoldenTestManager tests.

GoldenTestManager.m:88: error: -[NonFullScreenFlutterViewPlatformViewUITests testPlatformView] : failed - This test will fail - no golden named golden_non_full_screen_flutter_view_platform_view_iPhone SE (3rd generation)_17.0_simulator found. Follow the steps in the README to add a new golden.

https://chromium-swarm.appspot.com/task?id=647ed4e582e85a10

@cyanglaz Is this the error you were expecting from your comment flutter/flutter#133783 (comment)?

vashworth
vashworth approved these changes Sep 5, 2023
View reviewed changes
Copy link
Contributor

@vashworth vashworth 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 rename the PR title since it involves two changes now?

Other than that, LGTM

@cyanglaz
Copy link
Contributor Author

cyanglaz commented Sep 5, 2023

Yes, updating the simulator requires a change of the golden name.

@cyanglaz cyanglaz changed the title [iOS ] Dynamically read the default locale data in scenario test LocalizationInitializationTest [iOS ] Fix errors in unittest and scenario tests running against iOS 17 simulators (details in the description) Sep 5, 2023
@cyanglaz cyanglaz added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 5, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented Sep 5, 2023

auto label is removed for flutter/engine/45391, Failed to merge flutter/engine/45391 with FormatException: Unexpected end of input (at character 1)

^
.

@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Sep 5, 2023
fix
f96175a 
format

format

fromat

typo

fix screenshot tests crash
@cyanglaz cyanglaz force-pushed the scenario_test_ios17 branch from 6589aad to f96175a Compare September 5, 2023 22:11
@cyanglaz cyanglaz added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 5, 2023
@auto-submit auto-submit bot merged commit 5903490 into flutter:main Sep 5, 2023
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Sep 5, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 5, 2023
@engine-flutter-autoroll
590349006 [iOS ] Fix errors in unittest and scenario tests running ag…
e05a316 
…ainst iOS 17 simulators (details in the description) (flutter/engine#45391)
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Sep 6, 2023
@engine-flutter-autoroll
Roll Flutter Engine from 5b2cc9d9b8fe to 590349006d23 (2 revisions) (#…
3871139 
…134081)

flutter/engine@5b2cc9d...5903490

2023-09-05 [email protected] [iOS ] Fix errors in unittest and scenario tests running against iOS 17 simulators (details in the description) (flutter/engine#45391)
2023-09-05 [email protected] Roll Skia from 2b9fc6a2c250 to 1019c10a2d38 (2 revisions) (flutter/engine#45466)

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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
hellohuanlin
hellohuanlin reviewed Sep 6, 2023
View reviewed changes
testing/scenario_app/ios/Scenarios/ScenariosUITests/GoldenTestManager.m
XCUIScreenshot* screenshot = [[XCUIScreen mainScreen] screenshot];
if (!_goldenImage.image) {
XCTAttachment* attachment = [XCTAttachment attachmentWithScreenshot:screenshot.image];
XCTAttachment* attachment = [XCTAttachment attachmentWithScreenshot:screenshot];
Copy link
Contributor

@hellohuanlin hellohuanlin Sep 6, 2023

Choose a reason for hiding this comment

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

fascinating - how did it pass before!

@cyanglaz cyanglaz deleted the scenario_test_ios17 branch September 6, 2023 20:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@hellohuanlin hellohuanlin hellohuanlin left review comments

@vashworth vashworth vashworth 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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Multiple ScenariosUITests in engine fail with Xcode 15 in CI

3 participants

@cyanglaz @vashworth @hellohuanlin