-
Notifications
You must be signed in to change notification settings - Fork 47
[chore]: update CI config #343
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
Conversation
89bec63
to
626eec6
Compare
guard #unavailable(iOS 18.0) else { | ||
try XCTSkipIf(true, "Intrinsic content size does not update unless hosted in an actual app on iOS 18") | ||
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.
not sure what a better solution to this problem is (iOS 18 runtime seemingly doesn't invalidate/update VCs in the same way it used to)... use app hosted unit tests?
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.
use app hosted unit tests?
Yep. I've got a fix stashed if you want it.
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.
sounds good, but let's do as a separate PR then (unless it requires very little code change)
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.
this 'workaround' didn't actually seem to work around the problem for some reason, so i've just disabled the test currently
runs-on: macos-latest | ||
|
||
strategy: | ||
fail-fast: false # Don’t fail-fast so that we get all snapshot test changes |
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.
not sure this is actually needed, but copied it from the Blueprint config
|
||
- name: Install iOS ${{ matrix.sdk }} | ||
if: ${{ matrix.installation_required }} | ||
run: sudo xcodes runtimes install "iOS ${{ matrix.sdk }}" |
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.
How badly do you want to unit test on iOS 16? xcodes has a reputation of breaking a lot, I'd advise against depending on it 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.
what things have you seen break? personally i don't feel too strongly, but figure we might as well try hitting all the versions we support if it's not too hard
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.
seems like it adds a couple additional minutes to the CI time for that job. if it ends up being an issue i think we can probably drop it
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.
what things have you seen break?
Apple changes something on their backend and it just breaks this completely, blocking PRs in the meantime. The last time it happened we ended up disabling the check in Blueprint because it took more than several days to resolve.
guard #unavailable(iOS 18.0) else { | ||
try XCTSkipIf(true, "Intrinsic content size does not update unless hosted in an actual app on iOS 18") | ||
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.
use app hosted unit tests?
Yep. I've got a fix stashed if you want it.
6baeabb
to
8c492ff
Compare
8c492ff
to
89dda04
Compare
updates the CI config with the following changes:
i. this is largely copied from the analogous logic in the Blueprint CI config
ii. runs tests against the 3 most recent iOS SDKs (16.4, 17.5, 18.1)
i. this was primarily due to not knowing how to configure them to function correctly with the test matrix – something that should presumably be improved, but is no worse than what we're currently doing.
i. TODO: re-enable by adding support for app-hosted unit tests (tracked via this issue)