re-add preReleaseCommand

Author: JoshuaMoelans

.craft.yml

@@ -1,4 +1,5 @@
 minVersion: 2.21.4
+preReleaseCommand: pwsh ./scripts/bump-version.ps1
 changelogPolicy: auto
 artifactProvider:
   name: github

review.md

@@ -0,0 +1,61 @@
+# PR Review: `chore/desktop-integration-test`
+
+## Summary
+
+This branch migrates the CI integration test infrastructure from ad-hoc shell-based smoke tests to a structured Pester-based integration test framework using the `app-runner` module. It covers three platforms: **iOS**, **Linux**, and **Windows**. The key changes are:
+
+1. **New reusable workflows** for building Linux and Windows test apps (`smoke-test-build-linux.yml`, `smoke-test-build-windows.yml`)
+2. **New reusable workflow** for running desktop integration tests (`smoke-test-run-desktop.yml`)
+3. **Rewritten iOS test runner** (`smoke-test-run-ios.yml`) to use `app-runner` + Pester
+4. **New Pester test files**: `Integration.Tests.Desktop.ps1` and `Integration.Tests.iOS.ps1`
+5. **CI refactoring**: Linux build separated from WebGL, old inline `desktop-smoke-test` job replaced with modular build+run jobs
+6. **Test app changes**: crash type changed from `Abort` to `AccessViolation`, `CliConfiguration.cs` updated for real Sentry uploads
+7. **Dependency bumps**: sentry-cocoa 9.6.0, sentry-java 8.34.0, sentry-cli 3.3.0 (merged from main)
+
+---
+
+## Issues & Remarks
+
+### High Severity
+
+1. **`-CheckSymbols:$false` for Linux and Windows builds** — Both `smoke-test-build-linux.yml:95` and `smoke-test-build-windows.yml:84` pass `-CheckSymbols:$false` to the build step, while iOS and Android pass `-CheckSymbols` (true). The configure step also lacks `-CheckSymbols` for desktop. This means debug symbol upload is not verified for desktop builds. Is this intentional? If symbol upload should be tested, this is a gap.
+
+2. **`SENTRY_AUTH_TOKEN` in Docker for Linux build** — `ci-docker.sh` now passes `SENTRY_AUTH_TOKEN` into the Docker container. `CliConfiguration.cs` reads this at build time to decide whether to upload symbols to real Sentry. This means **every CI build (including PRs from forks)** will attempt real symbol uploads if the secret is available. The `SENTRY_AUTH_TOKEN` secret may not be available for fork PRs (GitHub Actions restricts secrets on fork PRs), which is fine — but this should be documented so contributors understand why symbol upload may silently fail for them.
+
+3. **`CliConfiguration.cs` security consideration** — The auth token is read from an environment variable at Unity Editor build time. This is fine for CI but be aware that if anyone runs this locally with `SENTRY_AUTH_TOKEN` set, it will upload symbols to the real `sentry-sdks/sentry-unity` project. Consider scoping the env var name to something more specific (e.g., `SENTRY_UNITY_TEST_AUTH_TOKEN`) to avoid accidental uploads.
+
+### Medium Severity
+
+4. **Desktop test `Platform` is "Desktop", not "Windows" or "Linux"** — In `Integration.Tests.Desktop.ps1:74`, the `TestSetup.Platform` is hardcoded to `"Desktop"`. The `CommonTestCases.ps1` app-context skip only checks for `"iOS"`. If there are platform-specific behaviors that differ between Windows and Linux (e.g., crash handler differences), the generic `"Desktop"` platform name makes it impossible to conditionally skip tests per-OS. Consider passing the actual OS platform.
+
+5. **`dist` test always skipped** — `CommonTestCases.ps1:34` skips the "Has correct dist attribute" test with the reason "dist is not yet read from AndroidManifest by sentry-java (pending submodule update)". This skip applies to *all* platforms, not just Android. Now that these tests run on iOS and Desktop too, the skip reason is misleading. Either un-skip for non-Android platforms or update the skip message.
+
+6. **No macOS desktop integration test** — The old `desktop-smoke-test` job had commented-out macOS support. The new workflows only cover Linux and Windows. macOS is only tested through the iOS simulator path. If macOS desktop testing is desired, it's still missing.
+
+7. **Crash type change from `Abort` to `AccessViolation`** — `IntegrationTester.cs` changes the crash mechanism from `ForcedCrashCategory.Abort` to `ForcedCrashCategory.AccessViolation`. This changes the nature of the crash being tested (signal-based vs. access violation). The change should be called out in the PR description with the rationale (e.g., Crashpad on Windows may not catch Abort properly).
+
+### Low Severity
+
+8. **Commit message "merge fuckup"** — This should be squashed or reworded before merging to main. The branch has a messy commit history with many WIP commits (`.`, `upsi`, `stoptheslop`, etc.) — consider squash-merging.
+
+9. **`checkout@v3` vs `checkout@v4` inconsistency** — `smoke-test-run-desktop.yml:30` uses `checkout@v4` (`actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683`) while all other workflows still use `checkout@v3` (`actions/checkout@f43a0e5ff2bd294095638e18286ca9a3d1956744`). This is fine functionally but inconsistent.
+
+10. **iOS run `max-parallel: 2`** — `ci.yml:310` adds `max-parallel: 2` to the iOS run matrix. This may significantly slow down the CI pipeline since iOS tests run across multiple version/init-type combinations. Presumably this is to avoid simulator resource contention — worth a comment explaining why.
+
+11. **`vaind/download-artifact` fork usage** — Linux and Windows build workflows use `vaind/download-artifact@e7141b6a94ef28aa3d828b52830cfa1f406a1848` (a fork with `wait-timeout`) instead of the official `actions/download-artifact`. This is already used elsewhere in the repo, but it's worth noting the dependency on a personal fork.
+
+12. **Hardcoded Linux executable name `test`** — `smoke-test-run-desktop.yml:51` sets `SENTRY_TEST_APP = "samples/IntegrationTest/Build/test"` and line 55 runs `chmod +x` on it. If the Unity build output name ever changes, this will silently fail. The Windows counterpart uses `test.exe`.
+
+13. **`results/` directory created in test scripts** — Both `Integration.Tests.Desktop.ps1:68` and `Integration.Tests.iOS.ps1:82` create `$PSScriptRoot/results/` for output. This directory is not gitignored in the test directory. Not a problem in CI, but could be messy for local runs.
+
+14. **iOS test uses `checkout@v3` without submodule checkout** — The iOS run workflow does a separate `git submodule update --init modules/app-runner` step. The desktop workflow does the same. This is fine but could be simplified with `checkout@v4` + `submodules: true` (or at least `submodules: recursive` for the specific module).
+
+15. **Duplication between `Integration.Tests.Desktop.ps1` and `Integration.Tests.iOS.ps1`** — The two files are ~90% identical (same `Invoke-TestAction` function, same test structure, same crash-send logic). The main differences are: platform name, `Connect-Device` call, and `Install-DeviceApp`. Consider extracting the shared logic into a helper or parameterized base to reduce maintenance burden.
+
+---
+
+## Overall Assessment
+
+The refactoring is solid and achieves its goal of modernizing the integration test infrastructure. The `app-runner` module provides a clean abstraction, and the Pester-based tests are well-structured with good reuse of `CommonTestCases`. The CI workflow modularization (separate build/run jobs) is a clear improvement.
+
+The main concerns are around the `Platform` naming in desktop tests (#4), the potential for unintended symbol uploads (#2, #3), and the test duplication (#15). The branch should be squash-merged to clean up the commit history.