Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
1 change: 1 addition & 0 deletions .github/workflows/actions/test-and-build/action.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ runs:
- name: Run Tests
env:
NODE_OPTIONS: "--max_old_space_size=4096"
MDB_UNDER_TEST: "true"
run: |
npm run test
shell: bash
Expand Down
1 change: 1 addition & 0 deletions .github/workflows/draft-release.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ jobs:
GARASIGN_USERNAME: ${{ secrets.GARASIGN_USERNAME }}
SNYK_TOKEN: ${{ secrets.SNYK_TOKEN }}
JIRA_API_TOKEN: ${{ secrets.JIRA_API_TOKEN }}
MDB_UNDER_TEST: "true"

- name: Create Draft Release
run: |
Expand Down
1 change: 1 addition & 0 deletions .github/workflows/test-and-build-from-fork.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -40,4 +40,5 @@ jobs:
env:
NODE_OPTIONS: "--max_old_space_size=4096"
SEGMENT_KEY: "test-segment-key"
MDB_UNDER_TEST: "true"
run: npm run test
1 change: 1 addition & 0 deletions .github/workflows/test-and-build.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -48,3 +48,4 @@ jobs:
GARASIGN_USERNAME: ${{ secrets.GARASIGN_USERNAME }}
SNYK_TOKEN: ${{ secrets.SNYK_TOKEN }}
JIRA_API_TOKEN: ${{ secrets.JIRA_API_TOKEN }}
MDB_UNDER_TEST: "true"
5 changes: 3 additions & 2 deletions .vscode/launch.json
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,11 @@
"--extensionTestsPath=${workspaceFolder}/out/test/suite"
],
"env": {
"MOCHA_GREP": "${input:mochaGrep}"
"MOCHA_GREP": "${input:mochaGrep}",
"MDB_UNDER_TEST": "true"
},
"outFiles": ["${workspaceFolder}/out/**/*.js"],
"preLaunchTask": "npm: compile:extension",
"preLaunchTask": "npm: compile:extension"
}
],
"inputs": [
Expand Down
1 change: 1 addition & 0 deletions src/connectionController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -729,6 +729,7 @@ export default class ConnectionController {

if (!this._activeDataService) {
log.error('Unable to disconnect: no active connection');
this._disconnecting = false;
return false;
}

Expand Down
25 changes: 18 additions & 7 deletions src/extension.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,9 @@ debug.log = log.debug.bind(log);

import MDBExtensionController from './mdbExtensionController';

let mdbExtension: MDBExtensionController;
let mdbExtension: MDBExtensionController | undefined;

const isUnderTest = process.env.MDB_UNDER_TEST === 'true';
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const isUnderTest = process.env.MDB_UNDER_TEST === 'true';
const isUnderTest = process.env.NODE_ENV === "test" && process.env.MDB_UNDER_TEST === 'true';

nit and optional: Perhaps to make it more clear that this is a test only setting. Doing this would require setting the correct flag when running tests as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe let's get rid of MDB_UNDER_TEST altogether and just use the NODE_ENV?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could be problematic
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer being explicit about this if you're not married to the idea of using NODE_ENV @gagik

Copy link
Contributor

Choose a reason for hiding this comment

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

okay NODE_DEV in particular actually antipattern (TIL), although not necessarily relevant for our case: https://nodejs.org/en/learn/getting-started/nodejs-the-difference-between-development-and-production

"under test" is weird wording for me though, maybe just MDB_IS_TEST?

Copy link
Contributor

Choose a reason for hiding this comment

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

Would that create problems for us when/if we migrate to using vitest, considering it doesn't automatically set NODE_ENV? Or do we just set it ourselves?

Copy link
Contributor

Choose a reason for hiding this comment

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

we'd set it just like any other environment variable yeah

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the current setup, it actually doesn't set the NODE_ENV to "test" automatically, just FYI

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah it's just a matter of naming preferences. The real difference here is that in production environment this is pre-set to production so the variable is less random.
NODE_ENV === 'test' or MDB_IS_TEST is fine by me; but it should be one or the other imo

Copy link
Contributor Author

@tculig tculig Nov 19, 2025

Choose a reason for hiding this comment

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

So, as it turns out, NODE_ENV is being set to "development" by webpack. It overrides any value that is being passed as a runtime parameter. Thus, it is somewhat unclear when this variable will get hijacked by some other process, and is definitely not 100% under our full control. I would argue for keeping MDB_IS_TEST (renamed from MDB_UNDER_TEST), for consistency and full control @gagik


// Called when our extension is activated.
// See "activationEvents" in `package.json` for the events that cause activation.
Expand Down Expand Up @@ -51,13 +53,22 @@ export async function activate(
},
});

mdbExtension = new MDBExtensionController(context, {
shouldTrackTelemetry: true,
});
await mdbExtension.activate();
if (isUnderTest) {
log.info(
'Skipping MDBExtensionController activation because MDB_UNDER_TEST is set.',
);
return;
}

// Add our extension to a list of disposables for when we are deactivated.
context.subscriptions.push(mdbExtension);
if (!mdbExtension) {
mdbExtension = new MDBExtensionController(context, {
shouldTrackTelemetry: true,
});
await mdbExtension.activate();

// Add our extension to a list of disposables for when we are deactivated.
context.subscriptions.push(mdbExtension);
}
}

// Called when our extension is deactivated.
Expand Down
2 changes: 1 addition & 1 deletion src/test/runTest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ async function main(): Promise<any> {

// Download VS Code, unzip it and run the integration test
await runTests({
version: '1.103.2', // TODO(VSCODE-700) Once we fix the test setup issues, we should revert this to 'insiders'
version: 'insiders',
Copy link
Contributor

Choose a reason for hiding this comment

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

<3

extensionDevelopmentPath,
extensionTestsPath,
launchArgs: [testWorkspace, '--disable-extensions'],
Expand Down
4 changes: 3 additions & 1 deletion src/test/suite/connectionController.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ const sleep = (ms: number): Promise<void> => {
};

suite('Connection Controller Test Suite', function () {
this.timeout(5000);
this.timeout(10000);

const extensionContextStub = new ExtensionContextStub();
const testStorageController = new StorageController(extensionContextStub);
Expand Down Expand Up @@ -73,6 +73,8 @@ suite('Connection Controller Test Suite', function () {
extensionContextStub._workspaceState = {};
extensionContextStub._globalState = {};

testConnectionController.cancelConnectionAttempt();

await testConnectionController.disconnect();
testConnectionController.clearAllConnections();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import { beforeEach, afterEach } from 'mocha';
import chai from 'chai';
import sinon from 'sinon';
import PlaygroundSelectionCodeActionProvider from '../../../editors/playgroundSelectionCodeActionProvider';
import { LanguageServerController } from '../../../language';
import { mdbTestExtension } from '../stubbableMdbExtension';
import { PlaygroundController } from '../../../editors';
import { TEST_DATABASE_URI } from '../dbTestHelper';
Expand Down Expand Up @@ -32,11 +31,6 @@ suite('Playground Selection Code Action Provider Test Suite', function () {
let testActiveTextEditor;

beforeEach(async () => {
sandbox.replace(
mdbTestExtension.testExtensionController,
'_languageServerController',
new LanguageServerController(extensionContextStub),
);
sandbox.stub(vscode.window, 'showInformationMessage');
sandbox.stub(
mdbTestExtension.testExtensionController._telemetryService,
Expand Down Expand Up @@ -79,7 +73,6 @@ suite('Playground Selection Code Action Provider Test Suite', function () {
.getConfiguration('mdb')
.update('confirmRunAll', false);

await mdbTestExtension.testExtensionController._languageServerController.startLanguageServer();
await mdbTestExtension.testExtensionController._playgroundController._activeConnectionChanged();

testActiveTextEditor = sandbox.stub(vscode.window, 'activeTextEditor');
Expand Down
20 changes: 15 additions & 5 deletions src/test/suite/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,11 +54,21 @@ export async function run(): Promise<void> {
try {
// Run the mocha test.
mocha.run((failures) => {
if (failures > 0) {
e(new Error(`${failures} tests failed.`));
} else {
c();
}
// Deactivate the extension to properly clean up the language server
void mdbTestExtension.testExtensionController
.deactivate()
.then(() => {
if (failures > 0) {
e(new Error(`${failures} tests failed.`));
} else {
c();
}
})
.catch((deactivateErr) => {
console.error('Error deactivating extension:');
console.error(deactivateErr);
e(deactivateErr);
});
});
} catch (mochaRunErr) {
console.error('Error running mocha tests:');
Expand Down
9 changes: 4 additions & 5 deletions src/test/suite/language/languageServerController.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import chaiAsPromised from 'chai-as-promised';
import PlaygroundSelectionCodeActionProvider from '../../../editors/playgroundSelectionCodeActionProvider';
import ConnectionController from '../../../connectionController';
import EditDocumentCodeLensProvider from '../../../editors/editDocumentCodeLensProvider';
import { LanguageServerController } from '../../../language';
import { mdbTestExtension } from '../stubbableMdbExtension';
import { PlaygroundController } from '../../../editors';
import PlaygroundResultProvider from '../../../editors/playgroundResultProvider';
Expand All @@ -21,6 +20,7 @@ import { TEST_DATABASE_URI } from '../dbTestHelper';
import { TelemetryService } from '../../../telemetry';
import { ExtensionContextStub } from '../stubs';
import ExportToLanguageCodeLensProvider from '../../../editors/exportToLanguageCodeLensProvider';
import type { LanguageServerController } from '../../../language';

const expect = chai.expect;

Expand Down Expand Up @@ -58,9 +58,9 @@ suite('Language Server Controller Test Suite', () => {
const sandbox = sinon.createSandbox();

before(async () => {
languageServerControllerStub = new LanguageServerController(
extensionContextStub,
);
languageServerControllerStub =
mdbTestExtension.testExtensionController._languageServerController;

const testExportToLanguageCodeLensProvider =
new ExportToLanguageCodeLensProvider(testPlaygroundResultProvider);

Expand All @@ -73,7 +73,6 @@ suite('Language Server Controller Test Suite', () => {
playgroundSelectionCodeActionProvider: testCodeActionProvider,
exportToLanguageCodeLensProvider: testExportToLanguageCodeLensProvider,
});
await languageServerControllerStub.startLanguageServer();
await testPlaygroundController._activeConnectionChanged();
});

Expand Down
6 changes: 3 additions & 3 deletions src/test/suite/views/webviewController.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -157,9 +157,9 @@ suite('Webview Test Suite', () => {
suite('when oidc device auth flow setting is enabled', function () {
let originalDeviceAuthFlow;
before(async function () {
originalDeviceAuthFlow = vscode.workspace.getConfiguration(
'mdb.showOIDCDeviceAuthFlow',
);
originalDeviceAuthFlow = vscode.workspace
.getConfiguration('mdb')
.get('showOIDCDeviceAuthFlow');

await vscode.workspace
.getConfiguration('mdb')
Expand Down
Loading