Repository navigation
[AI Improvement] [Task] Replace the per-test require.cache scan in the global mocha afterEach hook with an O(1) custom-nock lookup - #11175
Conversation
There was a problem hiding this comment.
Code Review
This pull request optimizes the test cleanup process in mocha-bootstrap.js by targeting specific cache keys for the custom nock helper instead of scanning the entire require.cache. The reviewer suggests using require.resolve to dynamically and robustly locate the helper path, which eliminates the need for manual path construction and hardcoded file extensions, and updating the cleanup loop to use this resolved path directly.
| // Absolute require.cache keys of the custom undici nock helper (src/test/helpers/nock.ts), | ||
| // which sits next to this file. Looking them up directly avoids scanning ~3800 cache keys | ||
| // after every test. | ||
| const CUSTOM_NOCK_PATHS = [path.join(__dirname, "nock.ts"), path.join(__dirname, "nock.js")]; |
There was a problem hiding this comment.
Using require.resolve is more robust and idiomatic than manually constructing absolute paths with path.join and hardcoding .ts and .js extensions. It leverages Node's internal resolution mechanism to find the exact path used in require.cache, automatically handling whatever extension is active (e.g., .ts when running with ts-node or .js when running compiled code), and avoids potential platform-specific path casing or separator issues.
Once this is applied, the const path = require("path"); import on line 1 is no longer needed and can be removed.
| // Absolute require.cache keys of the custom undici nock helper (src/test/helpers/nock.ts), | |
| // which sits next to this file. Looking them up directly avoids scanning ~3800 cache keys | |
| // after every test. | |
| const CUSTOM_NOCK_PATHS = [path.join(__dirname, "nock.ts"), path.join(__dirname, "nock.js")]; | |
| let customNockPath; | |
| try { | |
| customNockPath = require.resolve("./nock"); | |
| } catch { | |
| // Ignore if the custom nock helper is not present | |
| } |
| for (const key of CUSTOM_NOCK_PATHS) { | ||
| try { | ||
| const mod = require.cache[key]; | ||
| if (mod && mod.exports) { | ||
| const customNock = mod.exports.default || mod.exports; | ||
| if (typeof customNock.cleanAll === "function") { | ||
| customNock.cleanAll(); | ||
| } | ||
| } catch { | ||
| // Ignore cleanup errors from custom nock | ||
| } | ||
| } catch { | ||
| // Ignore cleanup errors from custom nock | ||
| } | ||
| } |
There was a problem hiding this comment.
Update the cleanup loop to use the resolved customNockPath directly instead of iterating over CUSTOM_NOCK_PATHS.
| for (const key of CUSTOM_NOCK_PATHS) { | |
| try { | |
| const mod = require.cache[key]; | |
| if (mod && mod.exports) { | |
| const customNock = mod.exports.default || mod.exports; | |
| if (typeof customNock.cleanAll === "function") { | |
| customNock.cleanAll(); | |
| } | |
| } catch { | |
| // Ignore cleanup errors from custom nock | |
| } | |
| } catch { | |
| // Ignore cleanup errors from custom nock | |
| } | |
| } | |
| if (customNockPath) { | |
| try { | |
| const mod = require.cache[customNockPath]; | |
| if (mod && mod.exports) { | |
| const customNock = mod.exports.default || mod.exports; | |
| if (typeof customNock.cleanAll === "function") { | |
| customNock.cleanAll(); | |
| } | |
| } | |
| } catch { | |
| // Ignore cleanup errors from custom nock | |
| } | |
| } |
Resolves Buganizer b/566321038
Parent Goal: b/566303602
Proposed Improvement
Replaces the
for (const key of Object.keys(require.cache))loop insidecleanup()insrc/test/helpers/mocha-bootstrap.jswith an exact-path lookup of the custom undici nock helper (CUSTOM_NOCK_PATHS = [path.join(__dirname, "nock.ts"), path.join(__dirname, "nock.js")]).During a full unit test run,
require.cachecontains ~3,800 modules. Scanning all keys withendsWith()on every single test execution added ~0.6 ms per test (~3.3 s cumulative across ~5,566 tests) with zero work being done when the custom nock module was not loaded. The exact-path lookup is O(1) while preserving the lazy cleanup semantics (only runningcleanAll()if loaded).Verification
node -e 'require("ts-node/register"); const p=require("path"); require(p.resolve("src/test/helpers/nock.ts")); for (let i=0;i<3800;i++) require.cache["/fake/"+i+".js"]={exports:{}}; const h=require(p.resolve("src/test/helpers/mocha-bootstrap.js")).mochaHooks; const t=process.hrtime.bigint(); for (let i=0;i<5566;i++) h.afterEach(); console.log("5566 x afterEach:", Number(process.hrtime.bigint()-t)/1e6, "ms"); process.exit(0)'):npm run mocha:fast: 5566 passing, 11 pending, 0 failing.npx mocha src/mcp/util/crashlytics/availability.spec.ts src/mcp/util/apptesting/availability.spec.ts): 7 passing (5s).npm run lint:quiet: Clean (0 errors).npm run build: Clean (0 errors).