Persist test tracking even if a document is closed#5883
Conversation
DanTup
added
is bug fix
in testing
|
@codex review |
|
It looks like you tried to invoke |
![chatgpt-codex-connector[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e176d6fb91
ℹ️ 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".
There was a problem hiding this comment.
Pull request overview
This pull request modifies the document position and range trackers to persist tracking when documents are closed and reopened, fixing an issue where closing and reopening a file would stop position/range updates.
Changes:
- Replaced
onDidCloseTextDocumenthandler withonDidOpenTextDocumenthandler to validate trackers when documents reopen - Added validation logic to dispose trackers if file content changed externally while closed
- Updated tests to verify tracking persists across document close/reopen cycles
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/shared/vscode/trackers.ts | Replaced document close handler with document open handler that validates and cleans up invalid trackers instead of disposing all trackers on close |
| src/test/dart/editing/position_trackers.test.ts | Changed tests from expecting undefined on close to expecting continued tracking after close/reopen, using file-based documents instead of in-memory ones |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5883 +/- ##
==========================================
+ Coverage 67.36% 67.38% +0.02%
==========================================
Files 168 168
Lines 12899 12907 +8
Branches 2556 2557 +1
==========================================
+ Hits 8689 8697 +8
Misses 3756 3756
Partials 454 454 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Otherwise closing and re-opening a file will stop it from updating. There is a chance that a file is modified while it was closed, but I don't think there's a way to handle this (since we don't get proper edits). Fixes #5878
When a document is closed, isClosed is set to true and re-opening can return a new `TextDocument` instance. Although no tests failed with the previous code, I suspect that's because VS Code only cleans up closed TextDocuments periodically and we don't have a way to force it 😔
DanTup
force-pushed
the
persist-test-trackers
branch
from
5f6c3c4 to
01bdd60
Compare
+ some extra tests
![github-code-quality[bot]](proxy.php?url=https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
|
@codex review /gemini review |
![gemini-code-assist[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses the issue of position tracking being lost when a document is closed and re-opened. By changing the trackers map to use document.uri.toString() as a key and switching from onDidCloseTextDocument to onDidOpenTextDocument, the system can now persist tracking information across document lifecycle events. The new handleDocumentOpen logic correctly identifies and invalidates trackers if a document has been externally modified in a way that makes existing offsets invalid. The added tests thoroughly cover these new behaviors, including scenarios where tracking is retained, invalidated due to external modification, or invalidated if any part of a range becomes invalid. The changes are well-implemented and improve the robustness of position tracking.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d28c350843
ℹ️ 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".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
These were effectively the same now we're clearing all positions if any becomes invalid.
When we are setting a test up, if there are no open files, or only the one we are going to open, we can skip this (rather slow) work.
Otherwise closing and re-opening a file will stop it from updating.
There is a chance that a file is modified while it was closed, but I don't think there's a way to handle this (since we don't get proper edits).
Fixes #5878