Skip to content

Do not reregister watchers on every file change for qltests#3158

Merged
koesie10 merged 1 commit intomainfrom
koesie10/fix-qltest-discovery-watcher
Dec 21, 2023
Merged

Do not reregister watchers on every file change for qltests#3158
koesie10 merged 1 commit intomainfrom
koesie10/fix-qltest-discovery-watcher

Conversation

@koesie10
Copy link
Copy Markdown
Member

During the qltest discovery, we were recreating the watchers for qltests on every file change. This was causing the watchers to be recreated on each change, while there were no functional changes to the watchers themselves.

This commit moves the creation of the watchers to the constructor of the QLTestDiscovery class, and removes the creation of the watchers from the discover() method. The behavior should be the same.

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.

During the qltest discovery, we were recreating the watchers for qltests
on every file change. This was causing the watchers to be recreated
on each change, while there were no functional changes to the watchers
themselves.

This commit moves the creation of the watchers to the constructor of
the QLTestDiscovery class, and removes the creation of the watchers
from the discover() method. The behavior should be the same.
@koesie10 koesie10 marked this pull request as ready for review December 20, 2023 12:51
@koesie10 koesie10 requested a review from a team as a code owner December 20, 2023 12:51
Copy link
Copy Markdown
Contributor

@robertbrignull robertbrignull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also can't see any reason to recreate the watchers each time. As far as I can tell there isn't anything that could change between calls that would mean we need to remake them. The this.workspaceFolder is readonly and that's the only input.

cc @dbartol just in case he can see something / remembers something.

However, I think go ahead merging this when you're happy, and we can easily merge more changes to this area if we discover this has changed behaviour.

@koesie10 koesie10 merged commit f3780c6 into main Dec 21, 2023
@koesie10 koesie10 deleted the koesie10/fix-qltest-discovery-watcher branch December 21, 2023 08:49
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.

2 participants