Skip to content

Update number of IPHONEOS_DEPLOYMENT_TARGET in plugin_test_ios #91324

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 2 commits into from
Oct 7, 2021
Merged

Update number of IPHONEOS_DEPLOYMENT_TARGET in plugin_test_ios #91324

merged 2 commits into from
Oct 7, 2021

Conversation

jmagman
Copy link
Member

@jmagman jmagman commented Oct 5, 2021 •
edited
Loading

Change the integration test #72764 to explicitly check that the too-low plugin version number has been removed, instead of the expected build setting count. Which, as expected, was brittle...

Follow up to #91099
Will allow this test to be turned back on #88794

Task result:
{
  "success": true,
  "data": null,
  "detailFiles": [],
  "benchmarkScoreKeys": [],
  "reason": "success"
}

@jmagman jmagman added a: tests "flutter test", flutter_test, or one of our tests platform-ios iOS applications specifically labels Oct 5, 2021
@jmagman jmagman self-assigned this Oct 5, 2021
@flutter-dashboard flutter-dashboard bot added the c: contributor-productivity Team-specific productivity, code health, technical debt. label Oct 5, 2021
@google-cla google-cla bot added the cla: yes label Oct 5, 2021
@jmagman jmagman mentioned this pull request Oct 5, 2021
Closed
@jmagman jmagman requested a review from stuartmorgan-g October 6, 2021 00:11
stuartmorgan-g
stuartmorgan-g approved these changes Oct 6, 2021
View reviewed changes
Copy link
Contributor

@stuartmorgan-g stuartmorgan-g left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, but could you update the PR description to explain why the number is changing, so that it's clear to future people why this is correct (and not masking a regression as described in the comment just above this code)?

dev/devicelab/lib/tasks/plugin_tests.dart Outdated
@@ -179,7 +179,7 @@ class _FlutterProject {
// The plugintest target should not have IPHONEOS_DEPLOYMENT_TARGET set.
// See _reduceDarwinPluginMinimumVersion for details.
final int iosDeploymentTargetCount = 'IPHONEOS_DEPLOYMENT_TARGET'.allMatches(podsProjectContent).length;
if (iosDeploymentTargetCount != 9) {
if (iosDeploymentTargetCount != 12) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the comment above that says "6 times" be updated too? (It already didn't match, so maybe there's a distinction between the two numbers that I'm missing.)

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right, this is lame. Let me rework this check.

Copy link
Member Author

Choose a reason for hiding this comment

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

I updated the description, test logic, and comment.

This may be a bit brittle

Thanks, past Jenn.

stuartmorgan-g
stuartmorgan-g approved these changes Oct 7, 2021
View reviewed changes
Copy link
Contributor

@stuartmorgan-g stuartmorgan-g left a comment

Choose a reason for hiding this comment

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

LGTM

@fluttergithubbot fluttergithubbot merged commit 4ef6fc1 into flutter:master Oct 7, 2021
@jmagman jmagman deleted the plugin-test-count branch October 7, 2021 17:18
clocksmith pushed a commit to clocksmith/flutter that referenced this pull request Oct 29, 2021
@jmagman @clocksmith
Update number of IPHONEOS_DEPLOYMENT_TARGET in plugin_test_ios (flutt…
98ae927 
…er#91324)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stuartmorgan-g stuartmorgan-g stuartmorgan-g approved these changes

Assignees

@jmagman jmagman

Labels
a: tests "flutter test", flutter_test, or one of our tests c: contributor-productivity Team-specific productivity, code health, technical debt. platform-ios iOS applications specifically
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jmagman @stuartmorgan-g @fluttergithubbot