vscode icon indicating copy to clipboard operation
vscode copied to clipboard

Test gutter icons/locations do not update correctly when extension replaces children

Open DanTup opened this issue 3 years ago • 4 comments

This came up in the Dart extension, but I can reproduce in test-provider-example.

  • Clone the vscode-extension-samples project and open test-provider-sample
  • Comment out the line data.updateFromContents(ctrl, e.getText(), file); in updateNodeForDocument (this means tests will only update when saving the file, so we have control over it for test purposes)
  • Run the extension and open a markdown file
  • Confirm that makes changes to the tests only update the Test Explorer when saving the file
  • Cut all content from the file to the clipboard
  • Note that VS Code moves the gutter icons all to the first line, since the original locations are no longer valid
  • Now paste the contents back into the file
  • The gutter icons are still on line 1 - this is expected, because the extension has not provided VS Code any updated locations yet
  • Press Save to force the extension to re-parse the file and provide the tests back to VS Code
  • Note that gutter icons do not move, the remain on line 1

Although these actions seem contrived, I think it could be what's been raised at https://github.com/Dart-Code/Dart-Code/issues/4163. I think what's happening is that VS code might be seeing that the tests given to it are "the same as last time" and assuming no updates are required - however, since VS Code may have moved the icons when the document was modified, if it is doing that, I don't think it's a valid optimisation.

@connor4312 I'm not certain about this, but I did find some code in testItemCollection called diffTestItems that perhaps might be doing something like this?

DanTup avatar Sep 20 '22 14:09 DanTup

Interestingly, "Go to Test" in the explorer seems to jump to the "bad" location too. I guess it somehow gets its location from the decorations (so it can also compensate for edits), and is affected by the same issue?

DanTup avatar Sep 20 '22 14:09 DanTup

Yea, your assumption is right. Even when replacing all items, we do a diff and only send updates if there's a change. So when you remove, add, and save identical items, nothing at all will be updated. The downside to fixing this such that a sync happens even without a range update, is that we don't know if the test controller updated the tests as a result of re-discovering their locations, or something unrelated. But I suppose it should be okay to assume that any update will have involved discovery, since if an extension--for example--reads a label update it will presumably have also updated the location if necessary.

connor4312 avatar Sep 20 '22 22:09 connor4312

we don't know if the test controller updated the tests as a result of re-discovering their locations, or something unrelated

Could you track which fields were set and just update them?

I don't think it's unreasonable to assume if an extension is calling replace() that it's building from current information though (or at least, that it's eventually consistent.. if a user is typing at the time, another update comes later, for example).

That said, can't the diff just use the actual location VS Code has instead of the last one provided by the extension? It seems like VS Code knows where it's moved to, because the "Go to Test" location seems to jump to the decoration (and not the location provided by the extension)?

DanTup avatar Sep 21 '22 06:09 DanTup

That said, can't the diff just use the actual location VS Code has instead of the last one provided by the extension? It seems like VS Code knows where it's moved to, because the "Go to Test" location seems to jump to the decoration (and not the location provided by the extension)?

The diff happens in the extension host, while decorations are tracked in the renderer, so that would involve a lot of chatter.

I have a solution I'm fairly happy with, I'll get it into the next Insiders. Let me know if it works any better for you...

connor4312 avatar Sep 21 '22 19:09 connor4312

This bug has been fixed in the latest release of VS Code Insiders!

@DanTup, you can help us out by commenting /verified if things are now working as expected.

If things still don't seem right, please ensure you're on version ba515cdbb48470c116884deb6de17981d341ec06 of Insiders (today's or later - you can use Help: About in the command palette to check), and leave a comment letting us know what isn't working as expected.

Happy Coding!

vscodenpa avatar Sep 22 '22 08:09 vscodenpa

@connor4312 works great in insiders, thank you! :-)

DanTup avatar Sep 22 '22 11:09 DanTup

oops, forgot

/verified

DanTup avatar Sep 22 '22 11:09 DanTup