-
-
Notifications
You must be signed in to change notification settings - Fork 354
Add API stability check #5288
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
Add API stability check #5288
Conversation
224e823
to
8fd7125
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5288 +/- ##
=============================================
+ Coverage 92.801% 92.894% +0.093%
=============================================
Files 688 688
Lines 86264 86300 +36
Branches 30089 31202 +1113
=============================================
+ Hits 80054 80168 +114
+ Misses 6113 6029 -84
- Partials 97 103 +6
... and 23 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
bfe863d | 1223.19 ms | 1236.23 ms | 13.04 ms |
d33f2c8 | 1233.67 ms | 1246.92 ms | 13.24 ms |
ebfe678 | 1234.63 ms | 1254.52 ms | 19.89 ms |
06548c0 | 1226.71 ms | 1252.37 ms | 25.66 ms |
216bdf9 | 1193.69 ms | 1217.90 ms | 24.21 ms |
154f795 | 1225.53 ms | 1231.04 ms | 5.51 ms |
2095ae0 | 1238.20 ms | 1251.37 ms | 13.17 ms |
27f970b | 1234.21 ms | 1243.98 ms | 9.77 ms |
bdd896e | 1211.19 ms | 1239.06 ms | 27.87 ms |
0589699 | 1233.49 ms | 1249.09 ms | 15.60 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
bfe863d | 21.58 KiB | 414.57 KiB | 392.99 KiB |
d33f2c8 | 22.31 KiB | 820.52 KiB | 798.20 KiB |
ebfe678 | 22.31 KiB | 775.27 KiB | 752.95 KiB |
06548c0 | 20.76 KiB | 427.36 KiB | 406.59 KiB |
216bdf9 | 21.58 KiB | 418.13 KiB | 396.54 KiB |
154f795 | 20.76 KiB | 435.25 KiB | 414.49 KiB |
2095ae0 | 21.90 KiB | 709.06 KiB | 687.16 KiB |
27f970b | 21.58 KiB | 706.97 KiB | 685.39 KiB |
bdd896e | 22.31 KiB | 780.75 KiB | 758.43 KiB |
0589699 | 21.58 KiB | 656.60 KiB | 635.02 KiB |
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 stopped short of having a breaking diff fail the CI check for two reasons:
Sometimes we do want to merge breaking changes, for example a feature that we are still working on and isn't released yet. Also adding a new default parameter to a function is considered a breaking change. Although it requires re-building the app since the exported symbol did change, it is still source code compatible and this tool doesn't check for that.
A false sense of security. Not every breaking change is caught by this, for example this is a Swift only tool so it won't detect if you drop an @objc annotation.
You can see the results of running this in the CI check logs. Here's a diff with no breaking changes
I think we can start with simply running this and see how it goes, but in the long run I think it would make sense that CI fails or adds a PR comment that it detected breaking changes. Otherwise, I think most people will ignore this check.
SPM has built in support for determining if a change breaks the public API of a framework using
swift package diagnose-api-breaking-changes
However I couldn't get this to work with a prebuilt binary. Internally that command usesswift-api-digester
which is not very documented but I found a few threads on like this one.This PR runs swift-api-digester to get a report of anything that change in the API which may be a breaking change. This does not do any checking of the report to prevent any change from landing, it just generates the report so if anyone is interested they can look at it on their PRs. I think it's good to have running in main as a sanity check, and to ensure the processes used to generate the report doesn't break.
I stopped short of having a breaking diff fail the CI check for two reasons:
@objc
annotation.You can see the results of running this in the CI check logs. Here's a diff with no breaking changes
This only checks iOS, but we could add others if this turns out to be useful!
#skip-changelog