Repository navigation
test(sourcegen): parse snapshot fixtures as a real build does - #7189
Conversation
The source-generator snapshot harness parsed fixtures with the default parse options of Roslyn 4.7. Collection expressions in attribute arguments did not bind, so named arguments such as `Arguments = [5]` were dropped and the snapshots recorded output a real build never produces. - Parse fixtures and generated trees with LanguageVersion.Preview. - Host the generator on Roslyn 4.14, as the Roslyn414 variant runs in a current SDK. Roslyn 4.7 rejects collection expressions in attributes even in preview mode. - Fail a snapshot test when its input has syntax errors or the binding errors an old compiler reports for valid newer syntax. - Give the UnconditionalSuppressMessageAttribute stub its real constructor. - Give CrossCompilationAttributeTests their own metadata references. Under Roslyn 4.14, sharing them with parallel snapshot tests could make the derived compilation see the base through retargeting symbols, which have no syntax references. Fixes #7162
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Warning Review limit reachedYou'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 29 minutes. View limit details
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. Comment |
[Low impact] The PR appears safe to merge; no new actionable issues were found. SummaryThe snapshot harness now uses Roslyn 4.14 with preview parsing so newer fixture syntax binds correctly.
thomhurst explicitly acknowledges the existing No new actionable issues were found. Tests were not run during this review. Reviews (1) · Last reviewed commit: "test(sourcegen): parse snapshot fixtures..." · Reviewed by Greptile |
ReviewSolid test-harness fix. The root cause (default parse options plus Roslyn 4.7 silently dropping collection-expression attribute arguments) is diagnosed correctly. The Snapshot changes are all additions of Notes (non-blocking):
Overall: LGTM. |
Updated [TUnit](https://github.com/thomhurst/TUnit) from 1.75.0 to 1.76.28. <details> <summary>Release notes</summary> _Sourced from [TUnit's releases](https://github.com/thomhurst/TUnit/releases)._ ## 1.76.28 <!-- Release notes generated using configuration in .github/release.yml at v1.76.28 --> ## What's Changed ### Other Changes * Remove legacy JSON output implementation by @Saad-elnagar in thomhurst/TUnit#7234 * fix(engine): drop row-scoped data sources from failed test metadata by @kenny-ish in thomhurst/TUnit#7079 * Merge the analyzer release tables under one New Rules section (RS2007) by @bfs2021 in thomhurst/TUnit#7174 * fix: release finished test instances before session end by @thomhurst in thomhurst/TUnit#7262 * fix: never hand out a disposed shared data-source instance by @thomhurst in thomhurst/TUnit#7248 ### Dependencies * chore(deps): update all non-major dependencies to 1.76.0 by @thomhurst in thomhurst/TUnit#7233 * chore(deps): bump follow-redirects from 1.16.0 to 1.16.1 in /docs by @dependabot[bot] in thomhurst/TUnit#7232 * chore(deps): bump launch-editor from 2.14.1 to 2.14.2 in /docs by @dependabot[bot] in thomhurst/TUnit#7231 * chore(deps): update all non-major dependencies to 1.20.1 by @thomhurst in thomhurst/TUnit#7235 * chore(deps): update dependency awssdk.sqs to 4.0.100.20 by @thomhurst in thomhurst/TUnit#7236 * chore(deps): update dependency microsoft.playwright to 1.64.0 by @thomhurst in thomhurst/TUnit#7237 * chore(deps): update all non-major dependencies by @thomhurst in thomhurst/TUnit#7238 * chore(deps): update dependency path-to-regexp to v9 by @thomhurst in thomhurst/TUnit#7241 * chore(deps): update all non-major dependencies by @thomhurst in thomhurst/TUnit#7242 * chore(deps): update dependency serialize-javascript to v7.1.3 by @thomhurst in thomhurst/TUnit#7243 * chore(deps): update dependency imposter to 1.0.4 by @thomhurst in thomhurst/TUnit#7244 * chore(deps): update dependency stackexchange.redis to 3.4.20 by @thomhurst in thomhurst/TUnit#7245 * chore(deps): update dependency serialize-javascript to v7.1.4 by @thomhurst in thomhurst/TUnit#7246 * chore(deps): update dependency imposter to 1.0.5 by @thomhurst in thomhurst/TUnit#7247 * chore(deps): update all non-major dependencies to 13.6.4 by @thomhurst in thomhurst/TUnit#7251 * chore(deps): update dependency imposter to 1.0.6 by @thomhurst in thomhurst/TUnit#7258 * chore(deps): bump fast-uri from 3.1.8 to 3.1.9 in /docs by @dependabot[bot] in thomhurst/TUnit#7253 * chore(deps): bump body-parser from 1.20.6 to 1.20.9 in /docs by @dependabot[bot] in thomhurst/TUnit#7252 ## New Contributors * @Saad-elnagar made their first contribution in thomhurst/TUnit#7234 * @kenny-ish made their first contribution in thomhurst/TUnit#7079 * @bfs2021 made their first contribution in thomhurst/TUnit#7174 **Full Changelog**: thomhurst/TUnit@v1.76.0...v1.76.28 ## 1.76.0 <!-- Release notes generated using configuration in .github/release.yml at v1.76.0 --> ## What's Changed ### Other Changes * chore: add OSS scanner config, threat model and SECURITY.md by @thomhurst in thomhurst/TUnit#7080 * fix(deps): patch MessagePack for Aspire and use per-TFM Microsoft.Extensions.Http by @Quattordici in thomhurst/TUnit#7072 * fix: skip tests marked with an empty skip reason by @thomhurst in thomhurst/TUnit#7083 * fix(engine): report the engine mode actually selected in the banner by @thomhurst in thomhurst/TUnit#7085 * fix(engine): order keyed NotInParallel tests by priority, not hash code by @thomhurst in thomhurst/TUnit#7090 * fix: remove root causes of flaky tests and the broken Apple smoke jobs by @thomhurst in thomhurst/TUnit#7095 * fix(engine): run synchronous keyed NotInParallel tests concurrently by @thomhurst in thomhurst/TUnit#7087 * fix(sourcegen): run every [Arguments] row that infers the same generic type by @thomhurst in thomhurst/TUnit#7100 * fix: same filter path and display name for generic tests in both engine modes by @thomhurst in thomhurst/TUnit#7110 * fix(engine): apply max parallelism cap to keyed NotInParallel tests by @thomhurst in thomhurst/TUnit#7084 * fix(ci): install only Chromium for Playwright and wait on dpkg lock by @thomhurst in thomhurst/TUnit#6994 * fix(core): keep the engine-selected execution mode when late assemblies load by @thomhurst in thomhurst/TUnit#7091 * fix(engine): end fail-fast runs without crashing the test host by @thomhurst in thomhurst/TUnit#7115 * test: stop the AOT [EngineTest=Pass] run hitting its 5 minute budget by @thomhurst in thomhurst/TUnit#7125 * fix(engine): align reflection-mode data sources and hooks with source-gen by @thomhurst in thomhurst/TUnit#7094 * fix(pipeline): stop skipping the NuGet package tester modules by @thomhurst in thomhurst/TUnit#7127 * ci: take AOT publishes and doc snippets off the Ubuntu critical path by @thomhurst in thomhurst/TUnit#7128 * fix(analyzers): TUnit0001 false positive for MethodDataSource on generic type-parameter parameters by @thomhurst in thomhurst/TUnit#7109 * fix: report generic class constraint violations as failed tests in both modes by @thomhurst in thomhurst/TUnit#7112 * ci: cut runner contention and the AOT publish off the critical path by @thomhurst in thomhurst/TUnit#7131 * fix(assertions): make cross-type IsEqualTo work for numeric and nullable arguments on C# 12 by @thomhurst in thomhurst/TUnit#7086 * fix(engine): substitute class type arguments and array element types in build-time constraint checks by @thomhurst in thomhurst/TUnit#7102 * ci: test the NuGet packages on Windows in their own job by @thomhurst in thomhurst/TUnit#7130 * fix(sourcegen): bind params T[] on generic test methods by @thomhurst in thomhurst/TUnit#7101 * fix(engine): run every [Arguments] row on generic test classes in both modes by @thomhurst in thomhurst/TUnit#7111 * fix(engine): use last inferred type for repeated generic parameters by @theluckystrike in thomhurst/TUnit#7134 * Make the test suite pass in every environment and run fully in CI by @Quattordici in thomhurst/TUnit#7129 * fix(assertions): skip using directive for global-namespace assertion types by @thomhurst in thomhurst/TUnit#7139 * fix(core): format numeric display-name arguments with the invariant culture by @thomhurst in thomhurst/TUnit#7138 * fix(engine): dispose data-source objects of rows that end as failed tests by @thomhurst in thomhurst/TUnit#7150 * Fix net472 compile gate failure in assertions source generator tests by @thomhurst in thomhurst/TUnit#7170 * fix(sourcegen): close generic methods of generic classes on every class source by @thomhurst in thomhurst/TUnit#7152 * fix(engine): infer object for null rows on generic test methods in reflection mode by @thomhurst in thomhurst/TUnit#7142 * fix(engine): report every test once on fail-fast runs by @thomhurst in thomhurst/TUnit#7145 * test(engine): stop whole-process budgets killing healthy scheduling engine tests by @thomhurst in thomhurst/TUnit#7177 * feat(analyzers): report MethodDataSource Arguments that fit two overloads equally (TUnit0076) by @thomhurst in thomhurst/TUnit#7181 * fix(sourcegen): render top-level dynamic as object so typeof compiles by @thomhurst in thomhurst/TUnit#7182 * fix(sourcegen): pair each GenerateGenericTest type only with its typed ClassDataSource<T> by @thomhurst in thomhurst/TUnit#7188 * fix(engine): run untyped [Arguments(null)] rows on generic classes instead of dropping them by @thomhurst in thomhurst/TUnit#7203 * fix(engine): report generic test classes by their generic type definition in TRX by @thomhurst in thomhurst/TUnit#7200 * test(sourcegen): parse snapshot fixtures as a real build does by @thomhurst in thomhurst/TUnit#7189 * refactor(engine): dedupe reflection generic discovery by @thomhurst in thomhurst/TUnit#7183 * test: make TimeoutHelperTests deterministic under thread-pool load by @thomhurst in thomhurst/TUnit#7180 * fix(analyzers): accept null MethodDataSource arguments for nullable parameters by @thomhurst in thomhurst/TUnit#7179 * test(assertions): make WaitsFor cancellation tests robust to slow CI runners by @thomhurst in thomhurst/TUnit#7176 * fix(assertions): support [AssertionFrom] with unbound generic types by @thomhurst in thomhurst/TUnit#7144 ... (truncated) Commits viewable in [compare view](thomhurst/TUnit@v1.75.0...v1.76.28). </details> [](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Summary
The
TUnit.Core.SourceGenerator.Testssnapshot harness parsed fixtures fromtests/TUnit.TestProjectwith Roslyn 4.7's default parse options. Collection expressions did not bind there, so attribute arguments such asArguments = [5]were dropped. The snapshots then recorded what the generator does with broken input, not what a real build produces.[MethodDataSource(nameof(AsyncDataMethodWithArgs), Arguments = [5])]was snapshotted with noArgumentsline and a factory callingAsyncDataMethodWithArgs()with no argument (code that would not even compile).Arguments = new object[] { 5 }andAsyncDataMethodWithArgs(((int)arguments[0])), which is what a real build generates.Cause
Two things combined:
CSharpSyntaxTree.ParseText(source)used the default language version, notPreviewasTUnit.TestProjectbuilds with (CS8652).paramscollections (CS0225). Switching toPreviewalone changed no snapshot.The harness checks only generator diagnostics, so none of this was reported.
Fix
parseOptions), withLanguageVersion.Preview.VersionOverrideonMicrosoft.CodeAnalysis.Common/CSharp/Workspaces.Common). The generator is still built against 4.7. The Core generator has noROSLYN4_14conditionals, so this matches theRoslyn414variant that a current SDK loads.ThrowIfInputCannotBeParsed: a snapshot test now fails if its input has syntax errors, or if declaration binding reports CS0182/CS0225/CS8652, the errors a too-old compiler gives for valid newer syntax. Other compile errors stay allowed, because some fixtures reference packages the harness does not load (AutoFixture, Vogen, OneOf...). I checked that the guard works by temporarily removing the Roslyn override: 7 snapshot tests then fail with "The test input does not parse as a real build would".UnconditionalSuppressMessageAttributestub had no constructor, so every[UnconditionalSuppressMessage("x", "y")]in a fixture failed with CS1729. It now has the real(category, checkId)shape. No snapshot changed because of this.CrossCompilationAttributeTestsfailed deterministically after the Roslyn upgrade, but only when it ran in parallel with a snapshot test. Roslyn caches assembly symbols on each reference's metadata. With the sharedReferencesHelper.References, the derived compilation could bind different cached symbols than the base compilation did. It then saw the base project throughRetargetingAttributeData, which has noApplicationSyntaxReference, so the IDE-shape precondition failed. The class now creates fresh references per test. That passed 6 of 6 runs in the parallel combination that failed every time before.Snapshot changes, reviewed one by one
All the changes come from attribute arguments written as collection expressions, which used to be dropped.
AsyncMethodDataSourceDrivenTests.Test:Arguments = [5]is now emitted, and the factory callsAsyncDataMethodWithArgs(int)with the argument. Correct. The fixture has a singleAsyncDataMethodWithArgs(int baseValue)overload. The old snapshot called it with no arguments, which a real build cannot do.MethodDataSourceDrivenTests.TestandConflictingNamespaceTests.MethodDataSource_WithConflictingNamespace(DotNet8_0/9_0/10_0/Net4_7, same fixture): three[MethodDataSource(nameof(SomeMethod), Arguments = [...])]attributes now carry theirArguments = new object[] { 5 }/{ "Hello World!", 5, true }/{ "Hello World!", 7, true }lines. TheArgumentslines are correct. The new output also exposes an existing generator bug: the factory still callsSomeMethod(), the first overload, instead ofSomeMethod(int)/SomeMethod(string, int, bool). The sibling attributes written asnew object[] { ... }already showed this bug in the old snapshots, so it is not new. Open PR fix(sourcegen): infer generic method types from any MethodDataSource row shape #7143 fixes it ("the factory always called the first member with that name") and updates these same snapshots, so I did not file a duplicate issue. When fix(sourcegen): infer generic method types from any MethodDataSource row shape #7143 merges, these snapshots should show the overload-correct calls for the collection-expression attributes too. Whichever PR lands second needs a snapshot re-accept.No other snapshot changed on any TFM.
Tests
TUnit.Core.SourceGenerator.Testson net8.0, net9.0, net10.0 and net472: all pass after accepting the snapshots above (192 passed, 1 pre-existing skip on net8/9/10; 191 passed, 1 skip on net472)./*/*/*AttributeTests/*(the parallel combination that exposed the retargeting race) passed 6 of 6 runs.The change only affects the test harness. The generator, the engine and the public API are unchanged.
Fixes #7162
Follow-up filed: #7190 (the same parse-options pattern in the Assertions/Should/Mocks generator harnesses).