Add integration tests for `install_swiftpm_dependencies` #109

mokagio · 2024-08-29T09:01:51Z

See the README.md in the new tests/spm_caching folder

Note

Builds on top of #105 and supersedes #108.

I was not happy with how much noise there was in the diff with the project.pbxproj files, so I decided to use code generation for them.

I chose xcodegen because we use it in at least one other (not-yet-open-source) app. Unfortunately, by the time I realized xcodegen only generates projects and not workspaces (see yonaskolb/XcodeGen#168) I had already invested enough time in the set up to not justify starting from scratch. The bulk of the noise has been removed anyway thanks to not having to track the .xcodeproj folders.

Where to go from here

These tests are just for the happy paths. We could add more simulations, such as where the command runs in an empty folder and fails.
We could also consider using Tuist which supports creating a workspace to further DRY

At the time of writing, the lint and tests tasks failed, but because of rate liminting

Reading the xcodegen for this PR made me notice a docs typo, for which I opened an easy-fix PR yonaskolb/XcodeGen#1499

I have considered if this change warrants release notes and have added them to the appropriate section in the CHANGELOG.md if necessary. – N.A.

Clearer, right?

This reverts commit 4e7c222.

bin/install_swiftpm_dependencies

mokagio · 2024-08-29T10:54:33Z

bin/install_swiftpm_dependencies

+  elif [[ ${#FOUND_ROOT_WORKSPACE_PATHS[@]} -eq 0 && ${#FOUND_ROOT_PROJECT_PATHS[@] } -eq 1 && ! -f "Package.swift" ]]; then
+    XCODEPROJ_PATH="${FOUND_ROOT_PROJECT_PATHS[0]}"
+    echo " --> Found an \`.xcodeproj\`, and no \`Package.swift\` or \`.xcworkspace\` or \`.xcodeproj\` in the root of the repo."
+    echo "     Defaulting to \`--project \"${XCODEPROJ_PATH}\`"


The tests made me noticed this path was not handled 😄

Yeah, this was "kinda intentional" (as in, I initially didn't bother covering this case to avoid adding complexity to the implementation logic and because I didn't think it was worth it given that in practice and to my knowledge we don't have any project that have a sole .xcodeproj without having a xcworkspace alongside it.

But looking at the added code, and also now that we have integration tests to ensure said implementation is correct, we might as well covering this path after all indeed 👍

AliSoftware · 2024-08-29T13:44:55Z

At the time of writing, the lint and tests tasks failed, but because of rate liminting

CI is indeed failing with:

docker: Error response from daemon: toomanyrequests: You have reached your pull rate limit. You may increase the limit by authenticating and upgrading: https://www.docker.com/increase-rate-limit

This is likely unrelated to your changes, but begs the question if there would be a way to pull this image from ECR instead of dockerhub to avoid this from happening or something. That being said, I took a look at the https://gallery.ecr.aws/ and didn't find the buildkite/plugin-linter image there.

I think it's ok for now; but if we start encountering this error more frequently, we might want to investigate more about alternate solutions (e.g. mirror that plugin in our own AWS account and ECR private registry and use that?). I'm just hoping that the mere fact that this PR adds a lot of CI jobs to the pipeline is not the cause for this, though I don't see how that would be the case, because we only invoke docker for lint and test jobs… 🤷

AliSoftware

After some back and forth in misunderstanding what we were trying to test, I think my only remaining suggestion/feedback is to cp a Package.resolved fixture within the xcodeproj/xcworkspace folders generated by xcodegen, as opposed to using xcodebuild -resolvePackageDependencies to generate those and potentially creating false positives on the tests themselves.

AliSoftware · 2024-08-29T11:42:34Z

bin/install_swiftpm_dependencies

+  elif [[ ${#FOUND_ROOT_WORKSPACE_PATHS[@]} -eq 0 && ${#FOUND_ROOT_PROJECT_PATHS[@] } -eq 1 && ! -f "Package.swift" ]]; then
+    XCODEPROJ_PATH="${FOUND_ROOT_PROJECT_PATHS[0]}"
+    echo " --> Found an \`.xcodeproj\`, and no \`Package.swift\` or \`.xcworkspace\` or \`.xcodeproj\` in the root of the repo."
+    echo "     Defaulting to \`--project \"${XCODEPROJ_PATH}\`"


Yeah, this was "kinda intentional" (as in, I initially didn't bother covering this case to avoid adding complexity to the implementation logic and because I didn't think it was worth it given that in practice and to my knowledge we don't have any project that have a sole .xcodeproj without having a xcworkspace alongside it.

But looking at the added code, and also now that we have integration tests to ensure said implementation is correct, we might as well covering this path after all indeed 👍

AliSoftware · 2024-08-29T11:43:12Z

bin/install_swiftpm_dependencies

@@ -45,13 +45,21 @@ SPM_CACHE_LOCATION="${HOME}/Library/Caches/org.swift.swiftpm"
 # Try to guess what to do if no option provided explicitly
 if [[ -z "${XCWORKSPACE_PATH}" && -z "${XCODEPROJ_PATH}" && "${USE_SPM}" != "true" ]]; then
  echo "No \`--workspace\`, \`--project\` or \`--use-spm\` flag provided. Trying to guess the correct one to use..."
+
+  shopt -s nullglob


AliSoftware · 2024-08-29T11:44:52Z

tests/install_swiftpm_dependencies/README.md

+Also note that the tests are intentionally at the integration / end-to-end level. We could structure `install_swiftpm_dependencies` so that it spits out the `xcodebuild` or `swift` it plans to run and test that, but we would lose important feedback on how `xcodebuild` and `swift` themselves behave.
+Feedback on Apple's tooling behavior is crucial to get, given their known quirks.


tests/install_swiftpm_dependencies/test_scripts/run_tests_with_xcodebuild.sh

AliSoftware · 2024-08-29T13:16:27Z

tests/install_swiftpm_dependencies/test_scripts/test_project_automatic.sh

+# Also resolve packages to generate Package.resolved in expected location
+xcodebuild -resolvePackageDependencies -project Demo.xcodeproj
+
+echo "--- :wrench: Run install_swiftpm_dependencies"
+install_swiftpm_dependencies


Doesn't it defeat the purpose of the test to run xcodebuild -resolvePackageDependencies before running install_swiftpm_dependencies? 🤔 💭

Ok I now realize that the goal is to validate that install_swiftpm+dependencies uses the rght Package.resolved from the right path based on the --flags passed to it (or lack thereof), so maybe that makes sense after all…

But still, this feels like this initial xcodebuild -resolvePackageDependencies -project Demo.xcodeproj could easily generate a false positive on the integration test.

What do you think about instead just keep a Fixture-Package.resolved file in the project/ folder, and replace this xcodebuild -resolvePackageDependencies -project Demo.xcodeproj with a cp Fixture-Package.resolved Demo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved so that we still have a Package.resolved but without invoking xcodebuild before calling the install_swiftpm_dependencies SUT and risking side effects?

Granted, that kinda violates what you mention in the README about testing Apple tools as integration tests, as we'd instead assume the path where those are generated by xcodebuild. But I think that's still a valid assumption to make and unlikely to change, and I'd be ok to do that if that allows us to avoid the risk of side effects and test false positives.

Besides, if we go the route of using cp to put the Package.resolved in place:

That will make the test more stable by using a fixture for the Package.resolved, while calling xcodebuild -resolvePackageDependencies to let Xcode resolve the dependencies and create it will potentially create a different .resolved file at different times (depending on the latest versions of the dependencies available online at the time), and thus make for harder to reproduce test failures (can't easily git bisect to try to repro failures on an old build for example)

That will allow use to also cp an InvalidPackageFixture.resolved(one that would fail compiling, could even just be a file with sole content being let package = invalid) in the Demo.xcodeproj, while copying the valid PackageFixture.resolved into the Demo.xcworkspace/project.xcodeproj/…, which would allow us to validate that install_swiftpm_dependencies would use the right one when testing it on the workspace_and_project integration test below…

AliSoftware · 2024-08-29T13:18:54Z

.buildkite/pipeline.yml

+    agents:
+      queue: default

  - label: "🔬 Test"
    command: make test
+    agents:
+      queue: default


Ooof. Nice catch.

mokagio · 2024-09-03T05:34:28Z

bin/install_swiftpm_dependencies

 echo "~~~ Cleaning up cache files before saving cache"
 rm -rf "${SPM_CACHE_LOCATION}/checkouts" "${SPM_CACHE_LOCATION}/artifacts"
+echo "Done. Removed checkouts and artifacts subfolders from $SPM_CACHE_LOCATION"


Noticed this nitpick while checking the logs in the integration tests... The group had no logs under it, which made expanding it look odd.

Before (without echo)

After

mokagio · 2024-09-03T05:43:18Z

tests/install_swiftpm_dependencies/test_scripts/test_project_no_package.sh

+  echo "+++ :x: install_swiftpm_dependencies unexpectedly succeeded without a Package.resolved in the project folder!"
+  echo "Expected: $EXPECTED"
+  echo "Got: $(cat $LOGS_PATH)"
+  exit 1


See test-the-tests PR #110

mokagio · 2024-09-03T05:43:39Z

tests/install_swiftpm_dependencies/test_scripts/test_project_no_package.sh

+    echo "+++ :x: install_swiftpm_dependencies failed, but the message it printed is not what we expected."
+    echo "Expected: $EXPECTED"
+    echo "Got: $(cat $LOGS_PATH)"
+    exit 1


Again, see #110

mokagio · 2024-09-03T05:49:46Z

Thank you @AliSoftware for the Package.resolved fixture behavior and for the time you put in researching other possible ways of testing install_swiftpm_dependencies.

I took the fixture suggestion on board. It also made me realize it would be "easy" to verify the command fails if no Package.resolved is present in the xcodeproj SwiftPM folder.

I could have kept going adding more such tests, and maybe at some point I will, but I decided to add only a token one and see if we can get this merged. 😄

AliSoftware

LGTM; only found consistency nitpicks, will apply and validate it still works then merge.

bin/install_swiftpm_dependencies

AliSoftware · 2024-09-04T17:12:15Z

tests/install_swiftpm_dependencies/test_scripts/test_project_automatic.sh

+echo "--- :computer: Copy Package.resolved fixture"
+PROJECT="Demo.xcodeproj"
+XCODE_SPM_PATH="$PROJECT/project.xcworkspace/xcshareddata/swiftpm"
+mkdir -p "$XCODE_SPM_PATH"
+cp "$TESTS_LOCATION/../package_resolved_fixtures/valid.resolved" "$XCODE_SPM_PATH/Package.resolved"


AliSoftware · 2024-09-04T17:14:00Z

tests/install_swiftpm_dependencies/test_scripts/test_project_no_package.sh

+    echo "+++ :x: install_swiftpm_dependencies failed, but the message it printed is not what we expected."
+    echo "Expected: $EXPECTED"
+    echo "Got: $(cat $LOGS_PATH)"
+    exit 1


tests/install_swiftpm_dependencies/project/Makefile

tests/install_swiftpm_dependencies/workspace_and_project/Makefile

tests/install_swiftpm_dependencies/test_scripts/test_workspace_and_project_automatic.sh

tests/install_swiftpm_dependencies/test_scripts/test_workspace_and_project_explicit.sh

tests/install_swiftpm_dependencies/test_scripts/test_workspace_automatic.sh

tests/install_swiftpm_dependencies/test_scripts/test_workspace_explicit.sh

tests/install_swiftpm_dependencies/workspace_and_project/Makefile

Seems the `cp` of the `project.yml` was intentional after all, as using `--spec` instead broke it on CI

…/scheme/project (#105) * Refactor `install_swiftpm_dependencies` to allow specifying workspace/scheme/project * Improve log wording Co-authored-by: Gio Lodi <[email protected]> * Use presence of `Package.swift` instead of `Package.resolved` when guessing which method to use * More log wording improvements Co-authored-by: Gio Lodi <[email protected]> * Some more wording tweaks * Convert all indentation to spaces * Remove the need for `--scheme` parameter + Improve parsing by now allowing conflicting options but only parsing one + Adjust some wording * shellcheck * Improve more wording * More concise Usage (only basename) * Error if `--workspace` or `--project` w/o a value * Improve error when project/workspace not found If path to `--workspace` or `--project` not found, still compute the `PACKAGE_RESOLVED_LOCATION` and let the test of file existence (`[[ ! -f "${PACKAGE_RESOLVED_LOCATION}" ]]` provide a better error message. As opposed to using `-d` (`[[ -d "${XCWORKSPACE_PATH}" ]]` and `[[ -d "${XCODEPROJ_PATH}" ]]`), which would be false when that path doesn't exist and would resultin `PACKAGE_RESOLVED_LOCATION` being unassigned… and lead to a confusing error (`unable to guess path`) as a result. * Add integration tests for `install_swiftpm_dependencies` (#109) * Add `README` for `install_swiftpm_dependencies` tests * Add `install_swiftpm_dependencies` test for standalone package * Add `install_swiftpm_dependencies` with `--use-spm` parameter * Put commands in the `$PATH` for `install_swiftpm_dependencies` tests * Rename `tests/spm_caching` to `test/install_swiftpm_dependencies` Clearer, right? * Move test instructions to script to correctly export `$PATH` * Fix inverted cmds in CI * Add test for `xcodeproj` with automatic detection * Add test for `xcodeproj` with explicit parameter * Add tests for `install_swiftpm_dependencies` with `xcworkspace` * Remove backticks from group name—They don't render * Rename `standalone_package` to `package` * Rename `xcodeproj` to `project` * Make `test_workspace_*` executable * Add tests for scenario with both workspace and project in the same dir * Fix outdated scripts names * Add `set -o pipefail` to test scripts to avoid false positives * Fix typo in test script path * Move test scripts in dedicated folder to keep tidy * Update `xcworkspace` lookup to avoid false negative * Revert "Update `xcworkspace` lookup to avoid false negative" This reverts commit 4e7c222. * Remove trailing whitespaces from `install_swiftpm_dependencies` * Use `shopt -s nullglob` to avoid false negative * Code-generate project * Add handling to auto-detect standalone project * Generate `Package.resolved` for xcodegen project * Fix path in `test_workspace_and_project_automatic.sh` * Replace hardcode `project` with code-generated one * Replace project in workspace+project setup with code-generated one * Fix code-gen Buildkite script paths * Add missing `xcodegen` in some CI tests * Fix `pushd` not working from Makefile - Bypass it * Streamline `.gitignore` in `workspace` * Use `make` in `project` for consistency * Dry Buildkite pipeline * DRY env setup for SPM tests * DRY `xcodebuild` call in tests * Fix unbound TESTS_LOCATION * Fix wrong path for `xcodebuild` * Remove misplaced `USE_SPM` set * Use a fixture `Package.resolved` * Add test for `xcodeproj` without `Package.resolved` * Make a `~~~` group print a confirmation to avoid empty logs * Refine error logging in `test_project_no_package.sh` * Apply suggestions from code review * Revert change for `workspace_and_project/Makefile` Seems the `cp` of the `project.yml` was intentional after all, as using `--spec` instead broke it on CI --------- Co-authored-by: Olivier Halligon <[email protected]> --------- Co-authored-by: Gio Lodi <[email protected]>

mokagio added 23 commits August 9, 2024 15:03

Add README for install_swiftpm_dependencies tests

d2454f0

Add install_swiftpm_dependencies test for standalone package

f9e5484

Add install_swiftpm_dependencies with --use-spm parameter

6ef5c56

Put commands in the $PATH for install_swiftpm_dependencies tests

013598a

Rename tests/spm_caching to test/install_swiftpm_dependencies

19529b3

Clearer, right?

Move test instructions to script to correctly export $PATH

554cf33

Fix inverted cmds in CI

90ba0b5

Add test for xcodeproj with automatic detection

4404044

Add test for xcodeproj with explicit parameter

b205724

Add tests for install_swiftpm_dependencies with xcworkspace

85dcd90

Remove backticks from group name—They don't render

85dac57

Rename standalone_package to package

ddb2ab5

Rename xcodeproj to project

e95ccb0

Make test_workspace_* executable

7c942cd

Add tests for scenario with both workspace and project in the same dir

4dac87b

Fix outdated scripts names

0304f6a

Add set -o pipefail to test scripts to avoid false positives

7b124c4

Fix typo in test script path

4c60f49

Move test scripts in dedicated folder to keep tidy

a1d2f84

Update xcworkspace lookup to avoid false negative

4e7c222

Revert "Update xcworkspace lookup to avoid false negative"

2a4899f

This reverts commit 4e7c222.

Remove trailing whitespaces from install_swiftpm_dependencies

ce6e396

Use shopt -s nullglob to avoid false negative

b42adf0

mokagio force-pushed the mokagio/install_swiftpm_dependencies-tests-xcodegen branch from 2c65323 to 35f23ef Compare August 29, 2024 09:06

mokagio added 2 commits August 29, 2024 19:25

Code-generate project

80cc173

Add handling to auto-detect standalone project

ec2b5c1

mokagio force-pushed the mokagio/install_swiftpm_dependencies-tests-xcodegen branch from 80a08d1 to ec2b5c1 Compare August 29, 2024 09:26

mokagio changed the base branch from trunk to install_swiftpm_dependencies-improvements August 29, 2024 09:26

mokagio added 2 commits August 29, 2024 19:31

Generate Package.resolved for xcodegen project

a215f6c

Fix path in test_workspace_and_project_automatic.sh

a36ed9f

mokagio commented Aug 29, 2024

View reviewed changes

bin/install_swiftpm_dependencies Outdated Show resolved Hide resolved

mokagio and others added 2 commits August 29, 2024 20:47

Fix wrong path for xcodebuild

eb12b6a

Remove misplaced USE_SPM set

a2c39ce

mokagio force-pushed the mokagio/install_swiftpm_dependencies-tests-xcodegen branch from bee5300 to a2c39ce Compare August 29, 2024 10:47

mokagio marked this pull request as ready for review August 29, 2024 10:51

mokagio requested a review from AliSoftware August 29, 2024 10:53

mokagio commented Aug 29, 2024

View reviewed changes

AliSoftware reviewed Aug 29, 2024

View reviewed changes

mokagio force-pushed the mokagio/install_swiftpm_dependencies-tests-xcodegen branch from 2552c10 to 32e2f61 Compare September 3, 2024 04:48

Use a fixture Package.resolved

0bd7b11

mokagio force-pushed the mokagio/install_swiftpm_dependencies-tests-xcodegen branch from 32e2f61 to 0bd7b11 Compare September 3, 2024 04:54

Add test for xcodeproj without Package.resolved

125318e

mokagio force-pushed the mokagio/install_swiftpm_dependencies-tests-xcodegen branch from 81dca21 to 125318e Compare September 3, 2024 05:11

Make a ~~~ group print a confirmation to avoid empty logs

8ea9a2f

mokagio commented Sep 3, 2024

View reviewed changes

Refine error logging in test_project_no_package.sh

d4fda04

mokagio commented Sep 3, 2024

View reviewed changes

mokagio mentioned this pull request Sep 3, 2024

Verify spm integration behavior #110

Closed

mokagio requested a review from AliSoftware September 3, 2024 05:49

AliSoftware reviewed Sep 4, 2024

View reviewed changes

Apply suggestions from code review

9ffb391

AliSoftware reviewed Sep 4, 2024

View reviewed changes

tests/install_swiftpm_dependencies/workspace_and_project/Makefile Outdated Show resolved Hide resolved

Revert change for workspace_and_project/Makefile

52018fd

Seems the `cp` of the `project.yml` was intentional after all, as using `--spec` instead broke it on CI

AliSoftware merged commit 005fbb4 into install_swiftpm_dependencies-improvements Sep 4, 2024
13 checks passed

AliSoftware deleted the mokagio/install_swiftpm_dependencies-tests-xcodegen branch September 4, 2024 17:33

AliSoftware mentioned this pull request Sep 4, 2024

Refactor install_swiftpm_dependencies to allow specifying workspace/scheme/project #105

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add integration tests for `install_swiftpm_dependencies` #109

Add integration tests for `install_swiftpm_dependencies` #109

mokagio commented Aug 29, 2024 •

edited

Loading

mokagio Aug 29, 2024

AliSoftware Aug 29, 2024

AliSoftware commented Aug 29, 2024 •

edited

Loading

AliSoftware left a comment

AliSoftware Aug 29, 2024

AliSoftware Aug 29, 2024

AliSoftware Aug 29, 2024

AliSoftware Aug 29, 2024

AliSoftware Aug 29, 2024

AliSoftware Aug 29, 2024

mokagio Sep 3, 2024 •

edited

Loading

mokagio Sep 3, 2024

mokagio Sep 3, 2024

AliSoftware Sep 4, 2024

mokagio commented Sep 3, 2024

AliSoftware left a comment

AliSoftware Sep 4, 2024

AliSoftware Sep 4, 2024

		Also note that the tests are intentionally at the integration / end-to-end level. We could structure `install_swiftpm_dependencies` so that it spits out the `xcodebuild` or `swift` it plans to run and test that, but we would lose important feedback on how `xcodebuild` and `swift` themselves behave.
		Feedback on Apple's tooling behavior is crucial to get, given their known quirks.

Add integration tests for install_swiftpm_dependencies #109

Add integration tests for install_swiftpm_dependencies #109

Conversation

mokagio commented Aug 29, 2024 • edited Loading

Where to go from here

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AliSoftware commented Aug 29, 2024 • edited Loading

AliSoftware left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mokagio Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mokagio commented Sep 3, 2024

AliSoftware left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add integration tests for `install_swiftpm_dependencies` #109

Add integration tests for `install_swiftpm_dependencies` #109

mokagio commented Aug 29, 2024 •

edited

Loading

AliSoftware commented Aug 29, 2024 •

edited

Loading

mokagio Sep 3, 2024 •

edited

Loading