Expand Test Execution Coverage #979
Conversation
joeydong-stripe
force-pushed
the
joeydong/testExecutionCoverage
branch
2 times, most recently
from
1723dd2 to
63fe9c0
Compare
joeydong-stripe
force-pushed
the
joeydong/testExecutionCoverage
branch
3 times, most recently
from
f4d45bd to
70d738a
Compare
joeydong-stripe
force-pushed
the
joeydong/testExecutionCoverage
branch
from
70d738a to
b94736b
Compare
joeydong-stripe
force-pushed
the
joeydong/testExecutionCoverage
branch
from
b94736b to
4a00bf0
Compare
joeydong-stripe
changed the title
Tests/Tests/STPColorUtilsTest.m
Outdated
| // CMYK is the only one where all 1's results in a dark color | ||
| testColorSpace(colorSpaceNames[i], colorSpaceNames[i] != kCGColorSpaceGenericCMYK); | ||
| testColorSpace(name, name != (__bridge NSString *)kCGColorSpaceGenericCMYK); |
There was a problem hiding this comment.
Interesting, I would have assumed we needed to use [NSString isEqualToString:] but the tests are passing anyway?
There was a problem hiding this comment.
I'm less confident about keeping this as a pointer equality check with the bridging involved. I think it's probable that CFStringRef is always toll-free bridged to NSString and this'll be fine, but I think it's also plausible that some future runtime changes that.
There was a problem hiding this comment.
Yeah I agree it's not smart to leave it as a pointer check. I'm more wondering why it still works. Maybe this constant is always mapped to the same memory address. Anyway, I'll switch it to the usual isEqualToString.
Tests/Tests/STPColorUtilsTest.m
Outdated
|
|
||
| if (@available(iOS 10.0, *)) { | ||
| colorSpaceNames = [colorSpaceNames arrayByAddingObjectsFromArray:@[ | ||
| (__bridge NSString *)kCGColorSpaceLinearGray, |
There was a problem hiding this comment.
Instead of adding all this (ugly?) bridging, since it was only two iOS 10 cases, did you consider just following the pattern that the iOS 11-only case kCGColorSpaceGenericLab took?
There was a problem hiding this comment.
Oh yeah, I was so focused on struggling with this Core Foundation syntax I forgot to try that.
Tests/Tests/STPColorUtilsTest.m
Outdated
| // CMYK is the only one where all 1's results in a dark color | ||
| testColorSpace(colorSpaceNames[i], colorSpaceNames[i] != kCGColorSpaceGenericCMYK); | ||
| testColorSpace(name, name != (__bridge NSString *)kCGColorSpaceGenericCMYK); |
There was a problem hiding this comment.
I'm less confident about keeping this as a pointer equality check with the bridging involved. I think it's probable that CFStringRef is always toll-free bridged to NSString and this'll be fine, but I think it's also plausible that some future runtime changes that.
| -scheme "Standard Integration (Swift)" \ | ||
| -sdk "iphonesimulator" \ | ||
| -destination "platform=iOS Simulator,name=iPhone 6,OS=11.2" \ | ||
| | xcpretty |
There was a problem hiding this comment.
Was it intentional to drop the -c flag that colorizes the output?
There was a problem hiding this comment.
Yeah, to me, the default behavior seems to works so why bother specifying it:
| # Execute tests (iPhone 6 @ iOS 11.2) | ||
| info "Executing tests (iPhone 6 @ iOS 11.2)..." | ||
|
|
||
| xcodebuild clean test \ |
There was a problem hiding this comment.
Why did you combine the build & test steps? They were intentionally separated last year: eb8009a
I know I've run into this problem in the past (and have the radar to prove it), but I don't know if it's still an issue in Xcode 9.
There was a problem hiding this comment.
i combined it to reduce code duplication, and it seems to work? i think if we need it again in the future, we should leave the reasoning as a comment in the code because a bum like me is going to miss the commit blame.
ci_scripts/run_tests.sh
Outdated
| -scheme "StripeiOS" \ | ||
| -configuration "Debug" \ | ||
| -sdk "iphonesimulator" \ | ||
| -destination "platform=iOS Simulator,name=iPhone 4s,OS=9.3" \ |
There was a problem hiding this comment.
What happens when you try to stick this in the previous xcodebuild command? Does it fail because Debug only builds for the active architecture?
Did you consider DRYing this up instead of duplicating so many parameters?
There was a problem hiding this comment.
What happens when you try to stick this in the previous xcodebuild command? Does it fail because Debug only builds for the active architecture?
Oops, forgot to leave a reason comment but yeah that's why. Never figured it out but your guess seems reasonable! The code duplication is definitely annoying. I haven't tried parameters in a variable before but I'll look into it.
There was a problem hiding this comment.
Duplication doesn't look too bad after adding the ONLY_ACTIVE_ARCH=NO usage
This reverts commit 3bdab3c.
…of trying to append to overall array
joeydong-stripe
force-pushed
the
joeydong/testExecutionCoverage
branch
from
a0d3392 to
2fc6b5b
Compare
Seems like Travis CI picks up our builds relatively quickly now so I think it's an appropriate time to expand our iOS version test coverage. Also would catch problems like #978. See individual commits.