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.

Sign up for GitHub

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

Jump to bottom

Use GitHub Actions for CI builds #80

Merged
merged 24 commits into from
Oct 19, 2020
Merged

Use GitHub Actions for CI builds #80

merged 24 commits into from
Oct 19, 2020

Conversation

NickEntin
Copy link
Collaborator

This moves us from Travis CI to GitHub actions, and parallelizes our builds a bit more, which should speed up CI times.

I also bumped to the latest (non-beta) release of CocoaPods, since we were a bit behind.

@NickEntin NickEntin requested a review from dfed July 23, 2020 23:33
@NickEntin NickEntin force-pushed the entin/github-actions branch 3 times, most recently from a3c4dbb to a2c72ce Compare July 24, 2020 00:19
@NickEntin
Copy link
Collaborator Author

NickEntin commented Jul 24, 2020 •
edited
Loading

Alright, this should be good now. Apologies for the noise.

The PR will still be marked as having missing checks, since it expects a Travis build. When I go to merge this, I'll switch the required checks over to GitHub Actions instead.

dfed
dfed requested changes Jul 24, 2020
View reviewed changes
Copy link
Collaborator

@dfed dfed left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I think this PR will require a major version bump, since as-is it This PR makes it too easy for us to accidentally drop support for Xcode 9.

.travis.yml Show resolved Hide resolved
AardvarkSample/AardvarkSample.xcodeproj/project.pbxproj Show resolved Hide resolved
AardvarkSample/AardvarkSample.xcodeproj/project.pbxproj Show resolved Hide resolved
dfed
dfed approved these changes Jul 24, 2020
View reviewed changes
Copy link
Collaborator

@dfed dfed left a comment

Choose a reason for hiding this comment

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

🙌

@NickEntin
Copy link
Collaborator Author

Since #82 bumps the minimum supported Xcode version to 11, I think we can go ahead and drop the Travis config, since we don't need to build on Xcode 9 now.

@NickEntin
Copy link
Collaborator Author

So, interestingly, the flaky test isn't coming from the expectation timeout inside the test, but rather from the expectation timeout in tearDown when it tries to clear the log store.

/Users/runner/work/Aardvark/Aardvark/AardvarkTests/ARKLogDistributorTests.m:109: error: -[ARKLogDistributorTests test_logDistribution_performance] : Asynchronous wait failed: Exceeded timeout of 5 seconds, with unfulfilled expectations: "tearDown".

I'll revert the commit to bump the timeout in the test and investigate why clearing the log store is taking so long.

@NickEntin
Copy link
Collaborator Author

Seeing the same failure in the Travis build on a different PR, so I don't think it's related to GitHub Actions.

@NickEntin
Copy link
Collaborator Author

Temporarily disabled the flaky test to unblock this PR and #82. Reverted my commits that were attempting to fix it.

@dfed If you're alright with disabling the test for now, I'll go ahead and merge this and file an issue to track re-enabling that test once we figure out why it was flaking.

dfed
dfed approved these changes Oct 19, 2020
View reviewed changes
Copy link
Collaborator

@dfed dfed left a comment

Choose a reason for hiding this comment

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

LGTM with some comments/questions

AardvarkTests/ARKLogDistributorTests.m
@@ -246,7 +246,7 @@ - (void)test_distributeAllPendingLogsWithCompletionHandler_informsLogObserversOf

#pragma mark - Performance Tests

- (void)test_logDistribution_performance;
- (void)disabled_test_logDistribution_performance;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we leave a comment here explaining why this is disabled? Maybe include in the comment what the failure we're seeing is?

Also: is this failure only on Github Workflow? Or also on Travis?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I haven't seen it on Travis in this PR, but I saw it in the Travis build on #82. I think it must be related to the Xcode version, although I haven't been able to repro locally, so there's likely other factors involved as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Addressed in e085b05

AardvarkSample/AardvarkSample.xcodeproj/xcshareddata/xcschemes/AardvarkSample.xcscheme Show resolved Hide resolved
.travis.yml
@@ -1,10 +0,0 @@
language: objective-c
osx_image: xcode9
Copy link
Collaborator

Choose a reason for hiding this comment

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

Approving this dropping of Xcode 9 CI since it looks like we're dropping support in #82.

.github/workflows/ci.yml
jobs:
xcode-build:
name: Xcode Build
runs-on: macOS-latest
Copy link
Collaborator

Choose a reason for hiding this comment

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

oooooh can we drop this version back to use something a little earlier than the latest?

Copy link
Collaborator Author

@NickEntin NickEntin Oct 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Unfortunately they don't have anything older than 10.15. 😞 Our options are 10.15 (latest released) or 11.0 (in beta).

@NickEntin
Update CocoaPods to 1.9.3
80a09f2
@NickEntin
Move from Travis CI to GitHub Actions for CI builds
451cdc3 
This also parallelizes our builds a bit more, which should speed up CI times.
@NickEntin
Update Bundler to 2.1.4 to match CI environment
4388909
@NickEntin
Fix incorrect path
a1ed7c9
@NickEntin
Use iPhone 11 Pro as CI device
4ea7d95 
The SE got renamed, so it can't find a matching device
@NickEntin
Update README to use GitHub Actions for CI status badge
3d0e807
@NickEntin
Update sample project to Swift 5
dc58196
@NickEntin
Build and lint pods with Xcode 9 using Travis
83435d6
@NickEntin
Revert "Update Bundler to 2.1.4 to match CI environment"
9fe8128 
This reverts commit ec68b5a.
@NickEntin
Use Bundler 1.17.3 for GitHub Actions
bc9e48c
@NickEntin
Increase timeout on flaky test
17b4114
@NickEntin
Remove Travis config
dc82b6e
@NickEntin
Revert "Use Bundler 1.17.3 for GitHub Actions"
456e1e8 
This reverts commit 0313360.
@NickEntin
Revert "Revert "Update Bundler to 2.1.4 to match CI environment""
32f4e56 
This reverts commit d19c596.
@NickEntin
Specify branch in README CI badge
8b63325
@NickEntin
Revert "Increase timeout on flaky test"
26134d5 
This reverts commit 74b414a.
@NickEntin
Bump tear down timeout to 10 seconds
4fccfd0
@NickEntin NickEntin merged commit 3212862 into master Oct 19, 2020
@NickEntin NickEntin mentioned this pull request Oct 20, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dfed dfed dfed approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@NickEntin @dfed