Repository navigation
DYN-10984: Run engine tests (ProtoTest) in GitHub Actions - #17370
Conversation
The DesignScript engine (src/Engine/, 72K lines) has 5,274 NUnit tests in test/Engine/ProtoTest, but no GitHub Actions workflow runs them — engine regressions surface only in the downstream Jenkins UI-test chain, hours after merge. Adds run_engine_tests.yml: on PRs touching src/Engine/** or test/Engine/**, restore + build DynamoCore.sln (the same commands build_dynamo_core.yml already runs) and run dotnet test on ProtoTest with an NUnit category filter, publishing trx results as an artifact. - Defaults to the SmokeTest category (~1,200 tests) for fast PR feedback; widen via the workflow_dispatch input or by changing the default once the suite's CI runtime is known. - Verifies ProtoTest.dll exists in the shared output path before running, failing with a clear message if the build missed it. - FFITarget.dll needs no special handling: all projects share one output path (bin/AnyCPU/Release), so it lands next to ProtoTest.dll automatically (~50 test files import it at runtime). - The Linux TODO in build_dynamo_core.yml is not addressed here — engine tests are Windows-first (FFITarget interop); a Linux pass would be a follow-up. Documents the engine-test command in AGENTS.md and copilot-instructions.md. Co-Authored-By: Claude Code <[email protected]>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
See the ticket for this pull request: https://autodesk.atlassian.net/browse/DYN-10984
The maiden run failed 24 of 1185 SmokeTest tests — investigation
showed all 24 are marked Category("Failure"), the repo's convention
for known-failing tests that track engine defects (~170 in the suite;
20 of them also carry SmokeTest). They fail by design and must not
gate PRs.
The filter is now Category=SmokeTest && Category!=Failure, which
selects 1150 tests. The workflow comment documents why.
Co-Authored-By: Claude Code <[email protected]>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The new gate currently fails 24 baseline tests and does not prevent zero-test false positives.
Review effort: Balanced
Findings: 1
Open (4)
What changed in this PR
Adds Windows GitHub Actions coverage for DesignScript engine tests.
Changes:
- Runs filtered ProtoTest tests for engine-related PRs.
- Publishes TRX results and supports manual filtering.
- Documents the engine test command.
| File | Description |
|---|---|
AGENTS.md |
Documents ProtoTest execution. |
.github/workflows/run_engine_tests.yml |
Adds engine-test CI workflow. |
.github/copilot-instructions.md |
Adds matching test guidance. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| dotnet test ${{ github.workspace }}\Dynamo\test\Engine\ProtoTest\ProtoTest.csproj | ||
| --no-build -c Release | ||
| --filter "$env:TEST_CATEGORY_FILTER" |
There was a problem hiding this comment.
Addressed in d471268: the 24 failures were all Category("Failure") tests — the repo's convention for known-failing tests tracking engine defects. The default filter now excludes that category (Category=SmokeTest&Category!=Failure), and the follow-up run was green: 1,161 passed, 0 failed. Filed the ~170-test Failure-category triage as a side finding on the ticket for the test-debt backlog.
| - name: Verify test assembly built | ||
| run: | | ||
| $dll = "${{ github.workspace }}\Dynamo\bin\AnyCPU\Release\ProtoTest.dll" |
There was a problem hiding this comment.
Addressed in 1ffea17: added a Verify tests executed step that runs after dotnet test (with if: always()), parses the TRX, and fails when the file is missing or ResultSummary.Counters.executed is zero. The pre-flight DLL check remains as a fast build-sanity check, but the TRX guard is what catches the silent-pass failure mode.
The folded-scalar run step passed the filter through in a way that pwsh split on the && operator, so dotnet test received a malformed filter (Missing operand). Switch to a literal block scalar with backtick line continuations, the standard pwsh form, so the quoted filter reaches dotnet test as a single argument. Co-Authored-By: Claude Code <[email protected]>
The NUnit adapter rejected the filter with Missing operand: vstest's
filter grammar uses single & and | as operators, not &&. The repo's
own AGENTS.md documents this ("Combine with & (AND) or | (OR)").
Filter is now Category=SmokeTest&Category!=Failure.
Co-Authored-By: Claude Code <[email protected]>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
- Drop unneeded checks:write permission (nothing creates checks) - Fail the run when the TRX is missing or reports zero executed tests, so a typo'd filter or missing adapter can't silently pass the gate - Bump upload-artifact v4 -> v7 to match repo convention and the Node.js 24-compatible major Co-Authored-By: Claude Code <[email protected]>
jasonstratton
left a comment
jasonstratton
left a comment
There was a problem hiding this comment.
@QilongTang , this looks good, but a couple of points that you can decide whether to make adjustments or merge as is:
- The docs command doesn't match CI. AGENTS.md and copilot-instructions.md both say --filter "Category=SmokeTest", without &Category!=Failure. Anyone running it locally will hit those 24 known failures and assume they broke something. I'd add the exclusion in both files; they need to stay identical for the parity check.
- The smoke-only limit may not be needed. The 1,161 tests ran in 12 seconds, and almost all of the ~3.5-minute job is the build. The full ~5,100 non-Failure tests would probably add about a minute. The PR says it would widen the filter once runtime was known, and the run shows the runtime now. I'd suggest making the default the full suite minus Failure
…lter Review follow-up from Jason on #17370: - The smoke-only default is no longer needed: the 1,161 SmokeTest tests ran in 12s while the job spends ~3.5 min on the build, so the full ~5,200-test suite minus Failure adds only about a minute. Default filter is now Category!=Failure (vstest treats tests with no category as matching a negative filter, so uncategorized tests are included). - AGENTS.md and copilot-instructions.md said --filter "Category=SmokeTest" without the Failure exclusion, so anyone running the documented command locally hit the ~190 known-failing tests. Both files now show the same Category!=Failure filter CI uses (kept identical for the parity check) and note SmokeTest as the fast subset. Co-Authored-By: Claude Code <[email protected]>
|
Both points addressed in 3992493 — thanks @jasonstratton.
Since the workflow triggers on changes to itself, this push is running the widened filter on this very PR — the run will confirm the full-suite runtime and pass rate before merge. |
SonarCloud flags third-party actions referenced by a mutable tag as a high-severity supply-chain issue, which failed the PR quality gate (Security Rating on New Code: C). Pin microsoft/setup-msbuild to the commit that v3 currently resolves to. DYN-10984 Co-Authored-By: Claude Opus 5.5 <[email protected]>
Apply the same pin as run_engine_tests.yml to the existing build_dynamo_all, build_dynamo_core and dynamo_bin_diff workflows. SonarCloud only flagged the new file because the others are not new code, but they carried the same mutable-tag supply-chain exposure. DYN-10984 Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
@QilongTang FYI: I pushed two small commits to this branch to clear the SonarCloud failures. Summary below.
Why SonarCloud failedBoth SonarCloud and SonarCloud Code Analysis failed on the same single finding:
What was changed
No behavior change: the pinned SHA is exactly what Not addressed here (possible follow-up)Other third-party actions in
🤖 Generated with Claude Code |
|
Purpose
DYN-10984: Run the engine test suite in GitHub Actions. Tech-debt item #2 from the 2026-10 codebase review — the DesignScript engine (src/Engine/, 72K lines across 128 files, the highest-blast-radius code in the repo) has 5,274 NUnit tests in test/Engine/ProtoTest, but no GitHub Actions workflow runs them. Engine regressions currently surface only in the downstream Jenkins UI-test chain, hours after merge. The Linux CI workflow even carries a TODO noting dotnet test finds no tests there (build_dynamo_core.yml:98).
Key changes:
Category=SmokeTest&Category!=Failure(~1,160 of the 5,274 tests) for fast PR feedback. It is a workflow_dispatch input, so the full suite can be run on demand; the default can be widened once the suite's CI runtime is known. TheFailurecategory is the repo's convention for known-failing tests tracking engine defects — they fail by design and must not gate PRs.Review follow-ups addressed (from the first Copilot review + Sonar):
checks: writepermission — nothing in the job creates checks; the job is now least-privileged (contents: readonly).upload-artifactv4 → v7 to match repo convention and the Node.js 24-compatible major.actions/checkout@v7across workflows), so this PR follows repo convention. Happy to switch if maintainers prefer SHA pinning repo-wide.Scope notes:
Declarations
Check these if you believe they are true
Release Notes
N/A
Reviewers
@jasonstratton @QilongTang
Notes to reviewers:
FYIs
@DynamoDS/eidos
🤖 Generated with Claude Code