#1974

fix(test): restore the credential-file tests broken by bare builtin imports

Opencarlosjarenomwants to merge intofrom

What Problem This Solves

test/unit/electron/main/envUtil.test.ts had 14 failing tests, every one reporting util.isString is not a function. They have been failing since #1895 and #1955 landed, so the suite covering how .env credential files are written was providing no protection at all — the fixes it was supposed to guard could have regressed silently.

Root cause

A bare import path from 'path' in envUtil.ts. Under the jsdom test environment, Vite resolves the bare specifier to the npm path package instead of the builtin, and that package's internal require('util') is served a browser stub whose named exports live under default. So util.isString is undefined and every path.join throws:

TypeError: util.isString is not a function
 ❯ Object.posix.join node_modules/path/path.js:489:15
 ❯ Module.writeEnvFile electron/main/utils/envUtil.ts:39:30

import path from 'node:path' resolves to the real builtin and works. Confirmed directly:

path.join (bare):   FALLA: util.isString is not a function
node:path.join:     ok

The rest of the tree already does this — 24 files use node:path and none of them use the bare form. Only electron/main had drifted, which is exactly where the broken tests live.

Solution Details

  • node: prefix for all 17 bare builtin imports across 7 files in electron/: path, fs, fs/promises, os, util, http, net, child_process, crypto. This is the direction the codebase was already going, and it fixes the tests at the source.
  • An ESLint no-restricted-imports guardrail for electron/**, in the same style as the existing no-restricted-properties guardrail, so a bare builtin cannot come back. Verified it fires: restoring a bare from 'path' produces the error.

Why not a vitest alias

I tried resolve.alias: { path: 'node:path' } first and it does fix the tests. I did not keep it: it is a test-only band-aid that leaves the source inconsistent, and it would silently mask the same problem for the other 16 bare imports. Fixing the source is the same amount of work and matches what the codebase already does everywhere else.

Why nobody noticed

CI only runs the tests a PR touches:

# The full suite currently contains known failures, so keep this gate
# focused on tests that the change adds or updates.
- name: Run changed frontend tests
  run: node scripts/run-changed-vitest.mjs

So the failures were tolerated rather than detected. Recovering these 14 shrinks that backlog.

Evidence

  • vitest run test/unit/electron/main/envUtil.test.ts — 16 passed (was 14 failed / 2 passed)
  • Full suite: 55 failed / 15 files → 41 failed / 14 files, 3675 → 3689 passed
  • No test that passed before fails now — the 14 recovered are exactly the envUtil ones
  • npm run type-check — clean
  • npx eslint electron/ eslint.config.js — clean; the repo's own pre-commit hooks pass

The 41 remaining failures are pre-existing and unrelated (integration tests, component tests, install-deps, buildConfig). The last two fail for different reasons — a subprocess and test timeouts, not module resolution — so I left them alone rather than widening the diff.

User Impact

None directly; this repairs test infrastructure. The practical effect is that the credential-file behaviour introduced by #1895 and #1955 is covered again, and future changes to it get checked.

Not referencing an issue: this is infrastructure repair rather than a product bug.

Activity

2
carlosjarenom1mo ago

Rebased onto current main and re-verified while it waited for review:

  • npm run type-check and eslint are clean.
  • Full backend suite: 3696 passed, 12 skipped — identical to the main baseline.
  • Conflicts with main resolved by hand; the fix is now scoped to just the files it needs.

The linked issue is still open, and this is ready for a look whenever you have time.