Skip to content

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

Merged
merged 1 commit into from
May 28, 2025
Merged

Add API stability check #5288

merged 1 commit into from
May 28, 2025

Conversation

noahsmartin
Copy link
Contributor

@noahsmartin noahsmartin commented May 25, 2025

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 uses swift-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:

  1. 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.
  2. 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

/* Generic Signature Changes */
/* RawRepresentable Changes */
/* Removed Decls */
/* Moved Decls */
/* Renamed Decls */
/* Type Changes */
/* Decl Attribute changes */
/* Fixed-layout Type Changes */
/* Protocol Conformance Change */
/* Protocol Requirement Change */
/* Class Inheritance Change */
/* Others */

This only checks iOS, but we could add others if this turns out to be useful!

#skip-changelog

@noahsmartin noahsmartin force-pushed the apiStability branch 6 times, most recently from 224e823 to 8fd7125 Compare May 25, 2025 21:30
Copy link

codecov bot commented May 25, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.894%. Comparing base (63307ef) to head (6bd8203).
Report is 9 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@              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     
Files with missing lines Coverage Δ
...ift/Integrations/UserFeedback/SentryFeedback.swift 72.727% <100.000%> (ø)

... and 23 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 63307ef...6bd8203. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

github-actions bot commented May 25, 2025

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1226.69 ms 1246.80 ms 20.10 ms
Size 23.76 KiB 821.43 KiB 797.67 KiB

Baseline results on branch: main

Startup times

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

@noahsmartin noahsmartin marked this pull request as ready for review May 27, 2025 13:25
@noahsmartin noahsmartin changed the title api check Add API stability check May 27, 2025
Copy link
Member

@philipphofmann philipphofmann left a 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.

@noahsmartin noahsmartin merged commit 18d1360 into main May 28, 2025
16 checks passed
@noahsmartin noahsmartin deleted the apiStability branch May 28, 2025 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants