chore: add resilience-test suite and remove individual test by PeterSchafer · Pull Request #6807 · snyk/cli

PeterSchafer · 2026-05-14T16:39:10Z

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 moves existing separate tests into a single test suite for easier overview and maintenance.

Where should the reviewer start?

How should this be manually tested?

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

N/A

Risk assessment (Low | Medium | High)?

Low, changes tests only

snyk-io · 2026-05-14T16:39:37Z

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

snyk-pr-review-bot · 2026-05-14T16:42:50Z

PR Reviewer Guide 🔍

🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review Global Side Effect 🟠 [major] Enabling `isSecretsEnabled` by default in `featureFlagDefaults` affects all acceptance tests in the repository that use the `fakeServer`. The existing comment in this file explicitly states that features should be enabled specifically by tests that target them. By making this a global default, other tests that do not expect or mock secrets-related API calls may fail when the CLI attempts to communicate with those endpoints. ['isSecretsEnabled', true], Brittle Test Scenario 🟡 [minor] In Scenario 4 (`mid-execution-maintenance`), the test sets the next status code to 200 for 4 requests without providing mock response bodies. CLI commands like `test` or `monitor` typically expect valid JSON from their initial API calls (e.g., for organization or user verification). If these calls return an empty 200 response, the CLI will fail with a JSON parsing error before reaching the maintenance failure state, causing the test to fail with an incorrect exit code. server.setNextStatusCode(200); server.setNextStatusCode(200); server.setNextStatusCode(200); server.setNextStatusCode(200);
📚 Repository Context Analyzed This review considered 5 relevant code sections from 4 files (average relevance: 1.00) CONTRIBUTING.md (2 segments, lines 1-218) package.json (lines 1-224) src/cli/main.ts (lines 1-236) cliv2/cmd/cliv2/main.go (lines 1-203)

PeterSchafer · 2026-05-14T16:50:11Z

    ['sbomTestReachability', false],
    ['useTestShimForOSCliTest', false],
    ['cliDotnetRuntimeResolution', false],
+    ['isSecretsEnabled', true],


Maybe we want to enable this only in the tests, to avoid doing this globally.

danskmt · 2026-05-15T10:30:25Z

+  // Scenario 3
+  {
+    name: 'unauthorized-401',
+    description: 'Backend returns 401 Unauthorized',


Nice addition, maybe worth adding to the PR description as well. Scenario 4 too.
nit: Also suggest changing the PR title to chore: unify resilience tests and add 401/mid-execution-maintenance scenarios

danskmt · 2026-05-15T10:31:55Z

-  it('does not attempt any retries', async () => {
-    await runSnykCLI(`test -d --log-level=trace`, {
-      env: {
-        ...env,
-        // apply a user configured attempts of 10
-        INTERNAL_NETWORK_REQUEST_MAX_ATTEMPTS: '10',
-      },
-    });
-
-    // Count how many times an endpoint was hit
-    const requests = server.getRequests();
-    const actualNetworkAttempts = requests.filter(
-      (r) => r.url.includes('/test-dep-graph') || r.url.includes('/vuln/'),
-    ).length;
-
-    expect(actualNetworkAttempts).toBe(1);
-  });


hmm I think this test is missing in the new file?

I am not fully sure why: the PR description mentions "remove individual test" but the backing ticket does not mention any of that

danskmt · 2026-05-15T10:32:57Z

-      INTERNAL_NETWORK_REQUEST_MAX_ATTEMPTS: '1',
-      INTERNAL_NETWORK_REQUEST_RETRY_AFTER_SECONDS: '1',


Also these two are not present in the new file

danskmt · 2026-05-15T10:36:13Z

+    envOverrides: {
+      SNYK_TIMEOUT_SECS: String(TIMEOUT_SECS),
+    },
+    skip: ['container sbom scratch'],


hmmm do we want to skip this here?

danskmt · 2026-05-15T10:37:38Z

-      // Should send instrumentation data even on timeout
-      const requests = server.getRequests();
-      const instrumentationRequest = requests.find((r) =>
-        r.url?.includes(`/api/hidden/orgs/${orgId}/analytics`),
-      );
-      expect(instrumentationRequest).toBeDefined();


I guess this is also absent in the new file

danskmt · 2026-05-15T10:38:51Z

-  beforeEach(async () => {
-    initialConfig = await getCliConfig();
-    // Set server to delay responses longer than the timeout (10s > 5s timeout)
-    server.setResponseDelay(SERVER_DELAY_MS);
-  });


We don't have beforeEach in the new file, is that intentional?

danskmt · 2026-05-15T10:39:53Z

+  'monitor',
+  'whoami',
+  'auth 11111111-2222-3333-4444-555555555555',
+  'sbom --org=11111111-1111-1111-1111-111111111111 --format=cyclonedx1.4+json',


Before it was:

sbom --org=test-org --format=cyclonedx1.4+json

Any reason for changing the org here?

I think this is just about aligning the test with baseEnv defined in lines 195-201

chore: add resilience-test suite and remove individual test

07a89e6

PeterSchafer requested review from a team as code owners May 14, 2026 16:39

PeterSchafer commented May 14, 2026

View reviewed changes

danskmt reviewed May 15, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: add resilience-test suite and remove individual test#6807

chore: add resilience-test suite and remove individual test#6807
PeterSchafer wants to merge 1 commit into
mainfrom
chore/CLI-1494_fault_injection

PeterSchafer commented May 14, 2026

Uh oh!

snyk-io Bot commented May 14, 2026 •

edited

Loading

Uh oh!

snyk-pr-review-bot Bot commented May 14, 2026

Uh oh!

PeterSchafer May 14, 2026

Uh oh!

danskmt May 15, 2026

Uh oh!

danskmt May 15, 2026

Uh oh!

robertolopezlopez May 15, 2026

Uh oh!

danskmt May 15, 2026

Uh oh!

danskmt May 15, 2026

Uh oh!

danskmt May 15, 2026

Uh oh!

danskmt May 15, 2026

Uh oh!

danskmt May 15, 2026

Uh oh!

robertolopezlopez May 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		INTERNAL_NETWORK_REQUEST_MAX_ATTEMPTS: '1',
		INTERNAL_NETWORK_REQUEST_RETRY_AFTER_SECONDS: '1',

Conversation

PeterSchafer commented May 14, 2026

Pull Request Submission Checklist

What does this PR do?

Where should the reviewer start?

How should this be manually tested?

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

Risk assessment (Low | Medium | High)?

Uh oh!

snyk-io Bot commented May 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Uh oh!

snyk-pr-review-bot Bot commented May 14, 2026

PR Reviewer Guide 🔍

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

snyk-io Bot commented May 14, 2026 •

edited

Loading