-
Notifications
You must be signed in to change notification settings - Fork 30
build: set up tuist #584
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: main
Are you sure you want to change the base?
build: set up tuist #584
Conversation
21b13fb
to
fcaa1df
Compare
bc07d6d
to
3b2dba0
Compare
587b880
to
6e12013
Compare
CHANGELOG.md
Outdated
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.
- Write a better CHANGELOG message.
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.
It's really nice to see Tuist replacing CocoaPods and its demo setup! 👏
I left some comments around Bundle extension tweaks that we can make, along with some Tuist updates to begin pulling in the test targets.
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.
In this EnglishDictionary.swift
file, there's a Bundle extension that we can tweak now that CocoaPods are gone - the extension can now always return the .module
bundle. I've tested this by opening the "Fuzz Testing" demo in the Demo app.
We could also make a similar change to the listableUIResources
property in Bundle+ListableUI.swift
.
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.
When the ListableTests target is pulled in by tuist, we can make this change to the Bundle extension in StableRNG.swift
too.
Demo/Project.swift
Outdated
import ProjectDescription | ||
|
||
let project = Project( | ||
name: "Demo", |
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 wonder if we should rename this to ListableDevelopment
to match naming conventions in other projects, like Workflow?
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.
definitely recommend that — make sure to get Workspace.swift
too
Demo/Project.swift
Outdated
.target( | ||
name: "DemoTests", | ||
destinations: .iOS, | ||
product: .unitTests, | ||
bundleId: "com.listable.demo.tests", | ||
deploymentTargets: .iOS("15.0"), | ||
sources: ["Test Targets/**"], | ||
dependencies: [ | ||
.target(name: "Demo") | ||
] | ||
), |
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.
This target is having trouble pulling in tests on my machine, but I was able to get tests to appear after replacing this target with the following ones. There is a bit more work for supporting the unit tests that require an app host. Sharing this as a start:
.target(
name: "ListableTests",
destinations: .iOS,
product: .unitTests,
bundleId: "com.listable.ListableTests",
deploymentTargets: .iOS("15.0"),
infoPlist: .default,
sources: ["../ListableUI/Tests/**"],
resources: ["../ListableUI/Tests/Resources/**"],
dependencies: [
.external(name: "ListableUI"),
.target(name: "EnglishDictionary"),
.target(name: "Snapshot")
]
),
.target(
name: "BlueprintUIListsTests",
destinations: .iOS,
product: .unitTests,
bundleId: "com.listable.BlueprintUIListsTests",
deploymentTargets: .iOS("15.0"),
sources: ["../BlueprintUILists/Tests/**"],
resources: [],
dependencies: [
.external(name: "BlueprintUILists"),
.external(name: "BlueprintUICommonControls"),
]
),
.target(
name: "Snapshot",
destinations: .iOS,
product: .framework,
bundleId: "com.listable.snapshot",
deploymentTargets: .iOS("15.0"),
sources: ["../Internal/Snapshot/Sources/**"],
resources: [],
dependencies: [.xctest]
),
8188cd9
to
ed87cb5
Compare
ed87cb5
to
e87a11b
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.
This is working well, nice work! There are a few final items that I wanted to iron out before merging:
- The
IdentifierChangedViewController
is missing imports toFoundation
andUIKit
. This controller was just merged intomain
. Scripts/generate_docs.sh
is having issues finding theListableUI
scheme.- There is a
Snapshot-Unit-Tests
target in Listable's main branch with a few unit tests for the internal library. We might want to pull that into Tuist as well.
e87a11b
to
698d47d
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.
Nice! Thanks for making those updates.
a29a804
to
1b93d6c
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.
I think there's some work remaining to ensure tests run and docs can still be generated, looks good overall though.
.github/workflows/run_tests.yml
Outdated
- name: Pod Install | ||
run: bundle exec pod install --repo-update | ||
- name: Test All | ||
run: tuist test --path Demo --os ${{ matrix.sdk }} -resultBundlePath TestResult |
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.
You need to specify the scheme for tuist as the last arg. It's currently just emitting a usage message and not actually running tests!
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 for catching this! I could swear I put the workspace name in there. tuist only allows one scheme in the parameters, so my options were either to run three separate commands for the different test schemes and merge the individual xcresult files or to use the workspace name instead so that it runs all the tests in the workspace.
Demo/.package.resolved
Outdated
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.
did tuist generate this one? I don't see it on other repos. Maybe see what happens if you just delete this one.
Demo/Project.swift
Outdated
import ProjectDescription | ||
|
||
let project = Project( | ||
name: "Demo", |
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.
definitely recommend that — make sure to get Workspace.swift
too
Demo/Project.swift
Outdated
destinations: .iOS, | ||
product: .app, | ||
bundleId: "com.listable.development", | ||
deploymentTargets: .iOS("15.0"), |
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.
you've probably already seen, but in the other projects I defined a bunch of extensions to abstract common things like deployment targets and bundle IDs.
Package.swift
Outdated
exclude: ["EnglishDictionary.podspec"], | ||
resources: [ | ||
.process("Resources"), | ||
] | ||
), | ||
.target( | ||
name: "Snapshot", | ||
path: "Internal Pods/Snapshot/Sources" | ||
path: "Internal/Snapshot/Sources" | ||
), | ||
.testTarget( | ||
name: "SnapshotTests", |
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.
Do these work from Package.swift? I think you may be forced to remove the test targets from Package.swift and move them to Project.swift due to the app host requirement. (Which IMO is fine and maybe better since you can move the internal targets out of here entirely.)
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 had to remove the test definitions as well since they had a dependency on the internal pods. Given that the tests are run as part of Tuist, is there any need to specify the unit tests in Package.swift
? I see them defined in Package.swift
in the Blueprint repo.
gem 'cocoapods' | ||
gem 'cocoapods-generate' | ||
|
||
gem 'jazzy' |
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.
You'll need to keep the Gemfile for jazzy, which is currently used by generate_docs.sh
.
1b93d6c
to
2c12973
Compare
Migrated local build and CI to tuist and removed CocoaPods entirely.
Checklist
Please do the following before merging:
Main
section.