Skip to content

🧪 testing improvement for getQueryTimeout config #135

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

Closed
zknpr wants to merge 1 commit into from
Closed

🧪 testing improvement for getQueryTimeout config #135

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions src/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,3 +39,29 @@ export const Title = 'SQLite Explorer';

// Copilot integration
export const CopilotChatId = 'github.copilot-chat';

import * as vsc from 'vscode';

@greptile-apps greptile-apps Bot Mar 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

Suggested change
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!


/**
* Retrieve maximum file size from user configuration.
*
* @returns Maximum size in bytes (0 = unlimited)
*/
export function getMaximumFileSizeBytes(): number {
const config = vsc.workspace.getConfiguration(ConfigurationSection);
const sizeMB = config.get<number>('maxFileSize') ?? 200;
return sizeMB * (2 ** 20);
}

/** Default query timeout in milliseconds (30 seconds) */
export const DEFAULT_QUERY_TIMEOUT_MS = 30000;

/**
* Retrieve query timeout from user configuration.
*
* @returns Query timeout in milliseconds
*/
export function getQueryTimeout(): number {
const config = vsc.workspace.getConfiguration(ConfigurationSection);
return config.get<number>('queryTimeout', DEFAULT_QUERY_TIMEOUT_MS);
}
4 changes: 2 additions & 2 deletions src/databaseModel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,14 @@ import type { DatabaseViewerProvider } from './editorController';

import * as vsc from 'vscode';

import { ConfigurationSection, FullExtensionId } from './config';
import { ConfigurationSection, FullExtensionId, getMaximumFileSizeBytes } from './config';
import { Disposable } from './lifecycle';
import { cancelTokenToAbortSignal, getUriParts, generateDatabaseDocumentKey } from './helpers';
import { HostBridge } from './hostBridge';
import { DatabaseConnectionBundle } from './connectionTypes';
import { DocumentRegistry } from './documentRegistry';

import { createDatabaseConnection, getMaximumFileSizeBytes } from './workerFactory';
import { createDatabaseConnection } from './workerFactory';
import { GlobalOutputChannel } from './main';

import { ModificationTracker } from './core/undo-history';
Expand Down
30 changes: 1 addition & 29 deletions src/workerFactory.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import type {

import { Worker } from './platform/threadPool';
import type { DatabaseConnectionBundle } from './connectionTypes';
import { ConfigurationSection } from './config';
import { getMaximumFileSizeBytes, getQueryTimeout } from './config';

// Native worker support (only in Node.js environment)
let nativeSupport: {
Expand All @@ -51,34 +51,6 @@ if (!import.meta.env.VSCODE_BROWSER_EXT) {
// Constants
// ============================================================================

// ============================================================================
// Configuration
// ============================================================================

/**
* Retrieve maximum file size from user configuration.
*
* @returns Maximum size in bytes (0 = unlimited)
*/
export function getMaximumFileSizeBytes(): number {
const config = vsc.workspace.getConfiguration(ConfigurationSection);
const sizeMB = config.get<number>('maxFileSize') ?? 200;
return sizeMB * (2 ** 20);
}

/** Default query timeout in milliseconds (30 seconds) */
const DEFAULT_QUERY_TIMEOUT_MS = 30000;

/**
* Retrieve query timeout from user configuration.
*
* @returns Query timeout in milliseconds
*/
export function getQueryTimeout(): number {
const config = vsc.workspace.getConfiguration(ConfigurationSection);
return config.get<number>('queryTimeout', DEFAULT_QUERY_TIMEOUT_MS);
}

// ============================================================================
// Worker Interface Types
// ============================================================================
Expand Down
85 changes: 85 additions & 0 deletions tests/unit/config.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
import './vscode_mock_setup';
import assert from 'node:assert';
import { test, describe, beforeEach, afterEach } from 'node:test';
import * as vscode from 'vscode';
import { getQueryTimeout, getMaximumFileSizeBytes } from '../../src/config';

describe('Configuration Retrievers', () => {
let originalGetConfiguration: any;

beforeEach(() => {
originalGetConfiguration = vscode.workspace.getConfiguration;
});

afterEach(() => {
vscode.workspace.getConfiguration = originalGetConfiguration;
});

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);
});
});
Comment on lines +18 to +50

@gemini-code-assist gemini-code-assist Bot Mar 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

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);
        });
    });


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));
});
});
Comment on lines +52 to +84

@greptile-apps greptile-apps Bot Mar 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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);
});

});
15 changes: 15 additions & 0 deletions tests/unit/vscode_mock_setup.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,3 +12,18 @@ Module._load = function (request, parent, isMain) {
}
return originalLoad(request, parent, isMain);
};

mockVscode.extensions = {
getExtension: () => ({ packageJSON: { version: '1.0.0' }, extensionUri: mockVscode.Uri.file('/fake/path') })
};

// Polyfill import.meta.env for tsx runner
// @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 };
}
Comment on lines +22 to +29

@gemini-code-assist gemini-code-assist Bot Mar 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

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 };
}

Comment on lines +21 to +29

@greptile-apps greptile-apps Bot Mar 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.