vscode-codeql icon indicating copy to clipboard operation
vscode-codeql copied to clipboard

Remove use of CLI server for resolving tests

Open koesie10 opened this issue 1 year ago • 2 comments

This PR changes the test resolver to use an implementation in the extension instead of calling out to the CLI server's resolve tests --strict-test-discovery. This results in improved performance in the extension due to less blocking operations on the CLI server. It's also more fault tolerant and is able to produce a result for the internal CodeQL repository.

I've verified that for github/codeql the results are exactly the same: all tests resolved by the CLI server are present in the list of this implementation and vice-versa. For the internal CodeQL repository, the only difference is that this version finds more results because it follows symlinks, which resolve tests doesn't do.

In the internal CodeQL repository, this implementation takes about 50 seconds to complete. codeql resolve tests takes about 4 seconds, but I'm not sure if that list is complete because it completes with an error. In github/codeql, this implementation takes 9 seconds while resolve tests takes about 3 seconds. Almost all of the time in this implementation is in the await pathExists(expectedFile) call. Removing it makes it complete in less than a second.

It's easiest to review this commit-by-commit.

Checklist

  • [ ] CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • [ ] Issues have been created for any UI or other user-facing changes made by this pull request.
  • [ ] [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

koesie10 avatar Jan 12 '24 14:01 koesie10

Almost all of the time in this implementation is in the await pathExists(expectedFile) call. Removing it makes it complete in less than a second.

Could you point to where this call is made? I'm struggling to find it. Or was this performance optimization already made?

robertbrignull avatar Jan 15 '24 16:01 robertbrignull

Could you point to where this call is made? I'm struggling to find it. Or was this performance optimization already made?

Sorry, I already removed this call as part of this commit. It was in qltest-discovery.ts before that. I worked around that slow call by discovering all .expected files beforehand, which is much faster.

koesie10 avatar Jan 16 '24 08:01 koesie10