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

[image_picker] Refactored tests to expose private interface in separate test header. #4722

Merged
merged 5 commits into from
Feb 8, 2022

Conversation

mvanbeusekom
Copy link
Contributor

@mvanbeusekom mvanbeusekom commented Feb 2, 2022

Refactored the existing unit tests to expose private interfaces in a separate test header instead of an inline interface. This was first introduced in the google_sign_in plugin (#4157) and after that also applied in the camera plugin (#4426) and the webview_flutter plugin (#4480).

I have moved this refactoring out of the changes I am working on for issue flutter/flutter#96903 and PR #4718.

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/plugins repo does use dart 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], 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.

Refactored the existing unit tests to expose private interfaces in a
separate test header instead of an inline interface. This was first
introduced in the google_sign_in plugin (flutter#4157) and after that also
applied in the camera plugin (flutter#4426) and the webview_flutter plugin
(flutter#4480).
@@ -6,6 +6,7 @@

@import image_picker;
@import XCTest;
#import <image_picker/FLTImagePickerImageUtil.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this still relying on the bug in the framework (that it includes this header in the packaged framework at all)?

Pre module-map I would expect this to just be #import "FLTImagePickerImageUtil.h" with appropriate include paths for the target. I expect there's probably a cleaner way using the Test module though?

Copy link
Contributor

Choose a reason for hiding this comment

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

@mvanbeusekom i think you can put this header under the submodule Test that you've created, and just do @import image_picker.Test; here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The solution proposed by @hellohuanlin worked like a charm. Thanks

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, thanks for getting into the weeds with this.

@jmagman
Copy link
Member

jmagman commented Feb 8, 2022

@stuartmorgan this needs another review since you requested changes.

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.

I'm not sure I'm sold on the specific things being exposed in the test header here, but we can tune that in the test PR if necessary; since the important thing is the structural change here, LGTM.

@mvanbeusekom mvanbeusekom added the waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. label Feb 8, 2022
@fluttergithubbot fluttergithubbot merged commit 039068e into flutter:main Feb 8, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
p: image_picker platform-ios waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants