Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove use of CLI server for resolving tests #3235

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

koesie10
Copy link
Member

@koesie10 koesie10 commented Jan 12, 2024

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.

This improves the performance by precomputing the set of expected files.
@robertbrignull
Copy link
Contributor

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?

@koesie10
Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants