test(cli): add artifact CLI tests using mock-based golden framework by sachin9058 · Pull Request #6419 · mindersec/minder

sachin9058 · 2026-04-25T08:06:32Z

Summary

This PR aligns CLI tests with the testing framework introduced in #6386 and #6414 by replacing help-output based tests with full command execution tests using mocked RPC clients and golden file assertions.

Key changes:

Replaced help-based tests with cli.CmdTestCase + cli.RunCmdTests
Added fixture-based mock responses using cli.LoadFixture
Introduced golden file validation for CLI output (table/json/yaml)
Implemented RPC client injection using GoMock for realistic command testing
Applied changes to artifact commands (list, supporting multiple output formats)

The scope is intentionally limited to validate the pattern before expanding it to other CLI commands.

Testing

Ran:
- go test ./cmd/cli/app/artifact -run TestArtifactListCommand -v
- go test ./cmd/cli/... -v
Regenerated golden files using:
- go test ./cmd/cli/app/artifact -run TestArtifactListCommand -v -args -update
Verified:
- Table, JSON, and YAML outputs match expected golden files
- Error handling for server failures
Ensured all CLI tests pass locally without regressions

coveralls · 2026-04-25T08:18:02Z

coverage: 60.554% (-0.006%) from 60.56% — sachin9058:test/cli-standardize into mindersec:main

evankanderson

You might look at the series started in #6386 and continued in #6414 for a more robust framework -- this mostly seems to be testing the output of Cobra`s help function, which seems brittle (what if Cobra changes the output) and unlikely to catch actual output regressions.

sachin9058 · 2026-04-28T11:28:28Z

@evankanderson Thanks

I see that the recent changes in #6386 and #6414 move toward a more realistic CLI testing framework using full command execution and mock injection.

I’ll take a look at those and rework this PR to align with that approach instead of focusing on help output.

sachin9058 · 2026-04-28T12:14:37Z

@evankanderson Updated this to align with the testing approach from #6386/#6414.

Switched to cli.CmdTestCase + RunCmdTests
Added fixture-based mocks and golden file validation
Covered table/json/yaml outputs and error handling

Keeping the scope focused on artifact commands to validate the approach before expanding further.

sachin9058 · 2026-04-28T13:15:44Z

Fixed the CI timeout issue.

Root cause was artifact list still using GRPCClientWrapRunE, which initializes a real gRPC connection before the injected mock client is checked. This caused tests to fall into the browser login flow in CI.

Updated the command to use a direct RunE path with client resolution handled via getArtifactClient, ensuring the injected mock is used first.

Tests no longer trigger login or xdg-open in CI.

evankanderson

Can you add tests for artifact get in this PR, as well as list?

sachin9058 · 2026-04-30T06:51:47Z

@evankanderson Thanks for the suggestion.

I noticed profile/status golden tests are already being added in #6420, so to avoid duplication I added golden-output tests for the artifact commands affected in this PR (specifically artifact get).

This should now cover output validation without overlapping with existing work.

sachin9058 · 2026-04-30T07:11:11Z

@evankanderson

I’ve addressed the remaining points:

Added golden-output tests for artifact get
Added a test case for the --from argument in artifact list
Moved getArtifactClient to artifact.go to align with other commands
Split tests into artifact_list_test.go and artifact_get_test.go
Removed the earlier profile test (it used the old help-output pattern and is already covered in added test files for profile and profile/status command #6420)

Both commands now use direct RunE so injected clients are used during tests.

Let me know if you’d like any additional coverage or adjustments.

evankanderson

Sorry that you're colliding with the somewhat-broader #6420; there are a few things you'll probably want to pick up from that PR. I think that's probably the last golden-file-testing related infrastructure change at the moment.

sachin9058 · 2026-04-30T19:02:10Z

@evankanderson Thanks I’ve rebased this PR onto the latest branch and aligned it with the pattern from #6420.

removed the old RunCommand helper
removed test-specific logic that skipped evaluation
switched to cli.GetCLIClient for both artifact and profile clients
ensured full evaluation flow runs using injected mocks
kept tests using gomock + golden outputs with realistic data

All CLI tests pass locally:

Let me know if anything still needs adjustment.

sachin9058 · 2026-04-30T19:15:40Z

@evankanderson

I also updated the artifact get goldens (the failure was due to JSON formatting differences only spacing changes, not data changes).

Locally everything is passing:

go test ./cmd/cli/app/artifact
go test ./cmd/cli/...

However, CI is still failing on the artifact tests, and I’m not able to reproduce the failure locally.

Could you help point out what’s different in CI or if there’s something I might be missing (e.g., environment-specific formatting or test expectations)?

Happy to adjust once I understand the discrepancy.

evankanderson · 2026-04-30T23:51:06Z

This is also affected by #6430

sachin9058 · 2026-05-01T00:12:29Z

@evankanderson Thanks

This PR was still using JSON golden tests, so it was hitting the same protojson instability tracked in #6430. I’ve removed the JSON test cases and goldens here as well and kept table/yaml outputs.

All CLI tests are now passing locally.

- add reasoning column to CLI output - aggregate details, guidance, remediation, alerts, and output - centralize reasoning formatting helpers - improve readability with bullet formatting - remove previous explain-mode implementation

… feedback

… RPC injection - add golden-output tests for artifact get (table/json/yaml + error) - add fixtures and goldens for CLI output validation - inject RPC clients via context for test isolation - short-circuit gRPC setup when injected clients are present to avoid login/network in tests

… pattern - switch artifact get to use cli.GetCLIClient for both artifact and profile clients - remove custom injection logic and align with upstream CLI patterns - add golden-output tests for artifact get (table/json/yaml + error case) - use gomock-based clients with realistic evaluation data - update goldens to reflect current CLI formatting - regenerate profile/status goldens after rebase to match latest output

…n tests

- add table/json/yaml golden tests for - use injected RPC client to avoid login flow in CI - keep artifact list tests unchanged (already covered)

…tructure - add golden tests for artifact get - add --from test case for artifact list - move getArtifactClient to artifact.go - split tests into artifact_list_test.go and artifact_get_test.go - remove outdated profile tests to avoid duplication

…indersec#6430)

- keep table/yaml goldens - remove JSON goldens (see mindersec#6430) - update profile status table outputs to match current renderer

sachin9058 · 2026-05-01T07:01:25Z

@evankanderson Rebased onto latest upstream/main and resolved the conflicts.

removed remaining JSON golden files and test cases (per Unstable JSON output in CLI tests due to protojson formatting #6430)
kept artifact tests fixture-driven (table/yaml/error cases)
updated profile/status goldens to match current renderer
ensured tests align with the current CLI testing pattern

go test ./cmd/cli/... passes locally.

Let me know if anything else needs adjustment.

evankanderson · 2026-05-01T13:40:18Z

Changes in this file don't appear meaningful?

This adds one newline, removes one newline, and removes a useful comment about why json output is not tested.

I don't understand why this comment was resolved.

https://github.com/sachin9058/minder/blob/test/cli-standardize/cmd/cli/app/artifact/artifact_get_test.go

Ahh sorry Evan actually i forget to address this comment and resolved this comment accidently. Now i have fixed this file so that it matches exactly with the upstream.

…ction - replace getArtifactClient with cli.GetCLIClient - remove custom context injection logic - clean up unnecessary test changes

sachin9058 · 2026-05-01T14:03:53Z

@evankanderson Thanks addressed these points:

switched artifact list to use cli.GetCLIClient instead of the custom helper
removed the extra context injection logic and aligned with the existing pattern
cleaned up artifact_get_test.go to remove non-meaningful changes

All CLI tests are passing locally. Let me know if anything else needs adjustment.

sachin9058

Now i have restored the comments and all the lines now it looks good and same as upstream

sachin9058 · 2026-05-01T15:52:33Z

@evankanderson I’m not able to reproduce these lint failures locally.

Running golangci-lint run ./... shows 0 issues on my side, but CI is reporting multiple goconst/gosec/govet errors across unrelated packages.

This looks like a mismatch between local and CI lint environments (possibly different golangci-lint versions or config).

Would you like me to align with the CI setup and investigate further, or should these be handled separately?

evankanderson · 2026-05-01T15:58:11Z

-	fromFilter := viper.GetString("from")
+		return nil
+	},
+	RunE: func(cmd *cobra.Command, _ []string) error {


Converting this from a function named listCommand to an anonymous function adjusts the indentation in a way that makes the GitHub diff very hard to review. Please put this back to listCommand, in the same style as every other command.

sachin9058 requested a review from a team as a code owner April 25, 2026 08:06

evankanderson reviewed Apr 27, 2026

View reviewed changes

sachin9058 changed the title ~~test(cli): standardize CLI tests and improve validation~~ test(cli): add artifact CLI tests using mock-based golden framework Apr 28, 2026

sachin9058 requested a review from evankanderson April 28, 2026 12:15

evankanderson reviewed Apr 29, 2026

View reviewed changes

Comment thread cmd/cli/app/artifact/artifact_get.go Outdated

Comment thread cmd/cli/app/artifact/artifact_test.go

Comment thread cmd/cli/app/artifact/artifact_test.go

Comment thread cmd/cli/app/profile/profile_test.go

evankanderson mentioned this pull request Apr 29, 2026

feat(cli): improve evaluation reasoning output using existing fields #6417

Merged

sachin9058 requested a review from evankanderson April 30, 2026 08:02

evankanderson reviewed Apr 30, 2026

View reviewed changes

Comment thread cmd/cli/app/testutils/cli_helpers.go Outdated

Comment thread cmd/cli/app/artifact/artifact_get.go Outdated

Comment thread cmd/cli/app/artifact/artifact_get.go Outdated

sachin9058 force-pushed the test/cli-standardize branch from dfaa74f to 745cd1b Compare April 30, 2026 19:00

DharunMR mentioned this pull request May 1, 2026

Unstable JSON output in CLI tests due to protojson formatting #6430

Open

sachin9058 requested a review from evankanderson May 1, 2026 00:27

evankanderson reviewed May 1, 2026

View reviewed changes

sachin9058 added 8 commits May 1, 2026 12:12

refactor(cli): simplify evaluation details rendering and apply review…

46ea753

… feedback

chore: trigger CI rerun

e2fba50

refactor(cli): address review feedback and simplify details rendering

156b8f7

test(cli): standardize CLI tests and improve validation

b806f75

Standardize artifact and profile CLI tests

50167d8

sachin9058 added 9 commits May 1, 2026 12:22

test(cli): align artifact tests with CmdTestCase + golden pattern

1854d92

fix(cli): avoid GRPC wrapper in artifact list to prevent login flow i…

fbcda82

…n tests

test(cli): add golden tests for artifact get and fix CI login issue

fb333b5

- add table/json/yaml golden tests for - use injected RPC client to avoid login flow in CI - keep artifact list tests unchanged (already covered)

test(cli): refresh artifact get golden tests

0d81ca5

fix(cli): format generated artifact mock

00599e6

chore: trigger CI

2728e07

test(cli): remove JSON golden tests due to protojson instability (see m…

f6cc27a

…indersec#6430)

test(cli): update goldens after rebase and renderer changes

231a024

- keep table/yaml goldens - remove JSON goldens (see mindersec#6430) - update profile status table outputs to match current renderer

sachin9058 force-pushed the test/cli-standardize branch from 684cf15 to 231a024 Compare May 1, 2026 07:00

sachin9058 added 2 commits May 1, 2026 12:36

refactor(cli): inline artifact list logic into RunE

f503cb3

test(cli): remove remaining JSON goldens and finalize CLI test cleanup

8c4ab04

evankanderson reviewed May 1, 2026

View reviewed changes

refactor(cli): align with cli.GetCLIClient and remove custom RPC inje…

0fba74f

…ction - replace getArtifactClient with cli.GetCLIClient - remove custom context injection logic - clean up unnecessary test changes

sachin9058 requested a review from evankanderson May 1, 2026 14:07

chore(test): restore upstream comments in artifact_get_test.go

4556e51

sachin9058 commented May 1, 2026

View reviewed changes

chore: retrigger CI

9aa2a52

evankanderson reviewed May 1, 2026

View reviewed changes

Conversation

sachin9058 commented Apr 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Testing

Uh oh!

coveralls commented Apr 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

evankanderson left a comment

Choose a reason for hiding this comment

Uh oh!

sachin9058 commented Apr 28, 2026

Uh oh!

sachin9058 commented Apr 28, 2026

Uh oh!

sachin9058 commented Apr 28, 2026

Uh oh!

evankanderson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sachin9058 commented Apr 30, 2026

Uh oh!

sachin9058 commented Apr 30, 2026

Uh oh!

evankanderson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sachin9058 commented Apr 30, 2026

Uh oh!

sachin9058 commented Apr 30, 2026

Uh oh!

evankanderson commented Apr 30, 2026

Uh oh!

sachin9058 commented May 1, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sachin9058 commented May 1, 2026

Uh oh!

evankanderson May 1, 2026

Choose a reason for hiding this comment

Uh oh!

evankanderson May 1, 2026

Choose a reason for hiding this comment

Uh oh!

sachin9058 May 1, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sachin9058 commented May 1, 2026

Uh oh!

sachin9058 left a comment

Choose a reason for hiding this comment

Uh oh!

sachin9058 commented May 1, 2026

Uh oh!

evankanderson May 1, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sachin9058 commented Apr 25, 2026 •

edited

Loading

coveralls commented Apr 25, 2026 •

edited

Loading