-
Notifications
You must be signed in to change notification settings - Fork 217
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
Actually fail CI when tests fail #313
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dfed--watch-ci #313 +/- ##
=================================================
Coverage ? 82.52%
=================================================
Files ? 16
Lines ? 984
Branches ? 0
=================================================
Hits ? 812
Misses ? 172
Partials ? 0 |
@@ -11,18 +11,15 @@ concurrency: | |||
cancel-in-progress: true | |||
|
|||
jobs: | |||
xcode-build-12: | |||
name: Xcode 12 Build | |||
runs-on: macOS-11 |
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.
.watchOS_9, | ||
.watchOS_10: | ||
return "watchsimulator" | ||
} | ||
} | ||
|
||
var shouldTest: Bool { |
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.
well this is nice at least! always true
!
exit(0) | ||
throw TaskError.code(1) |
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.
From #300. Reverting so we can catch errors.
do { | ||
try execute(commandPath: "/usr/bin/xcodebuild", arguments: xcodeBuildArguments) | ||
} catch { | ||
print("xcodebuild failed with error: \(error)") | ||
} | ||
try execute(commandPath: "/usr/bin/xcodebuild", arguments: xcodeBuildArguments) |
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.
From #300. This was the big bad.
guard testEnvironmentIsSignedOrDoesNotRequireEntitlement() else { | ||
return | ||
} |
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.
on recent Xcodes, shared group valets require a signed environment. Which makes sense... but is annoying.
@@ -894,7 +922,7 @@ class ValetIntegrationTests: XCTestCase | |||
let identifier = "Keychain_With_Account_Name_As_NSData" | |||
|
|||
// kSecAttrAccount entry is expected to be a CFString, but a CFDataRef can also be stored as a value. | |||
let keychainData = [ | |||
let keychainData: CFDictionary = [ |
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.
Why does Xcode 13 require this? No idea. But it does. Swift compiler bug I guess.
@@ -0,0 +1,11 @@ | |||
{ |
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.
our old watchOS target was old. Let's update it. This belonged in #311 but, well, CI told us everything worked 😅
* Get watchOS tests running in CI * Run the tests * Parallelize (#312) * Actually fail CI when tests fail (#313) * Actually fail CI when tests fail * Throw when we encounter errors * Remove builds with Xcode that do not support Swift Concurrency * Use appropriate Xcode for the job * More tests require signed environments now * Fix macOS 14 test sdk * Use correct simulator name * Do not embed watchOS * Massage compiler * Remove unnecessary Valet watchOS Test Host App.app * New watch host target * simpler * stop testing watchOS_8 * Xcode 13 or later
Uh oh.
This regressed in #300