Skip to content

Add macOS tests (Part 1)#45644

Merged
fluttergithubbot merged 1 commit intoflutter:masterfrom
gspencergoog:macos_tests
Jan 17, 2020
Merged

Add macOS tests (Part 1)#45644
fluttergithubbot merged 1 commit intoflutter:masterfrom
gspencergoog:macos_tests

Conversation

@gspencergoog
Copy link
Copy Markdown
Contributor

@gspencergoog gspencergoog commented Nov 26, 2019 •
edited
Loading

Description

This contains the test changes for adding TargetPlatform.macOS, which was done in #43457.

The main goal of this PR is to enable tests that are currently running only on iOS to also run on macOS, but only for the tests where that makes sense. For instance, we don't run any of the haptic feedback tests on macOS.

Related Issues

Tests

  • Converts most iOS-only tests to be variant tests that test both iOS and macOS.

Breaking Change

  • No, this is not a breaking change.

@fluttergithubbot fluttergithubbot added a: tests "flutter test", flutter_test, or one of our tests d: examples Sample code and demos f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. c: contributor-productivity Team-specific productivity, code health, technical debt. customer: gallery Relating to flutter/gallery repository. Please transfer non-framework issues there. tool Affects the "flutter" command-line tool. See also t: labels. work in progress; do not review labels Nov 26, 2019
@gspencergoog gspencergoog force-pushed the macos_tests branch 2 times, most recently from c7619ce to 71fc3bd Compare November 27, 2019 03:35
@gspencergoog gspencergoog changed the title Macos tests Add macOS tests Nov 27, 2019
@gspencergoog gspencergoog force-pushed the macos_tests branch 2 times, most recently from 247eec7 to 4cedd37 Compare December 20, 2019 23:36
@gspencergoog gspencergoog marked this pull request as ready for review December 21, 2019 04:39
@gspencergoog gspencergoog force-pushed the macos_tests branch 7 times, most recently from b9900ce to 0958510 Compare January 13, 2020 17:57
@zanderso zanderso removed the tool Affects the "flutter" command-line tool. See also t: labels. label Jan 15, 2020
@shihaohong shihaohong self-assigned this Jan 16, 2020
shihaohong
shihaohong reviewed Jan 16, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@shihaohong shihaohong left a comment

Choose a reason for hiding this comment

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

I'm not 100% done with reviewing this yet, but I want to comment quickly about a few things I have to note before continuing:

  1. It might be tough to tell from the beginning whether or not the test applies to a particular platform or not if a developer is not expecting the variant property at the end of the test without having something like (iOS) or (Android) in the test name. I feel like where possible, it might be better to just leave in the target platforms in the test names to avoid confusion when interpreting a unit test.

  2. We should also probably rename this PR to something like Test refactor and list out potential changes in the description of the PR. It's not entirely accurate that it's just adding macOS tests -- this PR adds additional testing for Fuchsia in some parts, since Fuchsia was added as a target if I'm understanding the code correctly, and reintroduces some tests that work correctly now but were originally commented out due to bugs.

  3. Wouldn't it be better to separate these changes out into multiple PRs? I'm not very confident that I caught every case where something could have been accidentally altered or if the tests are correct since this PR touches many different types of tests. Looking through the PR, I am simply pattern-matching what the test used to test for and assuming the same pattern would apply to macOS correctly.

@gspencergoog gspencergoog removed d: examples Sample code and demos f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. c: contributor-productivity Team-specific productivity, code health, technical debt. customer: gallery Relating to flutter/gallery repository. Please transfer non-framework issues there. labels Jan 16, 2020
@gspencergoog
Copy link
Copy Markdown
Contributor Author

gspencergoog commented Jan 16, 2020 •
edited
Loading

Excellent point about breaking this up. I'm going to break this up into several different changes so they can be reviewed separately.

@gspencergoog gspencergoog changed the title Add macOS tests Add macOS tests (Part 1) Jan 16, 2020
@fluttergithubbot fluttergithubbot added the f: cupertino flutter/packages/flutter/cupertino repository label Jan 16, 2020
@gspencergoog
Copy link
Copy Markdown
Contributor Author

gspencergoog commented Jan 16, 2020 •
edited
Loading

OK, this PR now only contains changes from the cupertino, foundation, rendering, and services libraries.

The material library changes are now in #48996
The widgets library changes are now in #48997

@gspencergoog gspencergoog requested a review from justinmc January 16, 2020 21:21
shihaohong
shihaohong approved these changes Jan 17, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@shihaohong shihaohong 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 bf1d822 into flutter:master Jan 17, 2020
@gspencergoog gspencergoog deleted the macos_tests branch March 13, 2020 16:09
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@justinmc justinmc Awaiting requested review from justinmc

1 more reviewer

@shihaohong shihaohong shihaohong approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

a: tests "flutter test", flutter_test, or one of our tests f: cupertino flutter/packages/flutter/cupertino repository framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@gspencergoog @shihaohong @jmagman @googlebot @zanderso @fluttergithubbot