Skip to content
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 StagehandTesting to SPM package, update CI #76

Merged
merged 1 commit into from
Sep 26, 2024

Conversation

luispadron
Copy link
Collaborator

Exposes StagehandTesting via the Package.swift. Cleans up some more Bazel references and updates CI to test SPM via xcodebuild which allows building the package targets but with the correct platforms. Using swift build it was trying to link the macOS version of XCTest even though i was specifying the iOS sdk 🤷🏼

@luispadron luispadron requested a review from jszumski September 20, 2024 19:16
@luispadron luispadron force-pushed the luis/add-stagehand-testing-to-spm branch 3 times, most recently from baab24e to 2ac4019 Compare September 20, 2024 21:06
Copy link
Collaborator

@dfed dfed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per my comment on #75, it'd be good to re-add unit tests to CI. Could maybe be part of this PR?

.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
Package.swift Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@luispadron
Copy link
Collaborator Author

Per my comment on #75, it'd be good to re-add unit tests to CI. Could maybe be part of this PR?

Ah my bad, yes I can look at getting tests added via the SPM workflow / pod lib lint.

@luispadron luispadron force-pushed the luis/add-stagehand-testing-to-spm branch from 2ac4019 to 8b79a7f Compare September 25, 2024 03:43
@luispadron
Copy link
Collaborator Author

@dfed see #77 as to why re-adding example CI is out-of-scope for this PR. I hope ill have time to get the examples updated but right now GitHub actions do not support the iOS versions / Xcode versions the example tests require. I'm focused on adding better support for SPM as that's what were moving to internally.

Copy link
Collaborator

@dfed dfed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change here generally makes sense to me, but getting CI's pod linting in sync with the Cocoapods version in Gemfile.lock is blocking imo.

- name: Lint StagehandTesting Podspec
run: bundle exec --gemfile=Example/Gemfile pod lib lint --verbose --fail-fast --include-podspecs=Stagehand.podspec StagehandTesting.podspec
- name: Lint podspecs
run: pod lib lint --verbose --fail-fast --include-podspecs=Stagehand.podspec StagehandTesting.podspec
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we not want bundle exec here? We should always ensure we're using the same cocoapods version to lint as the one we're going to publish with.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had removed that because the root of the repo doesnt have a Gemfile and it was referring to Example here which i was going to address build & testing in #77. I added back the bundle exec until we add a root Gemfile

swift build \
--sdk "$(xcode-select -p)/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS.sdk" \
--triple "arm64-apple-ios13.0"
# select minimum supported Xcode version
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

future-looking comment:

we have some prior art for dropping Xcode version support in minor version bumps. We also do not state the minimum required Xcode version in this project's README 😅 so there's no real contract to break. (maybe we should fix that last part, but... that also doesn't need to be today)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment is referring to the minimum Xcode version on macos-latest so we can target the earliest iOS version on those workers, but Stagehand still supports iOS 12 and that hasn't changed. Updating the Xcode version used in CI for builds/tests shouldn't effect semver afaik

@@ -1,4 +1,4 @@
// swift-tools-version:5.0.1
// swift-tools-version:5.8
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

Package.swift Show resolved Hide resolved
Comment on lines +32 to +35
.library(
name: "StagehandTesting",
targets: ["StagehandTesting"]
),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

love that this is getting exposed via SPM now!

Package.swift Outdated Show resolved Hide resolved
Package.swift Outdated Show resolved Hide resolved
@luispadron luispadron force-pushed the luis/add-stagehand-testing-to-spm branch 6 times, most recently from 5345169 to 0cf0c9d Compare September 25, 2024 15:07
@luispadron luispadron force-pushed the luis/add-stagehand-testing-to-spm branch from 0cf0c9d to 407c5cc Compare September 25, 2024 15:09
@luispadron luispadron merged commit 35b4691 into master Sep 26, 2024
3 checks passed
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.

4 participants