Skip to content
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

Run tests on either macOS 12 or macOS 13 #5089

Merged
merged 4 commits into from
Oct 10, 2023

Conversation

vashworth
Copy link
Contributor

In preparation for migrating our fleet to macOS 13, we're updating tests to run on either macOS 12 or macOS 13.

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/packages repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the package 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.

@@ -11,5 +11,19 @@ readonly DEVICE_NAME=Flutter-iPhone
readonly DEVICE=com.apple.CoreSimulator.SimDeviceType.iPhone-14
readonly OS=com.apple.CoreSimulator.SimRuntime.iOS-16-4

# Delete any existing devices named Flutter-iPhone. Having more than one may
# cause issues when builds target the device.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For reference, here's an example of this problem in action: https://ci.chromium.org/ui/p/flutter/builders/try/Mac_arm64%20custom_package_tests%20master/7040/overview

Unclear if it's a problem new to macOS 13

args: ["native-test", "--ios", "--ios-destination", "platform=iOS Simulator,name=iPhone 14,OS=latest"]
args: ["native-test", "--ios", "--ios-destination", "platform=iOS Simulator,name=Flutter-iPhone,OS=16.4"]
- name: boot simulator
# Ensure simulator is still booted
Copy link
Contributor Author

Choose a reason for hiding this comment

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

While I was testing, sometimes the simulator would shutdown after "native test" and before "drive examples"

See https://luci-milo.appspot.com/ui/p/flutter/builders/try/Mac_arm64%20ios_platform_tests_shard_4%20master/7162/overview for example (I added logs to drive example, where you'll see the Flutter-iPhone simulator is shutdown)

Copy link
Contributor

Choose a reason for hiding this comment

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

That's really weird. Is there any indication of why it's shutting down?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, the "native test" step shows the simulator is booted and works as expected. There's no logs that indicate it would be triggered to be shutdown. It also doesn't seem to happen every time.

@vashworth vashworth marked this pull request as ready for review October 9, 2023 15:12
@vashworth vashworth requested a review from tarrinneal as a code owner October 9, 2023 15:13
@vashworth vashworth requested a review from stuartmorgan October 9, 2023 15:13
@@ -21,7 +21,11 @@ tasks:
args: ["xcode-analyze", "--ios", "--ios-min-version=13.0"]
- name: native test
script: script/tool_runner.sh
args: ["native-test", "--ios", "--ios-destination", "platform=iOS Simulator,name=iPhone 14,OS=latest"]
args: ["native-test", "--ios", "--ios-destination", "platform=iOS Simulator,name=Flutter-iPhone,OS=16.4"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As we upgrade, there's no guarantee that there will be an iPhone 14 simulator. Better to use the simulator we create in the "create simulator" step (same for the pigeon test_suites.dart).

Copy link
Contributor

Choose a reason for hiding this comment

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

Pigeon's test_suites is a weird case; we need it to run locally as well as in CI. So we don't want to hard-code it to a simulator that only exists in CI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, so I updated Pigeon's test_suites to create and delete it's own simulator so that it'll work in CI and locally

FYI couple reasons I think that's the best approach:

  1. Due to new way Simulator runtimes are handled, when you use OS=latest or you omit the OS (which defaults to latest), it may use a newer simulator runtime that's not the default for the Xcode version. For example, if iOS 17 runtime is mounted, it'll use that instead of iOS 16.4 even if you're using Xcode 14.3. So even if there's an "iPhone 14" device with OS 16.4, it won't match against it since it's looking for iOS 17.
  2. There's no guarantee to be a Simulator named "iPhone 14" unless we create it beforehand (both in CI and locally)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also btw, I ran dart run tool/run_tests.dart in packages/packages/pigeon locally to make sure it passed locally


xcrun simctl boot "$DEVICE_NAME" || :
echo -e ""
xcrun simctl list
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to keep this echo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not needed, but I liked it - it puts a newline between the boot output and list output. Just gives it a little separation

@@ -21,7 +21,11 @@ tasks:
args: ["xcode-analyze", "--ios", "--ios-min-version=13.0"]
- name: native test
script: script/tool_runner.sh
args: ["native-test", "--ios", "--ios-destination", "platform=iOS Simulator,name=iPhone 14,OS=latest"]
args: ["native-test", "--ios", "--ios-destination", "platform=iOS Simulator,name=Flutter-iPhone,OS=16.4"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Pigeon's test_suites is a weird case; we need it to run locally as well as in CI. So we don't want to hard-code it to a simulator that only exists in CI.

args: ["native-test", "--ios", "--ios-destination", "platform=iOS Simulator,name=iPhone 14,OS=latest"]
args: ["native-test", "--ios", "--ios-destination", "platform=iOS Simulator,name=Flutter-iPhone,OS=16.4"]
- name: boot simulator
# Ensure simulator is still booted
Copy link
Contributor

Choose a reason for hiding this comment

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

That's really weird. Is there any indication of why it's shutting down?

@vashworth vashworth requested a review from stuartmorgan October 9, 2023 19:40
Copy link
Contributor

@stuartmorgan stuartmorgan left a comment

Choose a reason for hiding this comment

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

LGTM with nit (and with the .ci.yaml os changed to 12-or-13 per your comment).

@vashworth vashworth added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 10, 2023
@auto-submit auto-submit bot merged commit 4b483f2 into flutter:main Oct 10, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 10, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Oct 10, 2023
flutter/packages@7de08ec...4b483f2

2023-10-10 15619084+vashworth@users.noreply.github.com Run tests on either macOS 12 or macOS 13 (flutter/packages#5089)
2023-10-10 ychris@google.com [google_map_flutter_ios] fix `didBeginDraggingMarker` typo (flutter/packages#5085)
2023-10-09 stuartmorgan@google.com [process] Import the `process` package (flutter/packages#5095)
2023-10-09 hrkadam.92@gmail.com [go_router] Fixes missing state.extra in onException() (flutter/packages#5077)
2023-10-09 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 2.22.0 to 2.22.1 (flutter/packages#5094)
2023-10-09 engine-flutter-autoroll@skia.org Roll Flutter from 5207a30 to f52fe4f (18 revisions) (flutter/packages#5093)
2023-10-09 chrislangham@rightnow.org [platform] Example app for platform (flutter/packages#4834)
2023-10-09 49699333+dependabot[bot]@users.noreply.github.com Bump ossf/scorecard-action from 2.2.0 to 2.3.0 (flutter/packages#5091)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC flutter-ecosystem@google.com,rmistry@google.com 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
Mairramer pushed a commit to Mairramer/flutter that referenced this pull request Oct 10, 2023
flutter/packages@7de08ec...4b483f2

2023-10-10 15619084+vashworth@users.noreply.github.com Run tests on either macOS 12 or macOS 13 (flutter/packages#5089)
2023-10-10 ychris@google.com [google_map_flutter_ios] fix `didBeginDraggingMarker` typo (flutter/packages#5085)
2023-10-09 stuartmorgan@google.com [process] Import the `process` package (flutter/packages#5095)
2023-10-09 hrkadam.92@gmail.com [go_router] Fixes missing state.extra in onException() (flutter/packages#5077)
2023-10-09 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 2.22.0 to 2.22.1 (flutter/packages#5094)
2023-10-09 engine-flutter-autoroll@skia.org Roll Flutter from 5207a30 to f52fe4f (18 revisions) (flutter/packages#5093)
2023-10-09 chrislangham@rightnow.org [platform] Example app for platform (flutter/packages#4834)
2023-10-09 49699333+dependabot[bot]@users.noreply.github.com Bump ossf/scorecard-action from 2.2.0 to 2.3.0 (flutter/packages#5091)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC flutter-ecosystem@google.com,rmistry@google.com 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
HugoOlthof pushed a commit to moneybird/packages that referenced this pull request Dec 13, 2023
In preparation for migrating our fleet to macOS 13, we're updating tests to run on either macOS 12 or macOS 13.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App p: pigeon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants