Skip to content

test: add coverage for phantom-utils (getDeclaredPackages and installCmd) - #1124

Open
emihesi99 wants to merge 1 commit into
OWASP:mainfrom
emihesi99:feature/issue-1088-phantom-utils-tests
Open

test: add coverage for phantom-utils (getDeclaredPackages and installCmd)#1124
emihesi99 wants to merge 1 commit into
OWASP:mainfrom
emihesi99:feature/issue-1088-phantom-utils-tests

Conversation

@emihesi99

Copy link
Copy Markdown

What does this PR do?

Adds unit tests for the two pure helper functions in src/overrides/detectors/phantom-utils.ts that previously had no coverage, as requested in #1088.

Which issue does this PR fix?

Closes #1088

Changes

  • tests/overrides/detectors/phantom-utils.test.ts (new): 12 tests total.

getDeclaredPackages (7 tests)

  • Returns an empty Set for an empty package.json
  • Collects names from all four dependency sections
  • Ignores sections that are arrays, null, or strings
  • Deduplicates names that appear in multiple sections
  • Handles scoped package names

installCmd (5 tests)

  • Returns pnpm add for pnpm
  • Returns yarn add for yarn
  • Returns bun add for bun
  • Returns npm install for npm
  • Returns npm install as the default for an unknown package manager

Verification

Ran the full CI-matching sequence locally on Ubuntu with Node.js v22.22.1:

npm ci
npm run lint:tests
npm run build
node dist/index.js advisories sync
npm test

All checks passed. The full test suite shows 1828 passed, 2 flaky (network-dependent tests in tests/cli-integration.test.ts, confirmed passing on re-run).

Test hygiene

  • Uses the existing describe/it style from tests/overrides/detectors/oa001.test.ts
  • Imports use the .js extension as required by the project's ESM setup
  • No focused tests, no Mocha-isms (verified by npm run lint:tests)

…Cmd)

Adds unit tests for the two pure helper functions in src/overrides/detectors/phantom-utils.ts.

Covers:
- getDeclaredPackages: empty package.json, all four dependency sections, ignored non-object sections (array/null/string), deduplication, scoped names
- installCmd: pnpm/yarn/bun/npm mappings and the npm fallback for unknown package managers

Closes OWASP#1088

Signed-off-by: emihesi99 <lmr.alefl@gmail.com>

@sonukapoor sonukapoor left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for this, and sorry your comments on the issue went unanswered before you started.

Something you cannot see from your end: I ran your tests against 24 deliberately broken versions of phantom-utils.ts to check they would actually catch a regression. They killed 19 of the 20 that are reachable. That is well above average for a test-only change, and a lot more than "the tests pass".

The part I liked most is the three malformed-section tests. getDeclaredPackages has a three-clause guard and you wrote exactly one test per clause with no redundancy. Drop !Array.isArray and only the array test fails. Drop the typeof check and only the string test fails. Drop the null check and only the null test fails. The issue asked for the array and null cases, so the string case is yours, and it is the only thing covering that middle clause.

You also put the file in tests/overrides/detectors/ rather than where the issue said, which is the right call since it mirrors the source path and sits with its siblings. Good instinct to check the layout instead of following the instructions literally.

Two small things and then this is ready.


describe("installCmd", () => {
it("returns 'pnpm add' for pnpm", () => {
expect(installCmd("pnpm" as OverrideContext["packageManager"])).toBe(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

These five casts are not doing anything. PackageManager is already "npm" | "pnpm" | "yarn" | "bun" | "unknown" (src/overrides/context.ts:14), so every string you pass is already a member of the union. I checked by deleting all five plus the then-unused import on line 5, and tsc reports no errors.

Worth removing rather than leaving, because the cast suppresses the one error you would actually want. If someone later drops "unknown" or renames "bun", these tests keep compiling and the drift goes unnoticed. Uncast, the build breaks and tells you, which for a file whose whole job is to pin this function's contract is the behaviour you want.

It also collapses each assertion back to one line:

expect(installCmd("pnpm")).toBe("pnpm add");

};
const result = getDeclaredPackages(pkg);
expect(result).toEqual(new Set(["jest"]));
});

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

One case missing, and it is the only real gap the mutation run found. I added "overrides" to the section list in getDeclaredPackages and all 12 tests still passed.

It matters slightly more than it looks, because this function decides whether a package counts as declared for PD001 (high severity, and it carries an override) and PD002. A real package.json has overrides, resolutions and scripts sitting at the same level as the four sections that should count.

Something like:

it("ignores sections that are not dependency sections", () => {
  const pkg = {
    dependencies: { react: "^18.0.0" },
    overrides: { lodash: "4.17.21" },
    resolutions: { minimist: "1.2.8" },
    scripts: { build: "tsc" },
  };
  expect(getDeclaredPackages(pkg)).toEqual(new Set(["react"]));
});

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test: add coverage for phantom-utils (getDeclaredPackages and installCmd)

2 participants