Skip to content

test(sourcegen): fix net472 compile of AotConverterScanTests obsolete-conversion fixture - #7330

Merged
thomhurst merged 1 commit into
mainfrom
issue-7329-net472-isexternalinit
Nov 1, 2026
Merged

thomhurst merged 1 commit into
mainfrom
issue-7329-net472-isexternalinit

Conversation

@thomhurst

@thomhurst thomhurst commented Nov 1, 2026 •

Copy link
Copy Markdown
Owner

Summary

Fixes red main. AotConverterScanTests.SkipsConversionsBlockedByObsoleteError (added in #7317) compiled a fixture that declares public readonly record struct Token(string Value). A positional record needs System.Runtime.CompilerServices.IsExternalInit, and the net472 reference set doesn't include it, so the in-test compilation reported CS0518 and the test failed on net472 (main run 41311511530, modularpipeline (windows-latest)).

The fixture now declares a plain readonly struct with a constructor and a get-only property. The conversion operators and [Obsolete] attributes under test are unchanged.

Validation

  • dotnet test -c Release -f net472 --treenode-filter "/*/*/AotConverterScanTests/*" in tests/TUnit.Core.SourceGenerator.Tests: 2/2 passed
  • Same filter on net10.0: 2/2 passed

Closes #7329

Summary by CodeRabbit

  • Tests
    • Updated a test fixture to use an explicitly constructed, read-only value type.

@coderabbitai

coderabbitai Bot commented Nov 1, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Caution

Review failed

The pull request is closed.

Walkthrough

The Token fixture in SkipsConversionsBlockedByObsoleteError changes from a positional record struct to a readonly struct with an explicit constructor and read-only Value property. Conversion operators and generated-output assertions remain unchanged.

Changes: Converter scan test

Layer / File(s) Summary
Replace positional record fixture
tests/TUnit.Core.SourceGenerator.Tests/AotConverterScanTests.cs
The Token fixture uses a readonly struct with an explicit constructor and read-only Value property.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Change: Other · Severity of issue fixed: Medium

Merge Risk: Minimal · up to 65900

No actionable merge-blocking risk was identified in this test-fixture-only change.

- [ ] 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

A rabbit checks the token’s shape
A readonly struct takes its place
The constructor sets the value
Its property stays read-only, too
The operators remain in play
The output checks stay as they were

@thomhurst

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Nov 1, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-11-01T15:34:19.202601Z 6590082 Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@github-actions

github-actions Bot commented Nov 1, 2026

Copy link
Copy Markdown
Contributor

Reviewed: the change is small and correct. Replacing the positional record struct with a plain readonly struct (constructor + get-only property) removes the IsExternalInit dependency that net472 lacks, while leaving the conversion operators and [Obsolete(error: true)] attributes under test untouched. The explanatory comment in the fixture is helpful. Validation on net472 and net10.0 is documented. No issues found. LGTM.

@greptile-apps

greptile-apps Bot commented Nov 1, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 5/5

[Low impact] The fixture change appears safe to merge.

Summary

Replaces the fixture’s positional record with a plain readonly struct, avoiding its need for IsExternalInit on net472.

  • The converter test fixture compiles without record-only support.

Reviews (1) · Last reviewed commit: "test(sourcegen): avoid record struct in ..." · Reviewed by Greptile

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Another round soon, please!

Reviewed commit: 659008233a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@thomhurst
thomhurst marked this pull request as ready for review November 1, 2026 15:33
@github-actions

github-actions Bot commented Nov 1, 2026

Copy link
Copy Markdown
Contributor

Review: LGTM. The fixture now uses a plain readonly struct with a constructor and a get-only property instead of a positional record, which avoids the IsExternalInit (CS0518) failure on net472. The conversion operators and [Obsolete(error: true)] cases under test are unchanged, so the test still covers what it did before. The explanatory comment says why a record isn't used here. The PR reports 2/2 passing on net472 and net10.0. No design concerns.

@thomhurst
thomhurst enabled auto-merge (squash) November 1, 2026 15:34
@thomhurst
thomhurst disabled auto-merge November 1, 2026 15:35
@thomhurst
thomhurst merged commit 62db448 into main Nov 1, 2026
25 of 27 checks passed
@thomhurst
thomhurst deleted the issue-7329-net472-isexternalinit branch November 1, 2026 15:35
@thomhurst

Copy link
Copy Markdown
Owner Author

Red-main check: failed .NET run 41311511530 is the net472 CS0518 failure fixed by this PR in 62db448. Confirmed that commit is an ancestor of current main a828bb8. The .NET workflow is already dispatched on that head: https://github.com/thomhurst/TUnit/actions/runs/41662470681 (queued). Waiting for that run; no duplicate dispatch or rerun.

This was referenced Nov 3, 2026

This branch was previously deployed

1 inactive deployment
Pull Requests — 65900823 Deployed Nov 1, 2026 by thomhurst via modularpipeline (ubuntu-latest) #20505
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.

Red main: AotConverterScanTests.SkipsConversionsBlockedByObsoleteError fails on net472 (IsExternalInit)

1 participant