Skip to content

Centralize and normalize PackageManager and EnvironmentManager's IDs - #1934

Open
Eduardo Villalpando Mello (edvilme) wants to merge 6 commits into
mainfrom
api-2-manager-ids
Open

Eduardo Villalpando Mello (edvilme) wants to merge 6 commits into
mainfrom
api-2-manager-ids

Conversation

@edvilme

Copy link
Copy Markdown
Contributor

Use centralized manager IDs from /src/common/constants.ts throughout the codebase.

When querying manager capabilities, use the centralized manager ids or an "undefined" fallback.

@edvilme Eduardo Villalpando Mello (edvilme) added the debt Code quality issues label Oct 14, 2026
@edvilme Eduardo Villalpando Mello (edvilme) changed the title REF: Centralize manager ids Centralize and normalize PackageManager and EnvironmentManager's IDs Oct 14, 2026
@edvilme
Eduardo Villalpando Mello (edvilme) marked this pull request as ready for review October 14, 2026 22:08

This comment was marked as resolved.

@edvilme Eduardo Villalpando Mello (edvilme) added the no-merge PR is currently blocked from merging label Oct 14, 2026
@heejaechang

Heejae Chang (heejaechang) commented Oct 14, 2026 •

Copy link
Copy Markdown

🔒 Automated review in progress — Heejae Chang (@heejaechang) is auto-reviewing this PR.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

Operational IDs remain unchanged, telemetry filtering has focused coverage, and no blocking issues remain.

0 open findings

1 resolved since last review

🧠 Review effort: Balanced

@heejaechang

Copy link
Copy Markdown

Result: ⚠️ partially-verified

Verification details

Verification: Isolated verification observed failures that were not classified as caused by this PR: Additional telemetry-boundary probe.

Summary: TypeScript compilation succeeded, and 1,027 targeted unit tests passed, including both new telemetry-ID tests and the updated capability/timeout assertions. Six tests were pending; no repository tests failed. An additional telemetry-boundary probe failed before its assertions because my harness omitted the extension-identity stub used by existing tests. VS Code integration tests were not run, so verification is partial.

Test runs: 5 passed, 1 failed

  • ❌ Failed | unrelated to this PR | Additional telemetry-boundary probe | node <<'NODE'
    require('./out/test/unittests.js');
    const assert = require('node:assert/strict');
    const sinon = require('sinon');
    const {createHash} = require('node:crypto');
    const {CancellationError, Uri} = require('vscode');
    const sender = require('./out/common/telemetry/sender.js');
    const settings = require('./out/features/settings/settingHelpers.js');
    const {EventNames: E} = require('./out/common/telemetry/constants.js');
    const {sendManagerSelectionTelemetry} = require('./out/common/telemetry/helpers.js');
    const {PythonEnvironmentManagers} = require('./out/features/envManagers.js');
    const {InternalPackageManager} = require('./out/managers/common/registeredManagers.js');
    const telemetry = sinon.stub(sender, 'sendTelemetryEvent');
    const hash = id => createHash('sha256').update(id, 'utf8').digest('hex');
    (async () => {
    const id = 'third.party:tool', pkgId = 'third.party:packages';
    const environment = {envId: {managerId: id, id: 'env'}};
    const provider = {name: 'tool', preferredPackageManagerId: pkgId, refresh: sinon.stub().resolves(), getEnvironments: async () => [environment], get: async () => environment, set: async () => {}};
    const managers = new PythonEnvironmentManagers({getProjects: () => []});
    const registration = managers.registerEnvironmentManager(provider, {extensionId: 'third.party'});
    const packageRegistration = managers.registerPackageManager({name: 'packages'}, {extensionId: 'third.party'});
    assert.equal(managers.getEnvironmentManager(id).id, id);
    assert.equal((await managers.getEnvironmentManager(id).get()).envId.managerId, id);
    for (const [event, raw] of [[E.ENVIRONMENT_MANAGER_REGISTERED, id], [E.PACKAGE_MANAGER_REGISTERED, pkgId]]) assert.equal(telemetry.withArgs(event).lastCall.args[2].managerId, hash(raw));
    await managers.getEnvironmentManager(id).refresh('all');
    const failure = new Error('Expected service failure');
    provider.refresh.rejects(failure);
    await assert.rejects(managers.getEnvironmentManager(id).refresh('all'), error => error === failure);
    const discoveries = telemetry.withArgs(E.ENVIRONMENT_DISCOVERY).getCalls().map(call => call.args[2]);
    assert.deepEqual(discoveries.map(p => [p.managerId, p.result]), [[hash(id), 'success'], [hash(id), 'error']]);
    const manage = sinon.stub().resolves();
    const packages = new InternalPackageManager(pkgId, {name: 'packages', manage});
    await packages.manage(environment, {});
    for (const error of [new CancellationError(), failure]) {
    manage.rejects(error);
    await assert.rejects(packages.manage(environment, {}), caught => caught === error);
    }
    assert.deepEqual(telemetry.withArgs(E.PACKAGE_MANAGEMENT).getCalls().map(call => [call.args[2].managerId, call.args[2].result]), [[hash(pkgId), 'success'], [hash(pkgId), 'cancelled'], [hash(pkgId), 'error']]);
    assert.equal(packages.id, pkgId);
    sinon.stub(settings, 'getDefaultEnvManagerSetting').returns(id);
    sinon.stub(settings, 'getDefaultPkgManagerSetting').returns(pkgId);
    sendManagerSelectionTelemetry({getProjects: () => [{uri: Uri.file('/workspace/project')}]});
    assert.equal(telemetry.withArgs(E.ENVIRONMENT_MANAGER_SELECTED).lastCall.args[2].managerId, hash(id));
    assert.equal(telemetry.withArgs(E.PACKAGE_MANAGER_SELECTED).lastCall.args[2].managerId, hash(pkgId));
    registration.dispose(); packageRegistration.dispose(); managers.dispose();
    console.log('PASS: hashed registration, discovery success/error, package success/cancellation/error, and manager selection telemetry; raw routing/environment IDs and error identity preserved.');
    })().catch(error => { console.error(error); process.exitCode = 1; }).finally(() => sinon.restore());
    NODE
  • ✅ Passed | Compilation and telemetry, capabilities, and readiness tests | npm run compile-tests && node ./node_modules/mocha/bin/mocha.js --no-config --ui tdd --require ./out/test/unittests.js --timeout 180000 --reporter dot out/test/common/telemetry/helpers.unit.test.js out/test/extensionApi.capabilities.unit.test.js out/test/features/common/managerReady.unit.test.js
  • ✅ Passed | Changed common, feature, and internal API regression tests | node ./node_modules/mocha/bin/mocha.js --no-config --ui tdd --require ./out/test/unittests.js --timeout 180000 --reporter dot out/test/common/pythonPath.unit.test.js out/test/common/inlineScript/cacheLayout.unit.test.js out/test/common/inlineScript/interpreter.unit.test.js out/test/extensionApi.unit.test.js out/test/features/envCommands.inlineSelectionErrors.unit.test.js out/test/features/envCommands.unit.test.js out/test/features/envManagers.lastKnown.unit.test.js out/test/features/envManagers.unit.test.js out/test/features/interpreterSelection.unit.test.js out/test/features/projectManager.fileEvents.unit.test.js out/test/features/projectManager.initialize.unit.test.js out/test/features/settings/settingHelpers.unit.test.js out/test/features/terminal/shellStartupActivationVariablesManager.unit.test.js out/test/features/terminal/utils.unit.test.js out/test/features/views/treeViewItems.unit.test.js out/test/internal/publicApiCompatibility.unit.test.js out/test/internal/pythonToolManagers.unit.test.js out/test/internal/pythonTools.startup.unit.test.js out/test/internal/pythonTools.unit.test.js
  • ✅ Passed | Changed manager regression tests | node ./node_modules/mocha/bin/mocha.js --no-config --ui tdd --require ./out/test/unittests.js --timeout 180000 --reporter dot out/test/managers/builtin/inlineScript/envManager.unit.test.js out/test/managers/builtin/pipPackageManager.unit.test.js out/test/managers/builtin/sysPythonManager.savedSelection.unit.test.js out/test/managers/builtin/venvManager.resolve.unit.test.js out/test/managers/builtin/venvUtils.createWithProgress.unit.test.js out/test/managers/conda/condaEnvManager.initialize.unit.test.js out/test/managers/conda/condaUtils.pythonVersions.unit.test.js out/test/managers/fastPath.get.unit.test.js out/test/managers/pipenv/pipenvUtils.getPipenv.unit.test.js out/test/managers/poetry/poetryPackageManager.unit.test.js out/test/managers/common/registeredManagers.versionLookup.unit.test.js
  • ✅ Passed | Dependency and test-runner discovery | printf 'AUTOMATION_SANDBOX_PROFILE=%s\n' "$AUTOMATION_SANDBOX_PROFILE"; node --version; npm --version; git status --short; node -e "const fs=require('fs'); const p=require('./package.json'); console.log(JSON.stringify({scripts:p.scripts, node_modules:fs.existsSync('node_modules'), mocha:fs.existsSync('node_modules/mocha/bin/mocha.js'), typescript:fs.existsSync('node_modules/typescript/bin/tsc'), lockfile:fs.existsSync('package-lock.json'), compiledTests:fs.existsSync('out/test/unittests.js')},null,2));"; cat build/.mocha.unittests.json; sed -n '1,150p' src/test/unittests.ts
  • ✅ Passed | Offline dependency preparation | npm ci --offline --ignore-scripts
❌ Additional telemetry-boundary probe diagnostic output
TypeError: fsPath.replace is not a function
    at normalizePath (/workspace/out/common/utils/pathUtils.js:77:26)
    at getCallingExtension (/workspace/out/common/utils/frameUtils.js:69:64)
    at generateId (/workspace/out/features/envManagers.js:48:52)
    at PythonEnvironmentManagers.registerEnvironmentManager (/workspace/out/features/envManagers.js:104:27)
    at [stdin]:19:33
[container exit=1]
Harness failure before assertions: the mocked extensionPath is not a string. Existing envManagers tests explicitly stub getCallingExtension to avoid this lookup.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Approved via Review Center.

@heejaechang Heejae Chang (heejaechang) added the review-auto:approved Automated review: no blocking findings (approval posted). label Oct 15, 2026

@rchiodo Rich Chiodo (rchiodo) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approved via Review Center.

@bschnurr Bill Schnurr (bschnurr) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Approved via Review Center.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

debt Code quality issues no-merge PR is currently blocked from merging review-auto:approved Automated review: no blocking findings (approval posted).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants