Skip to content

Transition CI builds over to GitHub Actions#256

Merged
NickEntin merged 25 commits into
masterfrom
entin/gh-actions
Jun 12, 2021
Merged

Transition CI builds over to GitHub Actions#256
NickEntin merged 25 commits into
masterfrom
entin/gh-actions

Conversation

@NickEntin

Copy link
Copy Markdown
Collaborator

Travis CI no longer has free macOS builds, so it's time to migrate away. This moves our CI builds over to GitHub Actions.

@NickEntin NickEntin closed this Feb 11, 2021
@NickEntin NickEntin reopened this Mar 28, 2021
@codecov-io

Copy link
Copy Markdown

Codecov Report

Merging #256 (b6fef0e) into master (6305081) will increase coverage by 0.45%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #256      +/-   ##
==========================================
+ Coverage   86.25%   86.71%   +0.45%     
==========================================
  Files          16       16              
  Lines        1055      986      -69     
==========================================
- Hits          910      855      -55     
+ Misses        145      131      -14     
Impacted Files Coverage Δ
Sources/Valet/Internal/Service.swift 95.77% <0.00%> (-1.41%) ⬇️
Sources/Valet/SecureEnclaveAccessControl.swift 76.27% <0.00%> (-0.78%) ⬇️
Sources/Valet/Internal/Keychain.swift 89.00% <0.00%> (-0.50%) ⬇️
Sources/Valet/SecureEnclaveValet.swift 76.19% <0.00%> (+0.28%) ⬆️
Sources/Valet/SinglePromptSecureEnclaveValet.swift 75.75% <0.00%> (+0.49%) ⬆️
Sources/Valet/Valet.swift 94.55% <0.00%> (+4.48%) ⬆️

@NickEntin

Copy link
Copy Markdown
Collaborator Author

@dfed It looks like the macOS tests are failing due to missing entitlements. Any idea why this would have worked on Travis CI but not GitHub Actions? It should be running exactly the same commands, so presumably there's something different between the two environments?

@dfed

dfed commented Mar 31, 2021

Copy link
Copy Markdown
Collaborator

@NickEntin welp. Let's dig in.

Looking at one example test_setStringForKey_neutralizesMacOSAccessControlListVuln:

  • It's failing in CI
  • I'm not seeing a guard testEnvironmentIsSigned() in there, so we didn't expect it to fail in CI
  • I do see that it fails when I run it locally on my M1 Mac using 12.4 without going through the code-signing dance

I vaguely remember macOS 10.15 introducing changes that meant I had to code-sign the environment in order for macOS tests to run. Our xcode11.3 machine on Travis was running macOS 10.14.6, and our new build is on macOS 10.15.

So, with all the above info, I think we hit the macOS Catalina time-bomb. I hate to say it, but we should probably just delete the macOS tests from CI all-together. Speaking of which... I need to land #260 so folk know how on earth to test macOS.

@codecov-commenter

codecov-commenter commented Jun 9, 2021

Copy link
Copy Markdown

Codecov Report

Merging #256 (b9988d2) into master (a1b2e46) will increase coverage by 0.20%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #256      +/-   ##
==========================================
+ Coverage   86.60%   86.81%   +0.20%     
==========================================
  Files          16       16              
  Lines        1053      986      -67     
==========================================
- Hits          912      856      -56     
+ Misses        141      130      -11     
Impacted Files Coverage Δ
Sources/Valet/SecureEnclaveAccessControl.swift 76.27% <0.00%> (-6.78%) ⬇️
Sources/Valet/Internal/Keychain.swift 89.00% <0.00%> (-0.50%) ⬇️
Sources/Valet/SecureEnclaveValet.swift 76.19% <0.00%> (+0.28%) ⬆️
Sources/Valet/SinglePromptSecureEnclaveValet.swift 75.75% <0.00%> (+0.49%) ⬆️
Sources/Valet/Valet.swift 94.55% <0.00%> (+4.48%) ⬆️

@NickEntin

Copy link
Copy Markdown
Collaborator Author

Oh boo, turns out GitHub Actions doesn't actually support macOS 10.14, even though the virtual environments repo shows a specification for it. 😞 It silently uses macOS 10.15 instead.

Screen Shot 2021-06-08 at 10 50 10 PM

Screen Shot 2021-06-08 at 10 49 31 PM

There goes my workaround for testing macOS and running legacy iOS tests.

@NickEntin

Copy link
Copy Markdown
Collaborator Author

I'm not thrilled with what this does to our test matrix, but I think it's good enough for now and unblocks our CI.

Xcode Build and Test

Platform Before After
iOS 10, 11, 12, 13 12, 13, 14
tvOS 10, 11, 12, 13 12, 13, 14
watchOS 3, 4, 5, 6 5, 6, 7
macOS 10.15

Swift Package Manager Build

