Skip to content

fix: acceptance tests for secrets test output [PS-357]#6628

Open
alexandru-manea-snyk wants to merge 1 commit intomainfrom
fix/PS-357/extension-secrets-output-acceptance-tests
Open

fix: acceptance tests for secrets test output [PS-357]#6628
alexandru-manea-snyk wants to merge 1 commit intomainfrom
fix/PS-357/extension-secrets-output-acceptance-tests

Conversation

@alexandru-manea-snyk
Copy link
Contributor

@alexandru-manea-snyk alexandru-manea-snyk commented Mar 9, 2026 •
edited
Loading

Pull Request Submission Checklist

  • Follows CONTRIBUTING guidelines
  • Commit messages
    are release-note ready, emphasizing
    what was changed, not how.
  • Includes detailed description of changes
  • Contains risk assessment (Low | Medium | High)
  • Highlights breaking API changes (if applicable)
  • Links to automated tests covering new functionality
  • Includes manual testing instructions (if necessary)
  • Updates relevant GitBook documentation (PR link: ___)
  • Includes product update to be announced in the next stable release notes

What does this PR do?

This PR adds acceptance tests to validate the SARIF and human-readable outputs of the secrets test command. These tests are designed to codify our rendering expectations and serve as a shared contract for the CLI team to iterate against.

Where should the reviewer start?

  • test/jest/acceptance/snyk-secrets/snyk-secrets-test-user-journey.spec.ts;
  • the document linked in the ticket;

How should this be manually tested?

Run the acceptance tests locally.

What's the product update that needs to be communicated to CLI users?

N/A

Risk assessment (Low | Medium | High)?

Low - extends test suite.

What are the relevant tickets?

@snyk-io
Copy link

snyk-io bot commented Mar 9, 2026 •
edited
Loading

✅ Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
✅ Open Source Security 0 0 0 0 0 issues
✅ Licenses 0 0 0 0 0 issues
✅ Code Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@alexandru-manea-snyk alexandru-manea-snyk force-pushed the fix/PS-357/extension-secrets-output-acceptance-tests branch 6 times, most recently from 97767d5 to b04f5e4 Compare March 13, 2026 13:25
@alexandru-manea-snyk alexandru-manea-snyk marked this pull request as ready for review March 13, 2026 13:25
@alexandru-manea-snyk alexandru-manea-snyk requested review from a team as code owners March 13, 2026 13:25
@snyk-pr-review-bot

This comment has been minimized.

Sign in to view
@alexandru-manea-snyk alexandru-manea-snyk force-pushed the fix/PS-357/extension-secrets-output-acceptance-tests branch from b04f5e4 to 624110a Compare March 13, 2026 14:42
@snyk-pr-review-bot

This comment has been minimized.

Sign in to view
@alexandru-manea-snyk alexandru-manea-snyk force-pushed the fix/PS-357/extension-secrets-output-acceptance-tests branch from 624110a to 1352c55 Compare March 13, 2026 15:02
@snyk-pr-review-bot

This comment has been minimized.

Sign in to view
@alexandru-manea-snyk alexandru-manea-snyk force-pushed the fix/PS-357/extension-secrets-output-acceptance-tests branch from 1352c55 to 7f2d38b Compare March 18, 2026 08:48
@snyk-pr-review-bot

This comment has been minimized.

Sign in to view
@alexandru-manea-snyk alexandru-manea-snyk force-pushed the fix/PS-357/extension-secrets-output-acceptance-tests branch from 7f2d38b to cc9753d Compare March 18, 2026 12:37
@snyk-pr-review-bot

This comment has been minimized.

Sign in to view
@alexandru-manea-snyk alexandru-manea-snyk force-pushed the fix/PS-357/extension-secrets-output-acceptance-tests branch from cc9753d to 74f0b68 Compare March 18, 2026 15:34
@snyk-pr-review-bot

This comment has been minimized.

Sign in to view
@alexandru-manea-snyk alexandru-manea-snyk force-pushed the fix/PS-357/extension-secrets-output-acceptance-tests branch from 74f0b68 to cd18a7a Compare March 18, 2026 15:42
@snyk-pr-review-bot
Copy link

snyk-pr-review-bot bot commented Mar 18, 2026

PR Reviewer Guide 🔍

🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Test Logic Error 🟠 [major]

There is a logic contradiction between the setup of the isolated environment and the assertions in the human-readable output test.

The setupIsolatedIgnoreEnv function copies the same secret file twice to create multiple locations for the same secret identity, which is correctly verified in the SARIF grouping test. However, the should correctly render multiple ignores test (starting at line 317) expects two distinct ignore reasons (metadata 0 and metadata 1).

If Snyk Secrets groups these occurrences into a single finding identity (as the SARIF test at line 439-442 assumes), findingIds will only contain one unique identity. Consequently, the loop at line 133 will only run once, only one ignore will be created, and the assertion at line 341 for Test ignore reason metadata 1 will fail.

 
it('should correctly render multiple ignores and their metadata in the output', async () => {
  const { testDir, cleanup } =
    await setupIsolatedIgnoreEnv(TEMP_LOCAL_PATH);

  try {
    // Get human-readable with the ignores included
    const { stdout, stderr, code } = await runSnykCLI(
      `secrets test "${testDir}" --include-ignores`,
      { env, cwd: testDir },
    );

    expect(stderr).toBe('');
    expect(code).toBe(EXIT_CODES.VULNS_FOUND);

    // Multiple ignores are rendered properly
    expect(stdout).toMatch(/Ignored:\s*[2-9]/);
    expect(stdout).toContain('! [IGNORED]');

    // Validate ignores metadata is mapped and rendered correctly
    // Validates Expiration format
    expect(stdout).toMatch(/Expiration:\s+[A-Z][a-z]+\s+\d{2},\s+\d{4}/);

    // Validates the Reason field and spacing
    expect(stdout).toMatch(/Reason:\s+Test ignore reason metadata 0/);
    expect(stdout).toMatch(/Reason:\s+Test ignore reason metadata 1/);
Inconsistent Quoting 🟡 [minor]

The PR correctly updates most runSnykCLI calls to wrap paths in double quotes to handle potential spaces in the file system. However, the call on line 269 is missing these quotes, which is inconsistent with the other changes in this file (e.g., lines 151, 161, 296).

 
`secrets test ${TEMP_LOCAL_PATH}/${TEST_DIR}`,
Redundant Implementation 🟡 [minor]

The copyFolderSync function (lines 65-73) is a custom recursive implementation that does not handle symbolic links (using statSync instead of lstatSync), which can lead to infinite recursion if circular symlinks are present in the source. Since fs-extra is already a development dependency in this project (as seen in package.json), it is safer and more efficient to use fs.copySync() from fs-extra instead of reinventing the logic.

 
const copyFolderSync = (from: string, to: string) => {
  mkdirSync(to, { recursive: true });
  readdirSync(from).forEach((element) => {
    const fromPath = join(from, element);
    const toPath = join(to, element);
    if (statSync(fromPath).isFile()) copyFileSync(fromPath, toPath);
    else copyFolderSync(fromPath, toPath);
  });
};
📚 Repository Context Analyzed

This review considered 11 relevant code sections from 8 files (average relevance: 0.94)

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

Reviewers

@CatalinSnyk CatalinSnyk CatalinSnyk left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@alexandru-manea-snyk @CatalinSnyk