-
Notifications
You must be signed in to change notification settings - Fork 176
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
Fix bug with reimporting test cases #3616
Conversation
When re-importing a test database, if the source archive for that database is not in the workspace, then old source code will be seen when inspected. To reproduce: 1. Run a ql test file with a failure (I'm using a javascript DB). 2. Right click and import on the db that sticks around. 3. Change the JS source file for the test. 4. Re-run and still have a failure. 5. Re-import the database 6. Run the query under test 7. Click on a result item 8. **BUG:** The source code is old The problem is that the source archive cache is not being flushed in this case. I added a case to flush the source archive when the archive was imported into the workspace as a folder, but not when the archive is external. The fix is to listen for database remove events in the archive filesystem provider and flush the associated database source archive. There is a complication: The database remove event didn't emit the removed database. This is because the only place that consumed the event didn't need it. To get around this, I changed the structure of the events. I added a new `fullRefresh` boolean. If true, then the original database change handler will ensure the entire tree is refreshed. If false, only the selected database.
import { DatabaseEventKind } from "../../databases/local-databases/database-events"; | ||
import type { DatabaseManager } from "../../databases/local-databases/database-manager"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to do this in order to avoid cyclic import statements.
@@ -662,18 +663,19 @@ export class DatabaseManager extends DisposableObject { | |||
} | |||
// note that we use undefined as the item in order to reset the entire tree | |||
this._onDidChangeDatabaseItem.fire({ | |||
item: undefined, | |||
item, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is actually nowhere that listens to the add
event and requires this value to be defined. Just for consistency, I thought I'd change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code changes LGTM, but it seems like the tests still need to be updated
extensions/ql-vscode/src/databases/local-databases/database-events.ts
Outdated
Show resolved
Hide resolved
extensions/ql-vscode/src/databases/local-databases/database-manager.ts
Outdated
Show resolved
Hide resolved
extensions/ql-vscode/src/common/vscode/archive-filesystem-provider.ts
Outdated
Show resolved
Hide resolved
Also: - Address comments in PR - Add changelog note
a3609d6
to
088d2fa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
extensions/ql-vscode/src/databases/local-databases/database-events.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Koen Vlaswinkel <koesie10@users.noreply.github.com>
When re-importing a test database, if the source archive for that database is not in the workspace, then old source code will be seen when inspected.
To reproduce:
The problem is that the source archive cache is not being flushed in this case. I added a case to flush the source archive when the archive was imported into the workspace as a folder, but not when the archive is external.
The fix is to listen for database remove events in the archive filesystem provider and flush the associated database source archive.
There is a complication:
The database remove event didn't emit the removed database. This is because the only place that consumed the event didn't need it.
To get around this, I changed the structure of the events. I added a new
fullRefresh
boolean. If true, then the original database change handler will ensure the entire tree is refreshed. If false, only the selected database.Replace this with a description of the changes your pull request makes.
Checklist
ready-for-doc-review
label there.