Skip to content

test: add failing test for multiple JSON output #559

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
snyk-tim wants to merge 1 commit into
base: main
Choose a base branch
from
Open

test: add failing test for multiple JSON output #559

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions pkg/local_workflows/output_workflow_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -464,6 +464,38 @@ func Test_Output_outputWorkflowEntryPoint(t *testing.T) {
assert.Equal(t, expectedOutput, setup.writer.String())
})

// This test demonstrates a bug: when multiple application/json items are passed with --json flag,
// only the first item is output. The PopWritersByMimetype function removes the writer after the
// first use, causing subsequent items to find no writer available.
t.Run("should output all application/json items when json flag is set", func(t *testing.T) {

@basti-snyk basti-snyk Jul 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should-Fix: This adds a knowingly-failing test with no accompanying fix. Verified by running it: assert.Contains(outputStr, "second-project") fails — writer output is {"project":"first-project",...} only. That fails the parent Test_Output_outputWorkflowEntryPoint and makes go test ./pkg/local_workflows/... exit non-zero. If this branch merges to main on its own it red-lines CI for every consumer of the package until the fix lands.

The bug it demonstrates is real and the root cause is correctly identified: writerMapImpl.PopWritersByMimetype (output_workflow/writer.go) does delete(w.writers, mimeType), so the single JSON writer is consumed by the first item and the second finds none.

Before merge, either land the production fix in the same PR/stack (test turns green), or quarantine with t.Skip("<ticket>: multiple application/json items drop all but the first") referencing the bug, removed by the fix commit. If this PR is deliberately the RED step of a stack whose next commit carries the fix, this is acceptable — flagging so the decision is explicit.

— AI review

setup := setupTest(t)
setup.config.Set(output_workflow.OUTPUT_CONFIG_KEY_JSON, true)
defer setup.config.Set(output_workflow.OUTPUT_CONFIG_KEY_JSON, nil)

workflowIdentifier := workflow.NewTypeIdentifier(WORKFLOWID_OUTPUT_WORKFLOW, "output")

jsonPayload1 := `{"project":"first-project","dependencies":["pkg-a","pkg-b"]}`
jsonPayload2 := `{"project":"second-project","dependencies":["pkg-c","pkg-d"]}`

data1 := workflow.NewData(workflowIdentifier, "application/json", []byte(jsonPayload1))
data1.SetContentLocation("first-project/lock.file")

data2 := workflow.NewData(workflowIdentifier, "application/json", []byte(jsonPayload2))
data2.SetContentLocation("second-project/lock.file")

output, err := outputWorkflowEntryPoint(setup.invocationContextMock, []workflow.Data{data1, data2}, setup.outputDestination)

assert.Nil(t, err)
assert.Equal(t, []workflow.Data{}, output)

@bastiandoetsch bastiandoetsch Jun 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[Should Fix] This assertion is a tautology that gives false confidence. outputWorkflowEntryPoint always returns the empty slice declared at output_workflow.go:39 and discards the input/leftover returned by the Handle* calls, so output is []workflow.Data{} regardless of how many items were dropped — it passes in both the buggy and fixed states. It reads as if it verifies 'no data left unprocessed', but the dropped second item is in fact neither written nor returned; this assertion hides that. Drop it, or replace with one that pins a real contract (assert the second item is written, or surfaced as unconsumed — currently it's neither).

— AI review

@basti-snyk basti-snyk Jul 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggestion (non-blocking): assert.Equal(t, []workflow.Data{}, output) is vacuous here — outputWorkflowEntryPoint initializes output := []workflow.Data{} and returns it unchanged (handler results are assigned back into input, never output), so this passes for the same trivial reason in every existing test and cannot detect the multiple-JSON bug. The meaningful signal is the writer content asserted below.

— AI review


// BUG: Currently only the first JSON is output because PopWritersByMimetype
// removes the writer after the first item is processed.
// Expected: both JSON payloads should be in the output
outputStr := setup.writer.String()
assert.Contains(t, outputStr, "first-project", "First JSON payload should be in output")
assert.Contains(t, outputStr, "second-project", "Second JSON payload should be in output (BUG: this fails)")

@bastiandoetsch bastiandoetsch Jun 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[Critical] This is an ungated failing test — go test ./pkg/local_workflows/ fails here today ("...first-project..." does not contain "second-project"), empirically confirmed. There's no t.Skip, build tag, or other gate, and CI runs this in required gates, so merging it alone turns main red for every subsequent pipeline.

The bug it documents is real and well-diagnosed: HandleContentTypeOther loops per item and useWriterWithOther → PopWritersByMimetype deletes the writer from the map after the first use (writer.go:58-62), so the second application/json item finds no writer and is dropped. Nice reproduction.

Resolution (one required before merge): (a) land the production fix in this same PR so the test goes green on commit (preferred — a failing test should accompany its fix), or (b) t.Skip("<TICKET>: multiple application/json items dropped by PopWritersByMimetype — un-skip when fixed") to keep CI green while preserving the repro.

— AI review

})

t.Run("should print valid sarif json output", func(t *testing.T) {
setup := setupTest(t)
expectedSarif := "testdata/sarif-snyk-goof-ignores.json"
Expand Down