Save dirty documents before evaluating queries#2538
Conversation
dbartol
added
the
Complexity: Low
| workspaceFolderValue?: T; | ||
| } | ||
|
|
||
| const VSCODE_DEBUG_SETTING = new Setting("debug", undefined, "ql"); |
There was a problem hiding this comment.
I'm not sure how language scope works. If someone sets this in the workspace/machine/user scope, will it still be available for the language?
There was a problem hiding this comment.
To be honest, I'm not sure either. How VS Code computes the override behavior between, say, a language-independent workspace setting and a language-specific user setting is up to VS Code, but doesn't change how we implement this feature.
| return new Promise((resolve) => setTimeout(() => resolve("timeout"), ms)); | ||
| } | ||
|
|
||
| jest.setTimeout(10000); // A little extra time for the save dialog to appear. |
There was a problem hiding this comment.
I'm not sure, but I think that setting this value outside of a describe block makes it global. Inside of a describe block means it is only applicable in for those tests (I may have this mixed up with mocha, though).
There was a problem hiding this comment.
This file has been removed.
| expect(cleanDoc.isDirty).toBe(false); | ||
| }); | ||
|
|
||
| it("should not save dirty documents in other tab groups", async () => { |
There was a problem hiding this comment.
This feature is surprising to me. I would expect all docs in the workspace to be saved when I invoke the debugger. Is this the behaviour of this setting in the vscode?
There was a problem hiding this comment.
Also, is this why you can't use the saveAll API method?
There was a problem hiding this comment.
🤦 I was tricked by the name, but the VS Code source code just calls the internal equivalent of saveAll(). The actual behavior for other languages saves tabs in other groups as well. I'll fix it.
My bet is that somebody thought that saving only the current tab group was a good idea early on, so that's what the setting was named. Then, when every developer everywhere tried it and hated it, they just made it implicitly mean "save all".
There was a problem hiding this comment.
I've changed to code to just use saveAll(). The handling of untitled documents appears to be slightly different, but close enough that I won't bother emulating the VS Code behavior precisely.
The original test for saveAllInGroup() is now irrelevant, so instead I added a debugger test case to make sure the documents get saved at the right points. Since the source files are modified by the test, this required making a temporary copy of the data directory to use as the workspace, instead of opening the committed data directory directly. I had to move the test queries into the data directory that we use as the workspace folder, too, since we were pulling them directly from source.
aeisenberg
left a comment
aeisenberg
left a comment
There was a problem hiding this comment.
Nice. I'm assuming there's just a mixup in the tests. If so, 👍🏼.
|
|
||
| await editor.edit((editBuilder) => { | ||
| editBuilder.insert(new Position(0, 0), "/* another comment */"); | ||
| if (process.platform !== "win32") { |
There was a problem hiding this comment.
Is this functionality broken on windows, or is it just that the test is failing? Would you be able to add a comment to clarify this?
There was a problem hiding this comment.
Still struggling to fix a test hang that only repros on CI. Just ignore this until I get CI passing.
There was a problem hiding this comment.
In what I believe is the third 🤦 of this PR, I finally determined that the timeouts are not due to the "Save As" dialog popping up on an unexpected dirty document, but rather due to Workspace Trust, which triggers a prompt because I changed the tests to copy their workspace data to a temp folder before opening. I've disabled Workspace Trust when invoking the CLI integration tests.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
dbartol
force-pushed
the
dbartol/save-before-start
branch
from
fbbfede to
d9a1aa8
Compare
3 participants
Fixes #2390
When I refactored some of the query evaluation code, I broke the existing mechanism for saving dirty documents before evaluation. I still display the prompt to save, but then ignored the result and never saved the documents 🤦.
When using the VS Code debugger UI, there's already a setting named
debug.saveBeforeStartthat controls which documents to save before launching a debug session. When launching a query via a built-in VS Code command like "Start Debugging", VS Code saves the documents automatically. I've tried to emulate that behavior for the code paths that don't go through VS Code commands, including the non-debugger "CodeQL: Quick evaluation" and "CodeQL: Run query on selected database". I've also added that behavior to "CodeQL: Debug selection" when using an existing debug session, since it doesn't go through VS Code's debug launch code path.Depend on the user setting, VS Code will either save all dirty documents in the active tab group, including or excluding untitled documents, or it will not save anything. There's no exposed command that does this saving, so I had to emulate the behavior as best I could. I'm sure it's not exactly equivalent, but it should be close enough that nobody will notice.
I marked our existing CodeQL-specific setting for saving before evaluation as deprecated and ignored, pointing users to the core VS Code setting.
Testing the emulation was tricky, because
TextDocument's properties and methods are marked either non-configurable or readonly, so Jest can't spy on them. Instead, see the comments for how I tested using public VS Code APIs and one terrible hack. The Interwebs provided some incantations to allow the spying, but they all involved replacing methods on the prototype ofObjectso they seemed a bit scary. If someone more comfortable with JavaScript and mocking can suggest a better solution, I'm happy to change it.