Platform Before After
iOS 13 13, 14
tvOS 13 13, 14
watchOS 6 6, 7
macOS 10.15 10.15

@NickEntin

Copy link
Copy Markdown
Collaborator Author
Test Case '-[Valet_tvOS_Tests.SinglePromptSecureEnclaveIntegrationTests test_allKeys_doesNotReflectValetImplementationDetails]' started.
2021-06-10 23:32:12.171511+0000 Valet tvOS Test Host App[2434:43850] [Client,SPI,LAContext] externalizedContext on LAContext[2434:1] cid:2 returned 0
/Users/runner/work/Valet/Valet/Tests/ValetIntegrationTests/SinglePromptSecureEnclaveIntegrationTests.swift:119: error: -[Valet_tvOS_Tests.SinglePromptSecureEnclaveIntegrationTests test_allKeys_doesNotReflectValetImplementationDetails] : XCTAssertEqual failed: threw error "KeychainError.couldNotAccessKeychain"
Test Case '-[Valet_tvOS_Tests.SinglePromptSecureEnclaveIntegrationTests test_allKeys_doesNotReflectValetImplementationDetails]' failed (0.291 seconds).
Test Case '-[Valet_tvOS_Tests.SinglePromptSecureEnclaveIntegrationTests test_allKeys]' started.
2021-06-10 23:32:12.461466+0000 Valet tvOS Test Host App[2434:43850] [Client,SPI,LAContext] externalizedContext on LAContext[2434:1] cid:3 returned 0
/Users/runner/work/Valet/Valet/Tests/ValetIntegrationTests/SinglePromptSecureEnclaveIntegrationTests.swift:97: error: -[Valet_tvOS_Tests.SinglePromptSecureEnclaveIntegrationTests test_allKeys] : XCTAssertEqual failed: threw error "KeychainError.couldNotAccessKeychain"
2021-06-10 23:32:12.503067+0000 Valet tvOS Test Host App[2434:43850] [Client,SPI,LAContext] externalizedContext on LAContext[2434:1] cid:4 returned 0
/Users/runner/work/Valet/Valet/Tests/ValetIntegrationTests/SinglePromptSecureEnclaveIntegrationTests.swift:100: error: -[Valet_tvOS_Tests.SinglePromptSecureEnclaveIntegrationTests test_allKeys] : XCTAssertEqual failed: threw error "KeychainError.couldNotAccessKeychain"
2021-06-10 23:32:12.507018+0000 Valet tvOS Test Host App[2434:43850] [Client,SPI,LAContext] externalizedContext on LAContext[2434:1] cid:5 returned 0
/Users/runner/work/Valet/Valet/Tests/ValetIntegrationTests/SinglePromptSecureEnclaveIntegrationTests.swift:103: error: -[Valet_tvOS_Tests.SinglePromptSecureEnclaveIntegrationTests test_allKeys] : XCTAssertEqual failed: threw error "KeychainError.couldNotAccessKeychain"
2021-06-10 23:32:12.511302+0000 Valet tvOS Test Host App[2434:43850] [Client,SPI,LAContext] externalizedContext on LAContext[2434:1] cid:6 returned 0
/Users/runner/work/Valet/Valet/Tests/ValetIntegrationTests/SinglePromptSecureEnclaveIntegrationTests.swift:106: error: -[Valet_tvOS_Tests.SinglePromptSecureEnclaveIntegrationTests test_allKeys] : XCTAssertEqual failed: threw error "KeychainError.couldNotAccessKeychain"
Test Case '-[Valet_tvOS_Tests.SinglePromptSecureEnclaveIntegrationTests test_allKeys]' failed (0.065 seconds).

🤔 Not sure why these can't access the keychain on tvOS 14.

cc @allisonmoyer this is the same test class you were looking at in #268

@NickEntin

NickEntin commented Jun 11, 2021

Copy link
Copy Markdown
Collaborator Author

Hmm, we're getting back a -26276 error code in the tvOS tests, which doesn't match any of the OSStatus codes. Not sure how to debug this one.

The closest one is errSecDecode, so maybe it has something to do with that?

errSecDecode                             = -26275,    /* Unable to decode the provided data. */

@NickEntin

Copy link
Copy Markdown
Collaborator Author

The tvOS test failures are reproducing locally, so I think it's a separate bug in our test (filed #270 to track this). Removing tvOS 14 from our test matrix for now.

@NickEntin NickEntin marked this pull request as ready for review June 11, 2021 03:34
@NickEntin NickEntin requested review from allisonmoyer and dfed June 11, 2021 03:35

@dfed dfed left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM! Thanks for taking this on.

@dfed dfed mentioned this pull request Jun 11, 2021
@NickEntin NickEntin merged commit bce7b39 into master Jun 12, 2021
@NickEntin NickEntin deleted the entin/gh-actions branch June 12, 2021 19:51
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.

5 participants