Extract audit fallback test setup#350
Merged
Merged
Conversation
Move the repeated retired-endpoint pnpm mock sequence into a local helper inside the `runAuditJson` suite. Keep the non-empty dependency fallback scenario on its existing setup so the test cases remain distinct.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
This pull request refactors frontend test code and tightens test assertions for the audit utility suite, and updates dependency override parity checks and related fixtures. Test refactor and assertion improvements
Dependency override parity and docs
Validation and remaining signals
Scope
WalkthroughIntroduce setupRetiredEndpointFallback and assertFallbackSpawnCalls for retired pnpm-audit fallback tests; add ip-address to override-parity checks, bump basic-ftp to 5.3.1, update parity-check script and tests, and refresh the developer-guide example output. ChangesRetired endpoint fallback updates
Override parity and docs update
Sequence Diagram(s)sequenceDiagram
participant Test
participant Helper
participant spawnSync
participant fetch
Test->>Helper: setupRetiredEndpointFallback()
Helper->>spawnSync: pnpm audit / pnpm ls outputs
Test->>spawnSync: assertFallbackSpawnCalls()
Test->>fetch: perform advisory request
fetch-->>Test: mocked response
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 warning, 4 inconclusive)
✅ Passed checks (13 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend-pwa/scripts/audit-utils.test.mjs`:
- Around line 78-99: The helper setupRetiredEndpointFallback currently
duplicates the 410 retired-endpoint fixture by directly mocking spawnSyncMock
with createPnpmResult; replace its body to call the existing
setupRetiredPnpmAudit helper instead so the 410 payload/message is
centralized—specifically, remove the spawnSyncMock.mockReturnValueOnce(...)
chain in setupRetiredEndpointFallback and invoke setupRetiredPnpmAudit([{ name:
'frontend-pwa', dependencies }]) so createPnpmResult and the
ERR_PNPM_AUDIT_BAD_RESPONSE payload are provided by the single reusable
setupRetiredPnpmAudit implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 08272ee8-6a50-40ac-bbae-27492db035c5
📒 Files selected for processing (1)
frontend-pwa/scripts/audit-utils.test.mjs
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Replace duplicate 410 pnpm retry mock setup in setupRetiredEndpointFallback by reusing setupRetiredPnpmAudit for the shared retired-endpoint payload and message.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Add explicit assertions for the retired-endpoint fallback path so the tests verify the pnpm audit command runs first, the dependency listing runs second, and the bulk advisory endpoint is called.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. frontend-pwa/scripts/audit-utils.test.mjs Comment on file ❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
Move the repeated retired-endpoint spawn call-order checks into a local helper in the runAuditJson suite. Keep the asserted commands and encoding options unchanged while removing duplicated test assertions.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Pin `basic-ftp` and `ip-address` to patched releases in both Bun and pnpm override blocks. Regenerate the lockfiles so the audit path resolves away from the vulnerable transitive versions reported by pnpm audit. Include `ip-address` in the override parity check so future security pins stay aligned across both installers.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Replace the duplicated retired-endpoint error-path tests with a single table-driven Vitest case. Keep the mocked responses and fallback flow assertions unchanged while removing the repeated test structure.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend-pwa/scripts/audit-utils.test.mjs`:
- Around line 203-243: The table-driven test currently only checks call counts
for fetch but doesn't verify the actual fallback URL used; after the existing
expectations in the test for runAuditJson (i.e. after the spawnSync and fetch
call count assertions) add an assertion to verify that fetch was invoked with
the retired/fallback endpoint URL returned by setupRetiredEndpointFallback (or
the concrete fallback URL string) — locate the test using the names runAuditJson
and setupRetiredEndpointFallback and assert fetch.mock.calls[0][0] (or
equivalent first-argument check) equals the expected fallback URL to harden the
error-path test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8525adcb-dd3f-4e39-805a-e9561ce9be0c
📒 Files selected for processing (1)
frontend-pwa/scripts/audit-utils.test.mjs
Owner
Author
|
@coderabbitai Have the following now been resolved? frontend-pwa/scripts/audit-utils.test.mjs (1)
Apply interaction assertions in the three tests@@
it('throws a clear error when the bulk advisory endpoint fails', async () => {
setupRetiredEndpointFallback();
@@
await expect(runAuditJson()).rejects.toThrow(
'Bulk advisory audit failed (503 Service Unavailable)',
);
+ expect(spawnSyncMock).toHaveBeenNthCalledWith(
+ 1,
+ 'pnpm',
+ ['audit', '--json'],
+ expect.objectContaining({ encoding: 'utf8' }),
+ );
+ expect(spawnSyncMock).toHaveBeenNthCalledWith(
+ 2,
+ 'pnpm',
+ ['ls', '--json', '--depth', 'Infinity'],
+ expect.objectContaining({ encoding: 'utf8' }),
+ );
+ expect(fetch).toHaveBeenCalledTimes(1);
+ expect(String(fetch.mock.calls[0][0])).toBe(
+ 'https://registry.npmjs.org/-/npm/v1/security/advisories/bulk',
+ );
});
@@
it('normalizes advisory IDs from the bulk payload URL to lowercase groups', async () => {
setupRetiredEndpointFallback();
@@
expect(result.json.advisories).toEqual({
'GHSA-vghf-hv5q-vc2g': expect.objectContaining({
[githubAdvisoryIdKey]: 'GHSA-vghf-hv5q-vc2g',
[packageNameKey]: 'validator',
}),
});
+ expect(spawnSyncMock).toHaveBeenNthCalledWith(
+ 1,
+ 'pnpm',
+ ['audit', '--json'],
+ expect.objectContaining({ encoding: 'utf8' }),
+ );
+ expect(spawnSyncMock).toHaveBeenNthCalledWith(
+ 2,
+ 'pnpm',
+ ['ls', '--json', '--depth', 'Infinity'],
+ expect.objectContaining({ encoding: 'utf8' }),
+ );
+ expect(fetch).toHaveBeenCalledTimes(1);
+ expect(String(fetch.mock.calls[0][0])).toBe(
+ 'https://registry.npmjs.org/-/npm/v1/security/advisories/bulk',
+ );
});
@@
it('rejects blank bulk advisory responses instead of treating them as empty JSON', async () => {
setupRetiredEndpointFallback();
@@
await expect(runAuditJson()).rejects.toThrow(
'Failed to parse bulk advisory audit JSON: response body was empty.',
);
+ expect(spawnSyncMock).toHaveBeenNthCalledWith(
+ 1,
+ 'pnpm',
+ ['audit', '--json'],
+ expect.objectContaining({ encoding: 'utf8' }),
+ );
+ expect(spawnSyncMock).toHaveBeenNthCalledWith(
+ 2,
+ 'pnpm',
+ ['ls', '--json', '--depth', 'Infinity'],
+ expect.objectContaining({ encoding: 'utf8' }),
+ );
+ expect(fetch).toHaveBeenCalledTimes(1);
+ expect(String(fetch.mock.calls[0][0])).toBe(
+ 'https://registry.npmjs.org/-/npm/v1/security/advisories/bulk',
+ );
});Also applies to: 214-241, 243-256 🤖 Prompt for AI Agents |
This comment was marked as resolved.
This comment was marked as resolved.
Add the bulk advisory endpoint URL assertion to the retired-endpoint error-path table test. Keep the existing mock flow and error assertions unchanged while verifying the fallback request target.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Add the missing fetch call-count assertion to the advisory ID fallback test so it verifies both the request count and the bulk advisory URL.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai Have the following failed checks now been resolved? If further work is required, please provide an AI agent prompt for the remaining work to be done to address these failures. Do not treat warnings as optional or aspirational. Where a change is out of scope for this PR, propose a GitHub issue unless one exists already. 🚥 Pre-merge checks | ✅ 14 | ❌ 4 ❌ Failed checks (1 warning, 3 inconclusive)
|
This comment was marked as resolved.
This comment was marked as resolved.
Add JSDoc descriptions and return tags for the retired-endpoint fallback helpers in the audit utility test suite. Keep the test logic unchanged while improving docstring coverage.
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 8406 Let me check whether a GitHub issue already exists for the snapshot-test requirement before proposing one. 🏁 Script executed: Length of output: 6 No existing issue found. Creating one now. 🏁 Script executed: Length of output: 46
✅ All resolved
📋 Out of scope — tracked as issue
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This branch extracts the repeated retired-endpoint pnpm mock setup in the frontend audit utility tests so the fallback cases share one local helper while preserving their existing assertions and fetch setup.
No issue or roadmap task is referenced for this branch.
Review walkthrough
setupRetiredEndpointFallbackhelper inside therunAuditJsonsuite.spawnSyncMocksetup changed and the non-empty dependency fallback scenario remains separate.Validation
make check-fmt: passedmake lint: passedmake test: passedpnpm exec vitest run scripts/audit-utils.test.mjsfromfrontend-pwa: passedNotes
The branch is intentionally limited to test refactoring. It does not change production source files or exported symbols.