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

[camera] Remove OCMock from CameraExposureTests and CameraFocusTests #8351

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

mchudy
Copy link
Contributor

@mchudy mchudy commented Dec 27, 2024

Extracted from #8342 to keep PRs smaller

  • Introduces FLTDeviceOrientationProviding and FLTCaptureDeviceControlling protocols
  • Removes OCMock.h reference from CameraExposureTests.h and CameraFocusTests.h

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

Copy link
Contributor

@hellohuanlin hellohuanlin left a comment

Choose a reason for hiding this comment

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

Overall looks very good!

@hellohuanlin
Copy link
Contributor

CC @LouiseHsu who recently did a pretty big swift migration for in-app purchase plugin.

@mchudy mchudy force-pushed the feature/camera-ocmock-refactoring-part2 branch 2 times, most recently from e7d6b59 to c597ad3 Compare December 30, 2024 12:40
@mchudy
Copy link
Contributor Author

mchudy commented Dec 30, 2024

I'm not sure why the CI build fails. Locally it builds successfully. Do you have any tip maybe what can be the cause of this issue?

Failed to build iOS app
Lexical or Preprocessor Issue (Xcode): 'FLTCaptureDeviceControlling.h' file not found
/Volumes/Work/s/w/ir/x/w/packages/packages/camera/camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/include/camera_avfoundation/FLTCamMediaSettingsAVWrapper.h:7:8

@hellohuanlin
Copy link
Contributor

I'm not sure why the CI build fails. Locally it builds successfully. Do you have any tip maybe what can be the cause of this issue?

Failed to build iOS app
Lexical or Preprocessor Issue (Xcode): 'FLTCaptureDeviceControlling.h' file not found
/Volumes/Work/s/w/ir/x/w/packages/packages/camera/camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/include/camera_avfoundation/FLTCamMediaSettingsAVWrapper.h:7:8

it's been a while since i touched this, but it sounds like either module map or umbrella header. I'd look into a similar header and see how it is setup.

@hellohuanlin
Copy link
Contributor

I don't find a 2nd reviewer from the CODEOWNER for this plugin. Maybe @LouiseHsu who recently did a similar swift migration?

@mchudy mchudy force-pushed the feature/camera-ocmock-refactoring-part2 branch from 840af1d to 8256579 Compare December 30, 2024 22:47
@hellohuanlin hellohuanlin requested a review from jmagman December 30, 2024 23:20
@mchudy mchudy force-pushed the feature/camera-ocmock-refactoring-part2 branch 8 times, most recently from d65a805 to 04f1e8b Compare January 2, 2025 09:12
@mchudy
Copy link
Contributor Author

mchudy commented Jan 2, 2025

I fixed the CI, kind of a workaround (for some reason I cannot get this to work with subfolders). I'm gonna try to fix that in further PRs, but I don't want to block this one.

@mchudy mchudy force-pushed the feature/camera-ocmock-refactoring-part2 branch from 04f1e8b to d4c4f3a Compare January 14, 2025 10:01
packages/camera/camera_avfoundation/CHANGELOG.md Outdated Show resolved Hide resolved
// Format/Configuration
@property(nonatomic, strong) AVCaptureDeviceFormat *activeFormat;
@property(nonatomic, strong) NSArray<AVCaptureDeviceFormat *> *formats;
@property(nonatomic, copy) void (^setActiveFormatStub)(AVCaptureDeviceFormat *format);
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not clear from the naming what all of the "Stub" methods are; they should be documented. And if I'm understand the purpose of these methods correctly, it seems like on* rather than *Stub would be a more idiomatic (and thus clearer to readers) name.

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 took it from this design doc where this kind of naming was used https://docs.google.com/document/d/1XsaulkJA6_ZSpM7chkQLhQY25sQqhEMiqHMYzw8H85o/edit?resourcekey=0-_cUjF1c0iBvRLKfV-3gK5A&tab=t.0
I'm completely fine with replacing it with on* prefix though, for sure sounds clearer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I missed that in the doc. @hellohuanlin is there some source for that naming convention? The purpose of those methods does not at all match the use of "stub" I'm familiar with in tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

No source - IIRC I wrote XXStub simply because it's to replace OCMStub(...).andDo(...). I am fine with any convention we discuss here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, looking at the doc again, and the methods here, maybe there's just a mismatch in usage in this PR. In the doc it looks more like what I expect something called stub to be, which is replacing what the method does. In the usage here, it's basically just a notifier:

- (void)setFocusMode:(AVCaptureFocusMode)mode {
  _focusMode = mode;
  if (self.setFocusModeStub) {
    self.setFocusModeStub(mode);
  }
}

It seems like this is trying to both be a simple stub object (which takes state to return) and sort of a dynamic stub (which would use stub methods) at the same time, and sometimes the latter is really a stub and other times (like the above) it's not.

We should decide what pattern we want to use for read/write properties. If it's that the getter and setter have real implementations in the stub object, then any supplemental callback shouldn't be called "stub" IMO, because it has nothing to do with what the object returns. If it's that we have stub methods, then those should be the whole implementation, and the object should be stateless other than the stub methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, you're right, that's a mistake on my part. I also pictured those stub methods as having no default implementation so _focusMode = mode assignment shouldn't be there. I corrected this.

@mchudy mchudy requested a review from stuartmorgan January 16, 2025 14:47
@mchudy mchudy force-pushed the feature/camera-ocmock-refactoring-part2 branch from 81fe294 to 0670369 Compare January 17, 2025 14:14
@mchudy mchudy force-pushed the feature/camera-ocmock-refactoring-part2 branch from 0670369 to e3f1afd Compare January 17, 2025 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants