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.

Sign up for GitHub

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

Jump to bottom

Fix dubious index check #692

Merged
merged 3 commits into from
Nov 30, 2020
Merged

Fix dubious index check #692

merged 3 commits into from
Nov 30, 2020

Conversation

max-schaefer
Copy link
Contributor

@max-schaefer max-schaefer commented Nov 26, 2020 •
edited by aeisenberg
Loading

I'm pretty sure this is meant to check folderIndex, not index.

Can you advise on how to add a test for this? (Of course it's fine if you want to just take over the PR instead.)

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.
  • [n/a] @github/docs-content-dsp has been cc'd in all issues for UI or other user-facing changes made by this pull request.

@max-schaefer max-schaefer mentioned this pull request Nov 26, 2020
Closed
@aeisenberg
Copy link
Contributor

Thanks for creating this PR. I don't think we have any unit tests for this part of the code. I don't mind taking this over and making one. The change looks correct.

@aeisenberg
Copy link
Contributor

This slipped my mind yesterday. I'll check today.

@aeisenberg
Copy link
Contributor

A bit more complex than I was hoping to create these unit tests, but as we go forward, it should be easier to write them since we can base new ones off of existing tests.

@aeisenberg
Add unit tests for add/remove database
0c2b35f 
In order to do this, needed to move `databases.test.ts` to the
`minimal-workspace` test folder because these tests require that there
be some kind of workspace open in order to check on workspace folders.

Unfortunately, during tests vscode does not allow you to convert from a
single root workspace to multi-root and so several of the workspace
functions needed to be stubbed out.
@aeisenberg
Copy link
Contributor

@max-schaefer thanks for getting me to create these unit tests. They uncovered a bug!

adityasharad
adityasharad approved these changes Nov 30, 2020
View reviewed changes
Copy link
Contributor

@adityasharad adityasharad left a comment

Choose a reason for hiding this comment

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

LGTM, and thanks for the detailed tests! One question about test organisation.

@aeisenberg
Copy link
Contributor

Did you have a question? I don't see one posted.

@adityasharad
Copy link
Contributor

I did write one inline, and it seems to have been lost when I submitted the review...

Some of the tests are independent of the workspace, specifically the ones for URI processing. I wonder whether those could remain as no-workspace tests where we don't have to mock the DatabaseManager or the filesystem.

@aeisenberg
Copy link
Contributor

We could do that. Though I wonder what the benefit would be. I do think it's nice to keep related tests together even if they don't all require a workspace.

@adityasharad
Copy link
Contributor

My thinking was that they are unit tests that shouldn't rely on any properties of the workspace, and I'd like them to continue being independent of the workspace.
However we cannot use the vscode API outside the VS Code integration test harness anyway, so a true separation between unit and integration tests with respect to VS Code APIs is impossible anyway. Let's stick with this.

@adityasharad adityasharad merged commit f4624f3 into main Nov 30, 2020
@adityasharad adityasharad deleted the max-schaefer-patch-1 branch November 30, 2020 19:34
@aeisenberg
Copy link
Contributor

Previously, they were part of the no-workspace tests, which still run in a vscode harness, so there's not much difference between before and now. The only difference is that the tests now run in a context where they have access to a workspace (not needed for all tests).

aofaof0907 pushed a commit to aofaof0907/vscode-codeql that referenced this pull request Jul 27, 2021
@max-schaefer @aeisenberg @aofaof0907
Fix dubious index check (github#692)
573f2cc 
* Fix dubious index check

* Add unit tests for add/remove database

In order to do this, needed to move `databases.test.ts` to the
`minimal-workspace` test folder because these tests require that there
be some kind of workspace open in order to check on workspace folders.

Unfortunately, during tests vscode does not allow you to convert from a
single root workspace to multi-root and so several of the workspace
functions needed to be stubbed out.

* Update changelog

Co-authored-by: Andrew Eisenberg <aeisenberg@github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adityasharad adityasharad adityasharad approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@max-schaefer @aeisenberg @adityasharad