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

Standardize XCTest locations and harnesses #4005

Merged

Conversation

stuartmorgan-g
Copy link
Contributor

  • Moves XCTest files to the now-standard location
  • Ensures that the harnesses are called RunnerTests for consistency
  • Splits the image_picker unit tests out of the UI target into a new unit test target
  • Moves existing google_sign_in tests into the harness, since they weren't being run. One new test, added since we accidentally stopped compiling the file, was removed since it crashed other tests in the suite (which has non-trivial global state, so fixing it wasn't feasible here; I've follow up on the PR that added the test).

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. (Note that unlike the flutter/flutter repo, the flutter/plugins repo does use dart format. See plugin_tool format)
  • I signed the CLA.
  • The title of the PR starts with the name of the plugin 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.
  • I updated CHANGELOG.md to add a description of the change.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test exempt.
  • All existing and new tests are passing.

Copy link
Contributor

@cyanglaz cyanglaz left a comment

Choose a reason for hiding this comment

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

LGTM

@stuartmorgan-g
Copy link
Contributor Author

I'll look tomorrow to figure out what I broke.

CODE_SIGN_STYLE = Automatic;
GCC_C_LANGUAGE_STANDARD = gnu11;
INFOPLIST_FILE = RunnerTests/Info.plist;
IPHONEOS_DEPLOYMENT_TARGET = 14.5;
Copy link
Member

Choose a reason for hiding this comment

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

Don't override the project-level deployment target IPHONEOS_DEPLOYMENT_TARGET or TARGETED_DEVICE_FAMILY. I usually also don't override CLANG_*, GCC_* or MTL_* but that's less important.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I forgot Xcode generates a bunch of target-level settings by default. Fixed, and I'll go add that to the docs.

@jmagman
Copy link
Member

jmagman commented Jun 2, 2021

I'll look tomorrow to figure out what I broke.

Double-quoted include "FlutterAppDelegate.h" in framework header, expected angle-bracketed instead
Double-quoted include "FlutterMacros.h" in framework header, expected angle-bracketed instead
Double-quoted include "FlutterPlugin.h" in framework header, expected angle-bracketed instead
Double-quoted include "FlutterBinaryMessenger.h" in framework header, expected angle-bracketed instead
Double-quoted include "FlutterMacros.h" in framework header, expected angle-bracketed instead
Double-quoted include "FlutterChannels.h" in framework header, expected angle-bracketed instead
Double-quoted include "FlutterBinaryMessenger.h" in framework header, expected angle-bracketed instead
Double-quoted include "FlutterCodecs.h" in framework header, expected angle-bracketed instead
Double-quoted include "FlutterMacros.h" in framework header, expected angle-bracketed instead
Double-quoted include "FlutterCodecs.h" in framework header, expected angle-bracketed instead
Double-quoted include "FlutterPlatformViews.h" in framework header, expected angle-bracketed instead
Double-quoted include "FlutterCodecs.h" in framework header, expected angle-bracketed instead
Double-quoted include "FlutterMacros.h" in framework header, expected angle-bracketed instead
Double-quoted include "FlutterTexture.h" in framework header, expected angle-bracketed instead
Double-quoted include "FlutterMacros.h" in framework header, expected angle-bracketed instead
Double-quoted include "FlutterBinaryMessenger.h" in framework header, expected angle-bracketed instead
Double-quoted include "FlutterCallbackCache.h" in framework header, expected angle-bracketed instead
Double-quoted include "FlutterMacros.h" in framework header, expected angle-bracketed instead
Double-quoted include "FlutterChannels.h" in framework header, expected angle-bracketed instead
Too many errors emitted, stopping now
Could not build module 'Flutter'
Could not build module 'image_picker'
Testing cancelled because the build failed.

To import the Flutter framework, the new image_picker RunnerTests target shouldn't set CLANG_WARN_QUOTED_INCLUDE_IN_FRAMEWORK_HEADER=YES

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

@stuartmorgan-g
Copy link
Contributor Author

I'm not sure what I still have wrong in the new webview test target; I didn't see anything obvious comparing against one of the working projects.

@stuartmorgan-g
Copy link
Contributor Author

🤦🏻 Log reading fail on my part. Somehow I messed up the target membership of the test files in the webview test.

@stuartmorgan-g stuartmorgan-g merged commit 2a236e9 into flutter:master Jun 3, 2021
@stuartmorgan-g stuartmorgan-g deleted the xctest-location-standardization branch June 3, 2021 17:19
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 3, 2021
fluttergithubbot pushed a commit to flutter/flutter that referenced this pull request Jun 3, 2021
fotiDim pushed a commit to fotiDim/plugins that referenced this pull request Sep 13, 2021
- Moves XCTest files to the now-standard location
- Ensures that the harnesses are called RunnerTests for consistency
- Splits the image_picker unit tests out of the UI target into a new unit test target
- Moves existing google_sign_in tests into the harness, since they weren't being run.
  One new test, added since we accidentally stopped compiling the file, was removed
  since it crashed other tests in the suite (which has non-trivial global state, so fixing
  it wasn't feasible here; I've follow up on the PR that added the test).
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants