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

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

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.

@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.

@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
Copy link

PR Reviewer Guide 🔍

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

Fragile Shell Commands 🟠 [major]

Several execSync calls interpolate path variables (like TEMP_LOCAL_PATH and NO_GIT_DIR) directly into shell commands without quoting. This will cause tests to fail if the environment's workspace or temporary directory path contains spaces. Additionally, using shell commands like rm -rf, mkdir -p, and cp -r via execSync is non-portable across operating systems (e.g., Windows). Since the project already depends on rimraf and target Node 20, consider using built-in methods like fs.rmSync(path, { recursive: true, force: true }) or fs.mkdirSync(path, { recursive: true }) for better reliability.

  execSync(`mkdir -p ${NO_GIT_DIR}`);
  execSync(`cp -r ${TEMP_LOCAL_PATH}/${TEST_DIR}/* ${NO_GIT_DIR}/`, { stdio: 'pipe' });

  const { code, stdout, stderr } = await runSnykCLI(
    `secrets test ${NO_GIT_DIR}`,
    { env },
  );

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

  // Finding ID should not be included when it starts with UNDEFINED
  expect(stdout).not.toContain('Finding ID: UNDEFINED');
} finally {
  try {
    execSync(`rm -rf ${NO_GIT_DIR}`, { stdio: 'pipe' });
Unquoted CLI Arguments 🟠 [major]

The runSnykCLI helper is called with unquoted path arguments in multiple test cases. If the execution path contains spaces, the CLI will misinterpret the single path as multiple separate arguments, leading to test failures. This is inconsistent with the correct quoting used for testDir in line 352.

`secrets test ${TEMP_LOCAL_PATH}/${TEST_DIR}`,
{ env },
Weak Test Assertion 🟡 [minor]

The test intended to verify that Finding IDs are omitted when they cannot be calculated (line 256) uses a weak assertion. expect(stdout).not.toContain('Finding ID: UNDEFINED') only proves that the specific "UNDEFINED" string is absent; it doesn't verify that the entire Finding ID: field is omitted from the output as described in the test's purpose. If the output mistakenly included a blank Finding ID: or an incorrect value, the test would still pass.

expect(stdout).not.toContain('Finding ID: UNDEFINED');
📚 Repository Context Analyzed

This review considered 8 relevant code sections from 7 files (average relevance: 0.91)

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.

1 participant