🧪 testing improvement for getQueryTimeout config - #135
Conversation
…ileSizeBytes and refactor into config.ts Co-authored-by: zknpr <96851588+zknpr@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the testability and coverage of configuration retrieval functions within the extension. By extracting Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively refactors the configuration retrieval functions getQueryTimeout and getMaximumFileSizeBytes into src/config.ts, which is a great improvement for testability. The newly added unit tests in tests/unit/config.test.ts provide good coverage for these functions.
My review includes a couple of suggestions to enhance the maintainability of the new test code. One suggestion is to refactor the test setup to reduce code duplication. Another one is to clean up the import.meta.env polyfill to reduce the usage of @ts-ignore.
Overall, this is a solid contribution that improves the project's test coverage and structure.
| describe('getQueryTimeout', () => { | ||
| test('should return default timeout (30000ms) when not configured', () => { | ||
| vscode.workspace.getConfiguration = (section) => { | ||
| assert.strictEqual(section, 'sqliteExplorer'); | ||
| return { | ||
| get: (key: string, defaultValue: any) => { | ||
| assert.strictEqual(key, 'queryTimeout'); | ||
| return defaultValue; | ||
| }, | ||
| update: () => Promise.resolve() | ||
| } as any; | ||
| }; | ||
|
|
||
| const timeout = getQueryTimeout(); | ||
| assert.strictEqual(timeout, 30000); | ||
| }); | ||
|
|
||
| test('should return configured timeout when defined', () => { | ||
| vscode.workspace.getConfiguration = (section) => { | ||
| assert.strictEqual(section, 'sqliteExplorer'); | ||
| return { | ||
| get: (key: string, defaultValue: any) => { | ||
| assert.strictEqual(key, 'queryTimeout'); | ||
| return 15000; | ||
| }, | ||
| update: () => Promise.resolve() | ||
| } as any; | ||
| }; | ||
|
|
||
| const timeout = getQueryTimeout(); | ||
| assert.strictEqual(timeout, 15000); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The test setup for getQueryTimeout is duplicated across the two test cases. To improve maintainability and reduce repetition, you can extract the mock setup into a helper function within the describe block. This pattern can then be applied to the getMaximumFileSizeBytes tests as well.
describe('getQueryTimeout', () => {
const setupMock = (returnValue: any) => {
vscode.workspace.getConfiguration = (section) => {
assert.strictEqual(section, 'sqliteExplorer');
return {
get: (key: string, defaultValue: any) => {
assert.strictEqual(key, 'queryTimeout');
if (returnValue === 'use_default') {
return defaultValue;
}
return returnValue;
},
update: () => Promise.resolve()
} as any;
};
};
test('should return default timeout (30000ms) when not configured', () => {
setupMock('use_default');
const timeout = getQueryTimeout();
assert.strictEqual(timeout, 30000);
});
test('should return configured timeout when defined', () => {
setupMock(15000);
const timeout = getQueryTimeout();
assert.strictEqual(timeout, 15000);
});
});| if (typeof process !== 'undefined') { | ||
| // @ts-ignore | ||
| globalThis.import = globalThis.import || {}; | ||
| // @ts-ignore | ||
| globalThis.import.meta = globalThis.import.meta || {}; | ||
| // @ts-ignore | ||
| globalThis.import.meta.env = globalThis.import.meta.env || { VSCODE_BROWSER_EXT: false }; | ||
| } |
There was a problem hiding this comment.
To improve code clarity and reduce the number of @ts-ignore directives, you can cast globalThis to any once and then perform the property assignments. This makes the polyfill logic a bit cleaner.
if (typeof process !== 'undefined') {
const g = globalThis as any;
g.import = g.import || {};
g.import.meta = g.import.meta || {};
g.import.meta.env = g.import.meta.env || { VSCODE_BROWSER_EXT: false };
}
Greptile SummaryThis PR improves test coverage by extracting the
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[src/config.ts] -->|exports getMaximumFileSizeBytes| B[src/workerFactory.ts]
A -->|exports getQueryTimeout| B
A -->|exports getMaximumFileSizeBytes| C[src/databaseModel.ts]
B -->|uses getMaximumFileSizeBytes| D[establishConnection]
B -->|uses getQueryTimeout| D
C -->|uses getMaximumFileSizeBytes| E[open database file]
subgraph Tests
F[tests/unit/vscode_mock_setup.ts] -->|sets up vscode mock| G[tests/unit/config.test.ts]
G -->|imports & tests| A
end
style A fill:#d4edda,stroke:#28a745
style G fill:#d4edda,stroke:#28a745
style F fill:#fff3cd,stroke:#ffc107
Last reviewed commit: 375af20 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Copilot integration | ||
| export const CopilotChatId = 'github.copilot-chat'; | ||
|
|
||
| import * as vsc from 'vscode'; |
There was a problem hiding this comment.
Import placed at the bottom of the file
The import * as vsc from 'vscode' statement is placed after all the constant exports, which is unconventional and inconsistent with standard TypeScript/JavaScript practice. Imports should always be at the top of the file before any module-level declarations. While ES module import statements are hoisted and technically work regardless of position, this layout makes the file harder to read and the dependency on vscode less obvious.
| import * as vsc from 'vscode'; | |
| import * as vsc from 'vscode'; | |
| /** | |
| * Extension Constants | |
| * |
Move the import to the very top of the file.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| // @ts-ignore | ||
| if (typeof process !== 'undefined') { | ||
| // @ts-ignore | ||
| globalThis.import = globalThis.import || {}; | ||
| // @ts-ignore | ||
| globalThis.import.meta = globalThis.import.meta || {}; | ||
| // @ts-ignore | ||
| globalThis.import.meta.env = globalThis.import.meta.env || { VSCODE_BROWSER_EXT: false }; | ||
| } |
There was a problem hiding this comment.
import.meta.env polyfill is ineffective
This polyfill attempts to simulate import.meta.env by setting globalThis.import.meta.env, but this does not actually affect how import.meta is resolved in ES module files. import.meta is a host-defined meta-property resolved statically by the module system at parse/link time — it is not a runtime property lookup on globalThis. Setting globalThis.import has no effect on module-internal import.meta references.
In practice, the current tests pass only because src/config.ts (the module under test) does not contain any import.meta.env references. If a future test were to import src/workerFactory.ts directly (which does use import.meta.env), this polyfill would not prevent it from throwing. It could mislead contributors into believing they are covered.
The PR description correctly identifies that the architectural fix (moving the config functions into config.ts) is what avoids the import.meta.env problem — the polyfill is both unnecessary and non-functional. Consider removing it or replacing it with a comment explaining why workerFactory.ts should not be imported in unit tests.
| describe('getMaximumFileSizeBytes', () => { | ||
| test('should return default size (200MB) when not configured', () => { | ||
| vscode.workspace.getConfiguration = (section) => { | ||
| assert.strictEqual(section, 'sqliteExplorer'); | ||
| return { | ||
| get: (key: string, defaultValue: any) => { | ||
| assert.strictEqual(key, 'maxFileSize'); | ||
| return undefined; // Not configured | ||
| }, | ||
| update: () => Promise.resolve() | ||
| } as any; | ||
| }; | ||
|
|
||
| const size = getMaximumFileSizeBytes(); | ||
| assert.strictEqual(size, 200 * (2 ** 20)); | ||
| }); | ||
|
|
||
| test('should return configured size in bytes', () => { | ||
| vscode.workspace.getConfiguration = (section) => { | ||
| assert.strictEqual(section, 'sqliteExplorer'); | ||
| return { | ||
| get: (key: string, defaultValue: any) => { | ||
| assert.strictEqual(key, 'maxFileSize'); | ||
| return 50; // 50MB | ||
| }, | ||
| update: () => Promise.resolve() | ||
| } as any; | ||
| }; | ||
|
|
||
| const size = getMaximumFileSizeBytes(); | ||
| assert.strictEqual(size, 50 * (2 ** 20)); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Missing edge-case test for maxFileSize: 0 (unlimited)
The docstring on getMaximumFileSizeBytes (and the callers in workerFactory.ts) explicitly document that 0 means "unlimited". The tests cover the default (undefined → 200 MB) and a positive custom value, but do not exercise the 0 case. When a user sets maxFileSize: 0, config.get<number>('maxFileSize') ?? 200 evaluates to 0 (since 0 is not nullish), so the function correctly returns 0 * (2 ** 20) === 0. Adding a test for this would protect against a future regression where someone accidentally changes ?? to ||:
test('should return 0 (unlimited) when maxFileSize is configured as 0', () => {
vscode.workspace.getConfiguration = (_section) => ({
get: (_key: string, _defaultValue: any) => 0,
update: () => Promise.resolve()
} as any);
const size = getMaximumFileSizeBytes();
assert.strictEqual(size, 0);
});
|
Implemented manually on main. Extracted |
Understood. Acknowledging that this work was implemented manually on main. Stopping work on this task. |
…d code, bump sql.js - Replace `as any` casts in HostBridge toast methods with shared `showToast` helper that properly maps DialogConfig→MessageOptions and DialogButton→MessageItem (PR #132) - Extract getMaximumFileSizeBytes and getQueryTimeout from workerFactory.ts to config.ts for testability without import.meta.env (PR #135) - Remove dead rowIds filtering code in tableExporter non-rowid branch (PR #137) - Bump sql.js from 1.13.0 to 1.14.0 (PR #112) - Add configurable mock store to vscode test mock for workspace.getConfiguration - Add unit tests for getMaximumFileSizeBytes and getQueryTimeout Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
🎯 What: The testing gap addressed
The user requested tests for
getQueryTimeout, which retrieves the timeout from vscode configuration. A similar testable function,getMaximumFileSizeBytesalso existed. The main challenge was that these functions were located insrc/workerFactory.ts, which accessesimport.meta.envat the top level, causing tests executed with tsx to fail when the module is imported.📊 Coverage: What scenarios are now tested
src/config.tsto allow isolated unit testing.getQueryTimeoutandgetMaximumFileSizeBytes.✨ Result: The improvement in test coverage
npx tsx --tsconfig tsconfig.test.json --test tests/unit/config.test.ts.PR created automatically by Jules for task 11664364936686506517 started by @zknpr