Skip to content

Add ability to await the discovery refresh promise#2453

Merged
robertbrignull merged 7 commits intomainfrom
robertbrignull/refresh-promise
May 31, 2023
Merged

Add ability to await the discovery refresh promise#2453
robertbrignull merged 7 commits intomainfrom
robertbrignull/refresh-promise

Conversation

@robertbrignull
Copy link
Copy Markdown
Contributor

The intent of this PR is to give you access to a promise for the Discovery class refresh. This allows us to make the tests more type safe and less prone to flakiness.

One part of this PR is making refresh itself return a promise. This means in the tests we can do await discovery.refresh() instead of having to do await (discovery as any).discover() and calling the private discover method. This means we're no longer circumventing the public interface of the Discover class.

Another part of this PR is adding a getCurrentRefreshPromise method which means even if we weren't the ones to kick off the refresh directly we can still safely wait for it to be finished. This is useful when we are testing listeners to events that trigger a refresh, such as the listener for files changing on disk. We can now do await discovery.getCurrentRefreshPromise() instead of having to do await sleep(100) and hope for the best.

The commits are split up into separate changes, if you prefer to review 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.

@robertbrignull robertbrignull requested a review from a team May 26, 2023 09:49
@robertbrignull robertbrignull requested a review from a team as a code owner May 26, 2023 09:49
Copy link
Copy Markdown
Contributor

@shati-patel shati-patel left a comment

Choose a reason for hiding this comment

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

Thanks! I've read through the commits and I think I understand the idea 😅 As far as I could tell (from local testing), the behaviour is still the same too 🎉

I'm not super confident on the actual code changes, so I wouldn't mind a second look from a more confident TypeScript-y person 👀 ❤️

Comment thread extensions/ql-vscode/src/common/discovery.ts Outdated
Comment thread extensions/ql-vscode/src/common/discovery.ts Outdated
Comment thread extensions/ql-vscode/src/common/discovery.ts Outdated
Comment thread extensions/ql-vscode/src/common/discovery.ts Outdated
Comment thread extensions/ql-vscode/src/common/discovery.ts
@robertbrignull robertbrignull force-pushed the robertbrignull/refresh-promise branch from 7cb9831 to a0e6317 Compare May 31, 2023 09:45
@robertbrignull
Copy link
Copy Markdown
Contributor Author

I think all comments are addressed now, and the tests are passing again. Should be ready for a quick re-review.

Copy link
Copy Markdown
Member

@koesie10 koesie10 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for implementing the suggestions

@robertbrignull robertbrignull merged commit d30e58b into main May 31, 2023
@robertbrignull robertbrignull deleted the robertbrignull/refresh-promise branch May 31, 2023 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants