Drop the constraints that only existed to dodge #51 - #74
Open
HowardvanRooijen wants to merge 1 commit into
Open
Conversation
There was a problem hiding this comment.
Pull request overview
Removes workaround constraints in the test suite that existed solely to keep otherwise-unreferenced symbols mentioned (to avoid the pre-#51 Solve() crash), and updates one culture-thread test to capture exceptions so failures are reported cleanly instead of terminating the test host.
Changes:
- Removed incidental
.Where(t => t.X2 == 0)/ similar constraints from many tests so they now act as regression coverage for #51’s fix. - Updated the invariant-culture dedicated-thread test to catch and surface exceptions instead of allowing an unhandled thread exception to abort the run.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| solutions/Z3.Linq.Tests/UnsupportedExpressionTests.cs | Drops incidental X2 constraints so unsupported-expression translation failures are tested without workaround noise. |
| solutions/Z3.Linq.Tests/TheoremCompositionTests.cs | Removes workaround constraints so basic composition/logging tests also cover unconstrained-symbol solving. |
| solutions/Z3.Linq.Tests/SymbolTypeMarshallingTests.cs | Removes workaround constraints from round-trip tests and adds exception capture on the invariant-culture thread to avoid host termination. |
| solutions/Z3.Linq.Tests/OptimizationTests.cs | Removes workaround constraints so optimization/orderby tests cover unconstrained symbols post-#51. |
| solutions/Z3.Linq.Tests/EnvironmentTypeTests.cs | Removes an incidental nested constraint so cross-nesting solve coverage also exercises unconstrained nested symbols. |
| solutions/Z3.Linq.Tests/DistinctSelectorTheoremTests.cs | Removes workaround constraint so distinct-selector tests cover unconstrained symbols post-#51. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Test Results 1 files 1 suites 4s ⏱️ Results for commit 445aad0. ♻️ This comment has been updated with latest results. |
Thirty-two tests carried a constraint - usually .Where(t => t.X2 == 0) - whose only purpose was to keep the second symbol referenced, because an unreferenced one made Solve throw. With #51 fixed they are noise, and they were actively misleading: a reader could not tell which constraints were part of a test's subject and which were there to work around a defect. Removing them turns those tests into regression coverage for the fix. Reverting the completion flag now fails 51 tests rather than 12. Constraints whose value a test actually asserts are kept - the point stands only where the symbol was genuinely incidental. Also captures the exception on the invariant-culture thread. That solve ran on a bare Thread with no try/catch, so an exception there was unhandled and killed the test host: the run reported "zero tests ran" instead of one failure, which made the mutation check above impossible to read. Its sibling pin already did this. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
HowardvanRooijen
force-pushed
the
feature/drop-unconstrained-workarounds
branch
from
September 1, 2026 16:13
6c60d0e to
445aad0
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #73, which fixes #51.
Why
Thirty-two tests carried a constraint - usually
.Where(t => t.X2 == 0)- whose only purposewas to keep the second symbol referenced, because an unreferenced one made
Solvethrow. Theywere spread across six files and were actively misleading: a reader could not tell which
constraints belonged to a test's subject and which were there to work around a defect.
This is the cost of #51 made visible, and removing it is the last part of fixing it.
What it buys
The workarounds become regression coverage. With them gone, reverting the completion flag
fails 51 tests instead of 12 - the 32 removals expand to 39 extra cases through their
DataRows.That is a better guarantee than any test I could write deliberately: the marshalling of every
supported type, the whole optimisation surface, and most of the documented throw sites now
exercise the free-symbol path as a side effect of testing what they were already testing.
Not mechanical
Removal was per-site, not a search-and-replace. A constraint stays wherever the test asserts
the value it pins -
result.X2.ShouldBe(0)and friends - because dropping the constraint butkeeping the assertion would leave a test asserting a value on a now-free symbol, which is
exactly the flake the suite's determinism rule exists to prevent.
Kept for that reason: three tests in
DistinctSelectorTheoremTests, one inRewriterTests,two in
DistinctInlineArrayTheoremTests, the multi-symbol tests inEnvironmentTypeTests(where
X2 == 2is the subject, not a workaround), andSymbolsToString. Also kept:Solve_EverySymbolConstrained_ReturnsResult, whose entire subject is that every symbol isconstrained.
One test that could not fail properly
Solve_DoubleSymbolUnderInvariantCulture_RoundTripsTheValueran its solve on a bareThreadwith no
try/catch. Once its workaround was removed, the mutation check made that solvethrow - and because the exception was unhandled on a background thread, it took down the test
host. The run reported:
"Zero tests ran" rather than "one test failed" - which makes the mutation check above
unreadable, and would do the same to any future regression on that path. It now captures the
exception and asserts on it, exactly as its sibling pin already did.
Verification
./build.ps1 -Configuration Release- 46 tasks, 0 errors, 0 warnings