-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
ci: Add code integration testing to repo #1569
Conversation
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.
Thank you for submitting this PR @cbaker6!
This could definitely be helpful moving forward. Just a few comments.
.github/workflows/ci.yml
Outdated
@@ -0,0 +1,27 @@ | |||
name: CI |
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.
It would probably be helpful to make this name a bit more descriptive if possible.
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.
Do you have something in mind? I think the default action name GitHub provides for this is swift
and it usually is the same name of the file, swift.yml
.
If there was a special build.yml
and release.yml
then we could name them build
and release
. Do you think build
works better here?
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.
Because of the current name, this will show as ci / test (platform=iOS\ Simulator,OS=17.2,name=iPhone\ 15\ Pro\ Max, ResearchKit) (pull_request)
and ci / test (platform=iOS\ Simulator,OS=17.4,name=iPhone\ 15\ Pro\ Max, ORKCatalog) (pull_request)
in the build view and settings which is more specific. For example:
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.
I think having it say something along the lines of Build / RK Unit Tests or something close would be fine.
If we could avoid displaying the destination information here (platform, simulator, etc) that would also be great.
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.
.github/workflows/ci.yml
Outdated
|
||
on: | ||
push: | ||
branches: [ 'main' ] |
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.
Could we include stable as well for push and pull requests?
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.
Done!
.github/workflows/ci.yml
Outdated
|
||
jobs: | ||
test: | ||
runs-on: macos-13 |
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.
Could we update this to macos-latest?
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.
Github action still defaults macos-latest
to version 12.x.x
which is missing the latest Xcode https://github.com/actions/runner-images/blob/macOS-12/20240329.1/images/macos/macos-12-Readme.md#xcode
I've switched this to macos-14
instead. Let me know if this works
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.
It's saying it defaults to 14 here. But specifying macOS-14 is fine! Thanks for updating.
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.
I think they haven't updated the latest
images to point to macOS 14, when I tried latest, it should it's macOS 12 https://github.com/cbaker6/ResearchKit/actions/runs/8620787466/job/23628313753#step:1:8
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.
Remove catalog changes.
.github/workflows/ci.yml
Outdated
strategy: | ||
matrix: | ||
destination: ['platform=iOS\ Simulator,OS=17.4,name=iPhone\ 15\ Pro'] | ||
scheme: ['ResearchKit', 'ORKCatalog'] |
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.
Lets keep this specific for testing ResearchKit only for now.
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.
Do you want me to move all of the ORKCatalog
test updates out of this PR? I've tested the changes locally and they all pass now
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.
Yes, keeping the catalog app as is for now would be best.
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.
Reverted, moved to #1570
.github/workflows/ci.yml
Outdated
@@ -0,0 +1,27 @@ | |||
name: CI |
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.
I think having it say something along the lines of Build / RK Unit Tests or something close would be fine.
If we could avoid displaying the destination information here (platform, simulator, etc) that would also be great.
Thanks @cbaker6! Really appreciate you taking the time to make the updates. |
I believe you have to approve the PR for it to run the CI for the first time.
You can see the CI results on my fork: cbaker6#3