-
-
Notifications
You must be signed in to change notification settings - Fork 371
chore: Add test workflow for swift log #6945
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
base: feat/add-swift-log-integration
Are you sure you want to change the base?
Conversation
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| b06a366 | 1212.46 ms | 1242.20 ms | 29.74 ms |
| e39cfe2 | 1196.87 ms | 1216.73 ms | 19.87 ms |
| 587a9b5 | 1213.80 ms | 1250.94 ms | 37.14 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| b06a366 | 24.14 KiB | 1.02 MiB | 1016.90 KiB |
| e39cfe2 | 24.14 KiB | 1.02 MiB | 1016.90 KiB |
| 587a9b5 | 24.14 KiB | 1.02 MiB | 1016.89 KiB |
Previous results on branch: itay/add_ci_tests
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| cfb43c4 | 1209.69 ms | 1247.80 ms | 38.11 ms |
| 61ec528 | 1224.52 ms | 1228.96 ms | 4.43 ms |
| 458120a | 1227.60 ms | 1264.52 ms | 36.92 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| cfb43c4 | 24.14 KiB | 1.02 MiB | 1016.90 KiB |
| 61ec528 | 24.14 KiB | 1.01 MiB | 1012.90 KiB |
| 458120a | 24.14 KiB | 1.02 MiB | 1016.89 KiB |
philipphofmann
left a comment
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.
Thanks, I added a few comments.
scripts/integration-xcodebuild.sh
Outdated
| #!/bin/bash | ||
| set -euxo pipefail | ||
|
|
||
| # Disable SC1091 because it won't work with pre-commit |
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.
m: This script is quite similar to sentry-xcodebuild.sh. I would really appreciate if we don't have all of that logic duplicated.
| default: macos-26 | ||
| type: string | ||
|
|
||
| jobs: |
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.
m: There are many similarities to unit-test-common.yml. Can't we reuse unit-test-common.yml instead of having plenty of code duplicated?
| # Script to replace sentry-cocoa remote dependency with local path in integration packages | ||
| # This is useful for CI testing where we want to test integrations against the current code |
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.
m: I think this could be part of https://github.com/getsentry/sentry-cocoa/blob/main/scripts/prepare-package.sh
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 can move if there, but the only similarity is that both scripts manipulate Package.swift
But they are not so similar in which modifications they do
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat/add-swift-log-integration #6945 +/- ##
====================================================================
- Coverage 85.069% 85.065% -0.004%
====================================================================
Files 453 453
Lines 27681 27681
Branches 12167 12170 +3
====================================================================
- Hits 23548 23547 -1
- Misses 4087 4090 +3
+ Partials 46 44 -2 see 5 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
scripts/prepare-package.sh
Outdated
| fi | ||
|
|
||
| if is_enabled "$UPDATE_PATH_TO_SENTRY_COCOA"; then | ||
| sed -i '' 's|\.package(url: "https://github\.com/getsentry/sentry-cocoa", from: "[^"]*")|.package(path: "'../../../'")|g' "$PACKAGE_FILE" |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| XCODE_VERSION: ${{ inputs.xcode }} | ||
| DEVICE_NAME: ${{ inputs.device }} | ||
| OS_VERSION: ${{ inputs.test-destination-os }} | ||
| run: ./scripts/ci-boot-simulator.sh --xcode "$XCODE_VERSION" --device "$DEVICE_NAME" --os-version "$OS_VERSION" | ||
| run: ${{ inputs.path_to_scripts_folder }}/ci-boot-simulator.sh --xcode "$XCODE_VERSION" --device "$DEVICE_NAME" --os-version "$OS_VERSION" | ||
|
|
||
| # We split building and running tests in two steps so we know how long running the tests takes. | ||
| - name: Build Tests | ||
| working-directory: ${{ inputs.working_directory }} | ||
| id: build_tests | ||
| env: | ||
| PLATFORM: ${{ inputs.platform }} |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| if is_enabled "$UPDATE_PATH_TO_SENTRY_COCOA"; then | ||
| sed -i '' 's|\.package(url: "https://github\.com/getsentry/sentry-cocoa", from: "[^"]*")|.package(path: "'../../../'")|g' "$PACKAGE_FILE" | ||
| fi |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| with: | ||
| name: SwiftLog | ||
| scheme: SentrySwiftLog | ||
| working_directory: integrations/logs/sentry-cocoa-swiftlog | ||
| path_to_scripts_folder: ../../../scripts | ||
| xcode: ${{ matrix.xcode }} | ||
| test-destination-os: ${{ matrix.test-destination-os }} | ||
| device: ${{ matrix.device }} | ||
| platform: ${{ matrix.platform }} | ||
| runs-on: ${{ matrix.macos_version }} | ||
| timeout: 10 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
scripts/prepare-package.sh
Outdated
| fi | ||
|
|
||
| if is_enabled "$UPDATE_PATH_TO_SENTRY_COCOA"; then | ||
| sed -i '' 's|\.package(url: "https://github\.com/getsentry/sentry-cocoa", from: "[^"]*")|.package(path: "'../../../'")|g' "$PACKAGE_FILE" |
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.
Bug: Sed produces invalid Swift syntax with literal quotes
The sed command at line 143 produces invalid Swift package path syntax. The replacement pattern '.package(path: "'../../../'")' will generate .package(path: "'../../../'") in the Package.swift file, where the path string contains literal single quote characters. Swift package paths require just the path without extra quotes, like .package(path: "../../../"). The literal single quotes will cause the package resolution to fail because it will look for a directory named '../../../' instead of ../../../.
| if: always() | ||
| with: | ||
| report_paths: "build/reports/junit.xml" | ||
| report_paths: "${{ inputs.working_directory }}/build/reports/junit.xml" |
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.
Bug: Empty working_directory creates invalid absolute path prefix
The report_paths value "${{ inputs.working_directory }}/build/reports/junit.xml" will produce /build/reports/junit.xml when working_directory is not provided (defaults to empty string). This changes a relative path to an absolute path starting with /, breaking existing callers of unit-test-common.yml that don't specify working_directory (like fast-pr-checks.yml and test.yml). The previous value was simply "build/reports/junit.xml" without the prefix.
| -S|--spm-project) | ||
| SPM_PROJECT="true" | ||
| shift 1 | ||
| ;; |
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.
Bug: Argument parsing mismatch for spm-project flag
The --spm-project option's usage documentation (line 40) indicates it accepts a <bool> argument, but the implementation unconditionally sets SPM_PROJECT="true" and only does shift 1 instead of shift 2. All other options in this script read $2 and shift by 2. If a user runs --spm-project false following the documented interface, the variable is incorrectly set to "true" and then "false" is left as the next argument, causing an "Unknown option" error.
| uses: ./.github/workflows/unit-test-common.yml | ||
| with: | ||
| name: SwiftLog | ||
| scheme: SentrySwiftLog | ||
| working_directory: integrations/logs/sentry-cocoa-swiftlog | ||
| path_to_scripts_folder: ../../../scripts | ||
| xcode: ${{ matrix.xcode }} | ||
| test-destination-os: ${{ matrix.test-destination-os }} | ||
| device: ${{ matrix.device }} | ||
| platform: ${{ matrix.platform }} | ||
| runs-on: ${{ matrix.macos_version }} |
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.
Bug: sentry-xcodebuild.sh incorrectly adds -workspace Sentry.xcworkspace when testing SPM packages due to a missing --spm-project flag.
Severity: CRITICAL | Confidence: High
🔍 Detailed Analysis
The integrations-swift-log.yml workflow calls unit-test-common.yml to test an SPM package. However, unit-test-common.yml does not expose an input parameter to indicate an SPM project. Consequently, sentry-xcodebuild.sh is executed without the --spm-project true flag, causing it to incorrectly add -workspace Sentry.xcworkspace to xcodebuild arguments. This will fail because the SPM package directory (integrations/logs/sentry-cocoa-swiftlog/) does not contain a Sentry.xcworkspace file.
💡 Suggested Fix
Add an is_spm_project boolean input to unit-test-common.yml, pass it to sentry-xcodebuild.sh, and update integrations-swift-log.yml to pass is_spm_project: true.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: .github/workflows/integrations-swift-log.yml#L71-L85
Potential issue: The `integrations-swift-log.yml` workflow calls `unit-test-common.yml`
to test an SPM package. However, `unit-test-common.yml` does not expose an input
parameter to indicate an SPM project. Consequently, `sentry-xcodebuild.sh` is executed
without the `--spm-project true` flag, causing it to incorrectly add `-workspace
Sentry.xcworkspace` to `xcodebuild` arguments. This will fail because the SPM package
directory (`integrations/logs/sentry-cocoa-swiftlog/`) does not contain a
`Sentry.xcworkspace` file.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 5615901
| default: false | ||
| type: boolean | ||
|
|
||
| working_directory: |
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.
Bug: Missing default for working_directory in unit-test-common.yml breaks existing callers and causes workflow failures.
Severity: CRITICAL | Confidence: High
🔍 Detailed Analysis
The unit-test-common.yml workflow's working_directory input is required: false but lacks a default: value. Existing callers, such as test.yml, do not provide this parameter. When working_directory is an empty string, critical steps like ls -la ${{ inputs.working_directory }} will fail with ls: cannot access '': No such file or directory. Additionally, the report_paths for JUnit reports will incorrectly resolve to /build/reports/junit.xml, causing the report action to fail.
💡 Suggested Fix
Add a default: value to the working_directory input in unit-test-common.yml or update all existing callers (e.g., test.yml) to provide this parameter.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: .github/workflows/unit-test-common.yml#L72
Potential issue: The `unit-test-common.yml` workflow's `working_directory` input is
`required: false` but lacks a `default:` value. Existing callers, such as `test.yml`, do
not provide this parameter. When `working_directory` is an empty string, critical steps
like `ls -la ${{ inputs.working_directory }}` will fail with `ls: cannot access '': No
such file or directory`. Additionally, the `report_paths` for JUnit reports will
incorrectly resolve to `/build/reports/junit.xml`, causing the report action to fail.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 5615901
…/sentry-cocoa into itay/add_ci_tests
| run: | | ||
| ls -la . | ||
| ls -la sentry-cocoa | ||
| ls -la ${{ inputs.working_directory }} |
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.
Bug: Debug step will fail when sentry-cocoa directory missing
A debugging step named "Test" was accidentally committed to the workflow. The ls -la sentry-cocoa command will fail for any workflow invocation that doesn't have a sentry-cocoa directory present in the checkout, which includes existing workflows like test.yml and fast-pr-checks.yml that call this reusable workflow.
| device: ${{ matrix.device }} | ||
| platform: ${{ matrix.platform }} | ||
| runs-on: ${{ matrix.macos_version }} | ||
| timeout: 10 |
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.
Bug: Missing secrets inheritance for codecov token access
The test-integration job is missing secrets: inherit when calling the unit-test-common.yml reusable workflow. Other callers like test.yml and fast-pr-checks.yml include this directive. Without it, unit-test-common.yml cannot access secrets.CODECOV_TOKEN, causing the codecov upload steps to fail or behave unexpectedly.
🚨 Detected changes in high risk code 🚨High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:
|
Adds:
Closes #6973