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

Actually fail CI when tests fail #313

Merged
merged 14 commits into from
Apr 23, 2024
Merged

Actually fail CI when tests fail #313

merged 14 commits into from
Apr 23, 2024

Conversation

dfed
Copy link
Collaborator

@dfed dfed commented Apr 23, 2024

Uh oh.

This regressed in #300

@dfed dfed self-assigned this Apr 23, 2024
@codecov-commenter
Copy link

codecov-commenter commented Apr 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (dfed--watch-ci@c6cf43a). Click here to learn what that means.

❗ Current head de5f1a9 differs from pull request most recent head a0bb5e7. Consider uploading reports for the commit a0bb5e7 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@                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
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

well I broke Xcode 12 in addition to Xcode 11 builds in #308. Not ideal, but for the same reason we let #308 through, we're okay letting this change through.

Latest supported Xcode is 14. It'll be 15 on Friday.

.watchOS_9,
.watchOS_10:
return "watchsimulator"
}
}

var shouldTest: Bool {
Copy link
Collaborator Author

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!

Comment on lines -281 to +218
exit(0)
throw TaskError.code(1)
Copy link
Collaborator Author

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.

Comment on lines -352 to +289
do {
try execute(commandPath: "/usr/bin/xcodebuild", arguments: xcodeBuildArguments)
} catch {
print("xcodebuild failed with error: \(error)")
}
try execute(commandPath: "/usr/bin/xcodebuild", arguments: xcodeBuildArguments)
Copy link
Collaborator Author

@dfed dfed Apr 23, 2024

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.

Comment on lines +368 to +370
guard testEnvironmentIsSignedOrDoesNotRequireEntitlement() else {
return
}
Copy link
Collaborator Author

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 = [
Copy link
Collaborator Author

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 @@
{
Copy link
Collaborator Author

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 😅

@dfed dfed marked this pull request as ready for review April 23, 2024 07:38
@dfed dfed mentioned this pull request Apr 23, 2024
Base automatically changed from dfed--more-machines to dfed--watch-ci April 23, 2024 15:25
@dfed dfed merged commit a292670 into dfed--watch-ci Apr 23, 2024
23 checks passed
@dfed dfed deleted the dfed--fail branch April 23, 2024 15:33
dfed added a commit that referenced this pull request Apr 23, 2024
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants