Skip to content

Refresh stale core generator snapshots - #7549

Merged
thomhurst merged 1 commit into
mainfrom
issue-7537-generator-snapshots
Nov 4, 2026
Merged

thomhurst merged 1 commit into
mainfrom
issue-7537-generator-snapshots

Conversation

@thomhurst

@thomhurst thomhurst commented Nov 4, 2026 •

Copy link
Copy Markdown
Owner

Core generator snapshot tests fail on main because two expected files still describe older generated code. Refresh five expected lines to cover the existing AOT-compatible generic property getters and typed generic constructor calls. No generator behavior changes.

This fixes the snapshot failures also blocking #7535 and #7538.

Validation: reproduced 2 failures on unchanged main, reviewed both snapshot diffs, then ran the full core generator suite: 329 passed, 1 skipped, 0 failed (SDK 10.0.403, net10.0). git diff --check passed. Existing build warnings are tracked by #7410 and #7438.

Closes #7537

Summary by CodeRabbit

  • Tests
    • Updated generated-output expectations for generic property metadata and instance creation.
    • Coverage now reflects cached, AOT-compatible getters and direct construction for the tested generic types.

@coderabbitai

coderabbitai Bot commented Nov 4, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review.

Walkthrough

The pull request updates two source-generator test snapshots. The snapshots now show generic property getters and direct typed construction for the string and int test cases.

Changes: Generated snapshot updates

Layer / File(s) Summary
Refresh generated output snapshots
tests/TUnit.Core.SourceGenerator.Tests/ClassArgumentParameterTypesTests.Test.verified.txt, tests/TUnit.Core.SourceGenerator.Tests/GenericPropertyMetadataTests.GeneratesCachedAotCompatibleGetters.verified.txt
The snapshots use CreateGenericGetter for property getters and CastHelper.Cast<T> for direct construction of the closed generic test cases.

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

Change: Other · Severity of issue fixed: Low

Possibly related PRs

  • thomhurst/TUnit#7536: This PR updates the same two verified snapshots with the same generated output.
  • thomhurst/TUnit#7524: This PR introduced the generic getter generation reflected in one of the updated snapshots.
  • thomhurst/TUnit#7496: This PR introduced typed generic instance factories reflected in the other updated snapshot.
Merge Risk: Minimal · up to 2b20a

The updated constructor snapshot preserves the intended argument. No merge-blocking risk was identified.

Pre-merge checks: Passed 5
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: refreshing stale core generator snapshots. It matches the pull request objectives and changed files.
Linked Issues check Passed Issue #7537 requires the two failing core source-generator snapshots to match the existing generated code and the core suite to pass. This PR changes only the two named .verified.txt snapshots: thre…
Out of Scope Changes check Passed The reported changes are limited to five expected lines in the two snapshots named by issue #7537. The changes support the required snapshot updates and do not add unrelated source, behavior, or admin…
Docstring Coverage Passed No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Full details: Linked Issues check

Explanation

Issue #7537 requires the two failing core source-generator snapshots to match the existing generated code and the core suite to pass. This PR changes only the two named .verified.txt snapshots: three getter lines now use PropertyMetadataFactory.CreateGenericGetter, and two generic factory lines now use direct typed construction with CastHelper.Cast&lt;T&gt;. The reported core suite result is 329 passed, 1 skipped, and 0 failed. No generator behavior change is required by the issue.

Full details: Out of Scope Changes check

Explanation

The reported changes are limited to five expected lines in the two snapshots named by issue #7537. The changes support the required snapshot updates and do not add unrelated source, behavior, or administrative changes.

Full details: Docstring Coverage

Explanation

No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 0 files. (2 skipped: 2 unsupported.)

Finishing Touches
Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • 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 snapshots at dawn
Generic getters now greet the page
Typed constructors hop into view
Old activators leave the stage
The test files match the code again

Recent review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 92ef5963-3b19-4f98-8ec6-d00a963f7967
Commits

Reviewing files that changed from the base of the PR and between 10a0af7 and 2b20adb.

Files selected for processing: 2
  • tests/TUnit.Core.SourceGenerator.Tests/ClassArgumentParameterTypesTests.Test.verified.txt
  • tests/TUnit.Core.SourceGenerator.Tests/GenericPropertyMetadataTests.GeneratesCachedAotCompatibleGetters.verified.txt

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 1 remain after this review.

@thomhurst

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Nov 4, 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-04T03:20:36.468489Z 2b20adb 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 4, 2026

Copy link
Copy Markdown
Contributor

Review

This PR only touches two .verified.txt snapshots (5 lines). I read the whole diff and found no problems.

  • The ClassArgumentParameterTypesTests changes replace o => ((dynamic)o).X with PropertyMetadataFactory.CreateGenericGetter(typeof(...<>), "X"). That fits the AOT goal in Generic test class property metadata emits dynamic getters during Native AOT publishing #7499, because dynamic is not AOT-safe.
  • The GenericPropertyMetadataTests changes replace Activator.CreateInstance with a typed new T<...>(CastHelper.Cast<...>(args[0])). This also removes reflection from the instance-creation path.
  • There are no generator source changes, so no behaviour changes. The snapshots now match what current main generates, according to the PR description, which says the two failures reproduced on main and the generator suite passes with this change. I did not rerun the tests myself.
  • No .received.txt files are in the diff.

Minor observation: in __CreateInstance_1, the old output hardcoded the 42 argument (new object[] { 42 }). The new output reads args[0], which is consistent with __CreateInstance_0. It does assume the engine always passes the constructor args in args. If that is the intended contract after #7499, this is fine.

LGTM. Merging this should unblock #7535 and #7538.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Swish!

Reviewed commit: 2b20adbffe

ℹ️ 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

Copy link
Copy Markdown
Owner Author

Followed the blocker from #7535: its previous Claude run 41759062387 passed, but the generator snapshot failures still require this PR. Reviewed all review surfaces and the five-line snapshot diff. Claude approves the current head; its constructor-argument observation is nonblocking because these snapshots record the existing generator output, and the complete generator suite already passed. There are no inline findings or unresolved threads. Codex review for 2b20adb is running (request 6621427208 is under 15 minutes old), and CodeQL run 41764999801 remains in progress. Keep draft until Codex gives a positive verdict; then promote, read the promotion Claude review, and arm auto-merge. After merge, rebase #7535 onto main for fresh checks. No rerun or duplicate review request is warranted.

@greptile-apps

greptile-apps Bot commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 5/5

[Low impact] This snapshot-only refresh appears safe to merge.

Summary

Refreshes five expected lines in two core generator snapshots. No generator or runtime code changes.

  • Core generator snapshots now match generic getter and constructor output.

Reviews (1) · Last reviewed commit: "Refresh core generator snapshots for #75..." · Reviewed by Greptile

@thomhurst
thomhurst marked this pull request as ready for review November 4, 2026 03:19
@thomhurst

thomhurst commented Nov 4, 2026 •

Copy link
Copy Markdown
Owner Author

The earlier wait on CodeQL run 41764999801 is complete (success). Rechecked every review surface: Claude and Codex approve unchanged head 2b20adb, the promotion Claude verdict is positive, and there are no inline comments or review threads. Reviewed the five-line snapshot diff; no new code or validation was needed.

The fresh Enable-AutoMerge.ps1 gate passed and squash auto-merge is now armed on this head. Waiting for ready-only .NET run 41766395044 and the other CI checks. No rerun is warranted. After this PR merges, rebase #7535 onto fresh main for checks containing the snapshot fix. This PR remains the prerequisite, so #7535 cannot merge yet; it is not superseded and requires no human decision or external dependency.

@github-actions

github-actions Bot commented Nov 4, 2026

Copy link
Copy Markdown
Contributor

Snapshot-only refresh: five lines across two .verified.txt files, no generator code changes. The updates match the described current output (PropertyMetadataFactory.CreateGenericGetter replacing the dynamic getter, and typed constructor calls via CastHelper.Cast<T>(args[0]) replacing Activator.CreateInstance). These are the AOT-friendlier forms, so the new expectations are an improvement. No .received.txt files are included, and the PR reports the full generator suite passing. LGTM.

One non-blocking observation: in __CreateInstance_1 the old snapshot hard-coded new object[] { 42 }, while the new one reads args[0]. That is correct for the runtime-supplied arguments, assuming the engine always passes args for that path, which the passing test run supports.

@thomhurst
thomhurst enabled auto-merge (squash) November 4, 2026 03:20
@thomhurst

Copy link
Copy Markdown
Owner Author

Followed the current blocker from #7541. Its earlier Claude run 41762699192 completed successfully, but all three .NET legs fail on the two stale generator snapshots tracked in #7537. This PR supplies the focused snapshot correction and already has Closes #7537.

Rechecked conversation comments, reviews, inline comments, and GraphQL review threads: Claude and Codex approve unchanged head 2b20adb, with no blocking findings or unresolved threads. The fresh Enable-AutoMerge.ps1 gate passed; squash auto-merge is armed. Remaining CI includes .NET run 41766395044, AOT run 41766394875, and platform run 41766394939. No failed checks justify a rerun or code changes. After this merges, rebase #7541 onto fresh main to include the snapshot fix and run fresh CI. This is a workflow wait, with no human or upstream gate.

@thomhurst

Copy link
Copy Markdown
Owner Author

Followed the blocker from #7546: its promotion review run 41763438515 passed, but .NET run 41763438162 fails the same two generator snapshots fixed here. Rechecked all review surfaces: Claude and Codex approve current head 2b20adb; there are no inline findings or unresolved threads. The fresh Enable-AutoMerge.ps1 gate passed and auto-merge remains armed. Remaining .NET run 41766395044 needs to finish; no failed check warrants a rerun or code changes. After merge, rebase #7546 onto main and validate for fresh CI. GitHub rejects native PR dependencies (HTTP 422: source and target must be issues), so the prerequisite is recorded here and on #7546.

This was referenced Nov 5, 2026

This branch was previously deployed

1 inactive deployment
Pull Requests — 2b20adbf Deployed Nov 4, 2026 by thomhurst via modularpipeline (windows-latest) #21097
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.

Core generator snapshots disagree with generated constructor code on main

1 participant