-
Notifications
You must be signed in to change notification settings - Fork 71
refactor: Port MParticleWebView to Swift #319
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
refactor: Port MParticleWebView to Swift #319
Conversation
BrandonStalnaker
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few questions
| XCTAssertEqualObjects(_webView.originalDefaultAgent, defaultAgent); | ||
| } | ||
|
|
||
| - (void)testBackgroundCollection { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this test and the other removed for any particular reason? I'm not against it as they seem to be more testing mocking and Apple code but I'm curious if that's your reson
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yesh, so the 2 tests I removed were because OCMock wasn't properly detecting the call of the method (the "expect") even though it was 100% being called, confirmed via breakpoints. Seems to be some incompatibility with OCMock and Swift. In any case, when I looked closer at what was really being tested, it was just testing that a method was called that is literally always called, it wasn't really testing the logic.
Basically on iOS evaluateAgent is always called after calling startWithCustomUserAgent and on tvOS it never is, so this and the other test I removed ultimately were just testing that the #if os(iOS) worked, which isn't really useful.
BrandonStalnaker
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Summary
Testing Plan
Reference Issue (For mParticle employees only. Ignore if you are an outside contributor)