-
Notifications
You must be signed in to change notification settings - Fork 7
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
feat: iOS interop [WPB-15034] #850
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #850 +/- ##
=======================================
Coverage 76.20% 76.20%
=======================================
Files 113 113
Lines 20688 20688
=======================================
Hits 15766 15766
Misses 4922 4922 Continue to review full report in Codecov by Sentry.
|
5c5faef
to
eb2eb85
Compare
3163b52
to
f4c5666
Compare
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.
Approved with a single comment
0dc7182
to
20496b2
Compare
Could you please fold the fixup commit into the commit introducing the change the fixup commit is touching? |
Of course, I thought it's nicer to leave them as fixups while reviewing so you can see the only the relevant changes I make. |
interop/src/clients/InteropClient/InteropClient.xcodeproj/project.pbxproj
Show resolved
Hide resolved
20496b2
to
df98f2d
Compare
Regarding commit sequencing, I think it would be better if we modify That way, the first one would just add the |
if: startsWith(matrix.task, 'ios-simulator-arm') | ||
uses: actions/upload-artifact@v4 | ||
with: | ||
name: ${{github.event.number}}-swift-ffi |
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.
Here we're using github.event.number
, elsewhere we're using github.run_id
to uniquely name artifacts. I think we should be consistent, and use github.run_id
everywhere, unless there is a good reason to prefer github.event.number
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.
I just moved the job into separate file maybe @coriolinus can give a reason.
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 didn't touch that actually; it appears to come from 1ce7d95, which is @SimonThormeyer.
I don't think there's a strong reason here to prefer either one here. TBH we'd probably be fine without a distinguishing number in either case; looking deeper into the download artifact action, it automatically partitions by run id anyway. Adding an event number or run id just helps us keep track of things if we happen to download some artifacts for manual inspection.
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.
looking deeper into the download artifact action, it automatically partitions by run id anyway
Ah, didn't know that, thanks!
Was there a successful run of |
I think the migration logs are unrelated that's coming from the the ffi client but afterwards the web client is initialised and that got stuck apparently. |
887cc81
to
f85f214
Compare
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.
Looks good to me. 👍
I'm curious about the total CI time now, as github sometimes really takes a long time to give a macos runner.
It can a really long time |
f85f214
to
d317986
Compare
- install_app.sh installs the locally build app into a simulator device. - grant_permission.sh grants permission for opening the app from an URL.
The chrome webdriver is crashing with: V8 process OOM (Failed to reserve virtual memory for CodeRange) Upgrading the the latest Chrome (currently 134.0.6962.0) resolves the crash.
d317986
to
54f9a86
Compare
What's new in this PR
Adds an iOS client to the e2e interop tests
PR Submission Checklist for internal contributors
SQPIT-764
feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764
.