-
Notifications
You must be signed in to change notification settings - Fork 663
Split Unity and C# tests into separate jobs #3779
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
Conversation
|
This idea LGTM. If we're refactoring them anyway, I would probably make them both jobs under the existing CI workflow. |
| runs-on: spacetimedb-new-runner | ||
| container: | ||
| image: localhost:5000/spacetimedb-ci:latest | ||
| options: >- | ||
| --privileged | ||
| --cgroupns=host | ||
| timeout-minutes: 30 | ||
| env: | ||
| CARGO_TARGET_DIR: ${{ github.workspace }}/target | ||
| steps: | ||
| - name: Checkout repository | ||
| id: checkout-stdb | ||
| uses: actions/checkout@v4 | ||
|
|
||
| # Run cheap .NET tests first. If those fail, no need to run expensive Unity tests. | ||
|
|
||
| - name: Setup dotnet | ||
| uses: actions/setup-dotnet@v3 | ||
| with: | ||
| global-json-file: global.json | ||
|
|
||
| - name: Override NuGet packages | ||
| run: | | ||
| dotnet pack crates/bindings-csharp/BSATN.Runtime | ||
| dotnet pack crates/bindings-csharp/Runtime | ||
| # Write out the nuget config file to `nuget.config`. This causes the spacetimedb-csharp-sdk repository | ||
| # to be aware of the local versions of the `bindings-csharp` packages in SpacetimeDB, and use them if | ||
| # available. Otherwise, `spacetimedb-csharp-sdk` will use the NuGet versions of the packages. | ||
| # This means that (if version numbers match) we will test the local versions of the C# packages, even | ||
| # if they're not pushed to NuGet. | ||
| # See https://learn.microsoft.com/en-us/nuget/reference/nuget-config-file for more info on the config file. | ||
| cd sdks/csharp | ||
| ./tools~/write-nuget-config.sh ../.. | ||
| - name: Run .NET tests | ||
| working-directory: sdks/csharp | ||
| run: dotnet test -warnaserror | ||
|
|
||
| - name: Verify C# formatting | ||
| working-directory: sdks/csharp | ||
| run: dotnet format --no-restore --verify-no-changes SpacetimeDB.ClientSDK.sln | ||
|
|
||
| - name: Install Rust toolchain | ||
| uses: dtolnay/rust-toolchain@stable | ||
|
|
||
| - name: Cache Rust dependencies | ||
| uses: Swatinem/rust-cache@v2 | ||
| with: | ||
| workspaces: ${{ github.workspace }} | ||
| shared-key: spacetimedb | ||
| # Let the main CI job save the cache since it builds the most things | ||
| save-if: false | ||
|
|
||
| - name: Install SpacetimeDB CLI from the local checkout | ||
| run: | | ||
| cargo install --force --path crates/cli --locked --message-format=short | ||
| cargo install --force --path crates/standalone --locked --message-format=short | ||
| # Add a handy alias using the old binary name, so that we don't have to rewrite all scripts (incl. in submodules). | ||
| ln -sf $CARGO_HOME/bin/spacetimedb-cli $CARGO_HOME/bin/spacetime | ||
| - name: Check quickstart-chat bindings are up to date | ||
| working-directory: sdks/csharp | ||
| run: | | ||
| bash tools~/gen-quickstart.sh | ||
| "${GITHUB_WORKSPACE}"/tools/check-diff.sh examples~/quickstart-chat || { | ||
| echo 'Error: quickstart-chat bindings have changed. Please run `sdks/csharp/tools~/gen-quickstart.sh`.' | ||
| exit 1 | ||
| } | ||
| - name: Check client-api bindings are up to date | ||
| working-directory: sdks/csharp | ||
| run: | | ||
| bash tools~/gen-client-api.sh | ||
| "${GITHUB_WORKSPACE}"/tools/check-diff.sh src/SpacetimeDB/ClientApi || { | ||
| echo 'Error: Client API bindings are dirty. Please run `sdks/csharp/tools~/gen-client-api.sh`.' | ||
| exit 1 | ||
| } | ||
| - name: Start SpacetimeDB | ||
| run: | | ||
| spacetime start & | ||
| disown | ||
| - name: Run regression tests | ||
| run: | | ||
| bash sdks/csharp/tools~/run-regression-tests.sh | ||
| tools/check-diff.sh sdks/csharp/examples~/regression-tests || { | ||
| echo 'Error: Bindings are dirty. Please run `sdks/csharp/tools~/gen-regression-tests.sh`.' | ||
| exit 1 | ||
| } |
Check warning
Code scanning / CodeQL
Workflow does not contain permissions Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
The best way to fix the problem is to add an explicit permissions: block under the csharp-testsuite job in the .github/workflows/ci.yml workflow. As a minimal starting point, unless the job needs write-level permissions (for example, to upload reports to Checks or modify PRs), we should grant contents: read, which is sufficient for checking out code and accessing artifacts. To mirror the precedent set by the unity-testsuite job, you may optionally add additional permissions (e.g., checks: write) only if those capabilities are required by steps in the job (which may occur in test-reporting contexts). Otherwise, just set contents: read.
-
Copy modified lines R658-R659
| @@ -655,6 +655,8 @@ | ||
| if: ${{ github.event_name != 'pull_request' || !github.event.pull_request.head.repo.fork }} | ||
|
|
||
| csharp-testsuite: | ||
| permissions: | ||
| contents: read | ||
| runs-on: spacetimedb-new-runner | ||
| container: | ||
| image: localhost:5000/spacetimedb-ci:latest |
…ain permissions Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> Signed-off-by: John Detter <4099508+jdetter@users.noreply.github.com>
…om/clockworklabs/SpacetimeDB into jdetter/split-unity-and-csharp-tests
e7281b6 to
3407ad9
Compare
bfops
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.
Assuming all this code is just moved, this LGTM
Description of Changes
This has 2 benefits:
API and ABI breaking changes
None
Expected complexity level and risk
1
Testing