Skip to content

test: isolate exact-allocation measurements from the test runner - #108

Open
yongchul wants to merge 1 commit into
mainfrom
test/isolate-allocation-measurements
Open

yongchul wants to merge 1 commit into
mainfrom
test/isolate-allocation-measurements

Conversation

@yongchul

@yongchul yongchul commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Thank you for submitting a pull request to the Substrait project. Please keep
your description clear and concise for reviewers.

Change Summary [REQUIRED]

test: isolate exact-allocation measurements from the test runner.

  • Run warmup and measurement on a dedicated, non-thread-pool worker without inheriting the caller's execution context.
  • Warm up and measure the same synchronous 1,000-operation workload.
  • Share the measurement harness between leaf hashing and user-defined node comparison tests.
  • Add controls for execution-context/thread isolation, a single measured allocation, and exception propagation during both warmup and measurement.
  • Document the allocation-test invariants in the contributor guide.

Motivation [REQUIRED]

The Ubuntu/.NET 10 job for #107 intermittently measured 4,968 bytes in
ComparesUserDefinedNodesWithoutAllocations(32, false) instead of zero:
https://github.com/substrait-io/substrait-csharp/actions/runs/40767797749/job/123096548876

The unchanged job passed on rerun. The original allocation source remains unconfirmed.
This change hardens the measurement boundary against runner execution-context
and thread-pool activity without changing the product implementation or weakening
the allocation contract.

Reviewer Context [OPTIONAL]

  • Preserve unkeyed [NotInParallel] for exact-allocation tests.
  • Keep the zero-byte assertion and 1,000 measured operations for each existing case and comparison mode.
  • Keep delegate/task/thread setup, task completion, and assertions outside the measured region.
  • Measure exactly once after warmup: no retries, minimum-of-many sampling, or allocation allowance.
  • A control allocates only during the measured invocation, checks that the bytes are detected, and verifies that the callback ran exactly twice (warmup plus measurement).
  • Worker exceptions propagate through the returned task; they are not swallowed or replaced with successful results.
  • Only test code and contributor guidance change. chore(deps): update .NET dependencies for Brotli security fix #107 has already merged; this is a separate follow-up.

Validation [REQUIRED]

  • dotnet format Substrait.sln --verify-no-changes --no-restore - passed.
  • dotnet build Substrait.sln --configuration Release --no-restore - passed, zero warnings/errors.
  • dotnet test --solution Substrait.sln --configuration Release --no-build - passed: 5,314 tests, zero failed/skipped.

Additional validation on macOS ARM64, SDK 10.0.403, runtimes 8.0.33 and 10.0.14:

  • dotnet restore Substrait.sln - passed.
  • dotnet test --project tests/Substrait.Tests/Substrait.Tests.csproj --configuration Release --treenode-filter '/*/*/TypeUtilsTests/*' - passed: 3,056 tests across .NET 8 and .NET 10.
  • git diff --check - passed.

Repeated focused validation:

for attempt in 1 2 3 4 5 6 7 8 9 10; do
  echo "TypeUtils isolation stress run $attempt"
  dotnet test --project tests/Substrait.Tests/Substrait.Tests.csproj --configuration Release --no-build --treenode-filter '/*/*/TypeUtilsTests/*' --no-progress || exit
done

Result: all ten runs passed, 30,560 test executions across both runtimes.
The original failure was not reproducible locally before this change either;
these results establish the harness controls and preserve coverage, not proof
that every source of runtime allocation noise has been eliminated.
The CI matrix
also passed on Ubuntu, Windows, and macOS, including the full test suite and
standalone package-consumer checks.

Public API and compatibility [REQUIRED]

None.

Breaking changes [REQUIRED]

None.

Warm and measure the same synchronous workload on dedicated workers without flowing runner execution context. Preserve strict zero-byte assertions and add controls for isolation, measured allocations, and exception propagation.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@coderabbitai

coderabbitai Bot commented Oct 28, 2026

Copy link
Copy Markdown

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 5 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 4b51801c-e228-4455-b0ad-6fc292e0a72b
Commits

Reviewing files that changed from the base of the PR and between af35d53 and 0644e22.

Files selected for processing: 2
  • CONTRIBUTING.md
  • tests/Substrait.Tests/Tools/TypeUtilsTests.cs
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant