Skip to content

Adopt ordered multi-caret markers in FCS tests (no behaviour change) - #20082

Merged
abonie merged 5 commits into
mainfrom
tests/adopt-ordered-carets
Aug 4, 2026
Merged

Adopt ordered multi-caret markers in FCS tests (no behaviour change)#20082
abonie merged 5 commits into
mainfrom
tests/adopt-ordered-carets

Conversation

@T-Gro

@T-Gro T-Gro commented Jul 24, 2026

Copy link
Copy Markdown
Member

Follow-up to #20033, building on @auduchinok's #20077. Adopts the ordered multi-caret / selection markers (SourceContext.extractOrderedMarkedSources + {selstart}/{selend}) to remove copy-pasted test sources and magic coordinates.

No behaviour change — every migrated test keeps identical assertions (each commit rubber-duck-verified byte-for-byte; tests green on net472 + net10.0).

Reductions:

  • Goto-def tests: drop unused open System + now-redundant caret-anchor (*loc-N*) comments
  • Completion ArraySliceNotation (3 source copies → 1), CurriedArguments (5 → 1), Bug312557_2 (4 → 1), Bug69673_1 (2 → 1)
  • Symbols Nested copy-and-update (8 Facts sharing a copied source → 1)
  • BreakpointLocation: magic ((line,col),(line,col)) tuples → {selstart}/{selend} markers in the source

Deliberately not migrated (it would make the code worse, not better):

  • Tooltip/Completion tests using markAtStartOfMarker/markAtEndOfMarker on a shared source — they already reuse the source, with heterogeneous asserts per position; {caretN} there is 0 reduction and messier.
  • $ / direct-column low-level marker tests (IdentifierIsland, QuickParse, …) — $ is more compact than {caret}; migrating adds indirection + a ±1 column risk for no reduction.

T-Gro and others added 2 commits July 24, 2026 09:30
…anchor comments

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 9d20cd22-45dc-411f-9163-185fd6dd54d9
Uses SourceContext.extractOrderedMarkedSources (multi-caret) to replace whole-source
[<Theory>] copies with one marked source in DotOff.ArraySliceNotation (3 copies) and
CurriedArguments.Regression (5 copies). No behaviour change.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 9d20cd22-45dc-411f-9163-185fd6dd54d9
@github-actions

github-actions Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

⚠️ Release notes required, but author opted out

Warning

Author opted out of release notes, check is disabled for this pull request.
cc @dotnet/fsharp-team-msft

T-Gro and others added 3 commits July 24, 2026 14:59
Bug312557_2 (4 source copies -> 1 {caretN}) and Bug69673_1.CtrlSpaceForThis
(2-row Theory -> 1). No behaviour change.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 9d20cd22-45dc-411f-9163-185fd6dd54d9
…caretN}

The 8 Facts shared an identical source copied per caret; now one {caret1..8}
source + a (field-name, range) cases list. No behaviour change.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 9d20cd22-45dc-411f-9163-185fd6dd54d9
…source

Replaces magic ((line,col),(line,col)) tuples with {selstart}/{selend} markers
around the breakpoint span; the validation caret is inferred from {selend}
(SourceContext), so no {caret} needed. Compares against context.SelectedRange.
No behaviour change (all 6 tests green).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 9d20cd22-45dc-411f-9163-185fd6dd54d9
@T-Gro
T-Gro force-pushed the tests/adopt-ordered-carets branch from 54ac48a to b8c97d2 Compare July 24, 2026 13:46
@T-Gro
T-Gro marked this pull request as ready for review July 24, 2026 13:49
@T-Gro
T-Gro requested a review from a team as a code owner July 24, 2026 13:49
@T-Gro T-Gro added the NO_RELEASE_NOTES Label for pull requests which signals, that user opted-out of providing release notes label Jul 24, 2026
@T-Gro
T-Gro requested a review from abonie July 24, 2026 13:52
@github-actions github-actions Bot added AI-Tooling-Check-Bypassed Tooling check: non-fork PR, not diff-analyzed AI-Tooling-Check-Scanned-Clean Tooling check: diff analyzed, no interesting infrastructure files labels Jul 24, 2026
@auduchinok

auduchinok commented Jul 28, 2026

Copy link
Copy Markdown
Member

Deliberately not migrated (it would make the code worse, not better):
Tooltip/Completion tests using markAtStartOfMarker/markAtEndOfMarker on a shared source — they already reuse the source, with heterogeneous asserts per position; {caretN} there is 0 reduction and messier.
$ / direct-column low-level marker tests (IdentifierIsland, QuickParse, …) — $ is more compact than {caret}; migrating adds indirection + a ±1 column risk for no reduction.

Should we convert them all, so more tests could use the same infrastructure and the extra helpers could be removed?

@T-Gro T-Gro left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 This review was generated by AI (@expert-reviewer agent). Findings may contain inaccuracies — please verify independently.

Assessment: no significant issues found — this is a clean, behaviour-preserving test refactoring.

Verified the two highest-risk migrations line by line:

  • Symbols Nested copy-and-update: the 8 {caretN} markers map 1:1, in order, to the 8 original per-Fact (name, ((line,col),(line,col))) cases. The let nestedFunc source line is byte-identical, so the hardcoded ranges still match the token positions. List.iter2 over equal-length lists also guards against a miscounted marker (a length mismatch would throw and fail the test).
  • CurriedArguments.Regression (and the other collapsed Completion sources): extractOrderedMarkedSources (Checker.fs) sorts markers by Id and re-emits each source with only its own single caret, so each generated source is identical to the corresponding original InlineData single-caret source; the expected-names list preserves the original per-case expectations.

The remaining changes just drop now-redundant (*loc-N*) anchor comments and unused open System, with assertions untouched. Confirmed SourceContext.extractOrderedMarkedSources exists and behaves as assumed. No correctness/security/perf/error-handling concerns apply to this change.

@T-Gro T-Gro added the AI-reviewed PR reviewed by AI review council label Aug 4, 2026
@github-project-automation github-project-automation Bot moved this from New to In Progress in F# Compiler and Tooling Aug 4, 2026
@abonie
abonie merged commit 2038269 into main Aug 4, 2026
52 checks passed
@github-project-automation github-project-automation Bot moved this from In Progress to Done in F# Compiler and Tooling Aug 4, 2026
T-Gro pushed a commit that referenced this pull request Aug 4, 2026
Resolve conflicts by keeping the branch's dependency-only diff: reject PR #20082's
multi-caret marker test refactoring (removed from Checker.fs on this branch),
keeping the deleted FCS test files removed and Symbols.fs at the branch version.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI-reviewed PR reviewed by AI review council AI-Tooling-Check-Bypassed Tooling check: non-fork PR, not diff-analyzed AI-Tooling-Check-Scanned-Clean Tooling check: diff analyzed, no interesting infrastructure files NO_RELEASE_NOTES Label for pull requests which signals, that user opted-out of providing release notes

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants