Implement Isolated, Deterministic Test Suite for ruletype CLI Subcommands by DharunMR · Pull Request #6286 · mindersec/minder

DharunMR · 2026-04-06T08:51:38Z

Summary

This PR introduces a robust, table driven testing architecture for the ruletype CLI command lifecycle (create, apply, get, list, delete). Historically, CLI commands can be brittle to test due to tightly coupled network calls and unpredictable standard output. This implementation establishes a new pilot pattern for CLI testing in Minder by enforcing strict network boundaries (via gRPC mocking) and deterministic output verification (via Golden Files and I/O interception).

Dependency Injection & Mocking Strategy

To isolate the CLI logic from the actual Minder control plane, this test suite leverages gomock to stub the RuleTypeServiceClient.

Global Variable Overriding: The CLI commands rely on a package-level variable (getRuleTypeClient) to instantiate the gRPC client. During testing, we intercept this by overriding the variable with our generated mockv1.MockRuleTypeServiceClient
State Restoration: To prevent test pollution, t.Cleanup() is utilized to restore the original getRuleTypeClient function pointer after every subtest execution.
Concurrency Trade-off (//nolint:paralleltest): Because we are mutating a shared package level variable to inject the mock, running these tests with t.Parallel() would introduce race conditions and non-deterministic panics. Test functions are intentionally run serially and explicitly documented with the linter suppression comment.

Deterministic Output Verification (Golden Files)

Testing CLI tools requires validating not just the exit codes, but the exact string formatting (Tables, JSON, YAML) presented to the user.

I/O Interception: We use a combination of bytes.Buffer (attached to cmd.SetOut and cmd.SetErr) and os.Pipe() to cleanly capture os.Stdout. This ensures that underlying libraries (like table writers) that bypass cobra's standard buffers are still perfectly captured.
Golden File Assertions: The captured output is asserted against pre-verified .golden files stored in testdata/. This makes future UI/UX regressions immediately obvious during CI. (To update these files locally, run go test ./... -update).

Testing

Unit Tests: Executed go test -v ./cmd/cli/app/ruletype (100% pass rate on new logic).
Linting: Executed make lint confirming zero regressions or stylistic violations.

Proposed Future Roadmap & Refactoring

If this isolated mock driven testing approach aligns with the maintainer's vision for the CLI, I propose the following follow-up work:

Horizontal Expansion: Propagate this exact table driven structure to untested commands (e.g., profiles, repos, entities, etc).
Centralized Testkit: Extract the generic boilerplate specifically checkGoldenFile, loadFixture and the os.Pipe() stdout capturing logic out of the ruletype package and into a shared internal/cli/testutils or pkg/testkit package to heavily reduce code duplication across the CLI test suites.
CI/CD Integration (GitHub Actions): Implement a dedicated GitHub Actions workflow triggered specifically on path changes within cmd/cli/**. This will automatically run the isolated CLI test suite and verify golden files on all future pull requests, preventing output regressions and enforcing this new testing standard automatically.

evankanderson

This is very cool! A few comments -- I'd like to iterate on this a little bit, but I think you picked about the right "size" of example to catch interesting cases.

This seems a bit like WireMock, but I'm not sure pulling in that dependency is worthwhile.

cmd/cli/app/ruletype/ruletype_apply_test.go

evankanderson · 2026-04-07T03:04:33Z

cmd/cli/app/ruletype/ruletype_list_test.go

+	assert.Equal(t, string(expected), actual, "Output does not match golden file")
+}
+
+//nolint:unparam // filename is currently always the same, but kept generic for architectural consistency


This comment no longer appears to be correct.

You're right, my previous comment was a bit unclear. Since make lint is still flagging this as unparam in this specific file, I've updated the comment to clarify that I'm keeping the signature generic for architectural consistency across the ruletype package's test suite

You can use "_" as the name for an unparam rather than needing a nolint directive.

Thanks for the tip about using _ The only catch is that filename is actually being used inside the function body (filepath.Join("fixture", filename)).

If I change it to the blank identifier, I'll have to hardcode "mock_ruletypes_response.json" directly inside the function. That perfectly fixes the linter for our current tests, but if we ever add a new test case that requires a different JSON fixture, we'll have to revert it back to a parameter anyway. Anyways when refracting whole test to common file will solve it

cmd/cli/app/ruletype/ruletype_delete_test.go

DharunMR · 2026-04-07T17:18:15Z

@evankanderson
Just to confirm, I didn't introduce any new test frameworks or dependencies for this setup. I built the fixture logic entirely using Go's standard library (os, path/filepath, encoding/json) layered on top of our existing testify and gomock tooling.

coveralls · 2026-04-07T17:26:53Z

coverage: 59.781% (+0.5%) from 59.314% — DharunMR:testing-ruletype-cmd into mindersec:main

evankanderson

Thinking more about the payload-in-JSON approach, it occurs to me that would probably be easier with an actual server implementation (which simply used a lookup from the file) than it would be using a gomock-generated mock, so that may be where things get complicated.

I'm fine with the current use and file pattern, FIWIW -- I just like to consider / explore alternatives when a larger architectural change comes in.

One other thought -- do we have an easy way to regenerate these golden files if needed? It might be nice to have a flag to the test or something which caused it to write the golden files for updates if needed.

evankanderson · 2026-04-08T18:23:25Z

cmd/cli/app/ruletype/ruletype_apply_test.go

+			oldStdout := os.Stdout
+			r, w, _ := os.Pipe()
+			os.Stdout = w


Why are we doing this when we're already calling cmd.SetOut?

(We should probably fix places where we're leaking output not to cmd.Out)

Good point on cmd.SetOut. I used the os.Stdout capture because there are some parts of the execution flow (specifically loggers and some underlying util functions) that bypass the Cobra output buffer and write directly to Stdout. If I only use cmd.SetOut the golden files miss that output.

I totally agree on the safety issue I'll update the test to use t.Cleanup() to ensure os.Stdout is restored correctly even if a test fails. Alternatively, if I can guarantee that the command and its dependencies only use cmd.Printf even in future, I'd prefer to remove the pipe logic entirely to keep the test clean

Can you highlight (comment) on the output that gets missed? I'd rather fix the code to use cmd.Out properly, rather than work around it in tests -- the tests are highlighting a real issue.

every rendered table is currently bypassing Cobra's buffer because table.go hardcodes fmt.Println(l.table.Render()). I noticed there's actually a TODO around line 27 in table.go to make this output configurable that should solve the leak globally?

Yes, that would be great!

evankanderson · 2026-04-08T18:24:08Z

cmd/cli/app/ruletype/ruletype_apply_test.go

+			w.Close()
+			os.Stdout = oldStdout
+			var capturedStdout bytes.Buffer
+			_, _ = io.Copy(&capturedStdout, r)
+			r.Close()


If we actually need to do this, we should use a t.Cleanup() to undo the replacement. But I'm hoping we can simply capture cmd.Out instead.

#6291 this pr fixes the leak , after merge i'll remove those os.Stdout

evankanderson · 2026-04-08T18:26:49Z

cmd/cli/app/ruletype/ruletype_create_test.go

+			viper.Reset()
+
+			createCmd.Flags().VisitAll(func(f *pflag.Flag) {
+				if slice, ok := f.Value.(pflag.SliceValue); ok {
+					_ = slice.Replace([]string{})
+				} else {
+					_ = f.Value.Set(f.DefValue)
+				}
+				f.Changed = false
+			})
+
+			ctrl := gomock.NewController(t)
+			defer ctrl.Finish()
+
+			mockClient := mockv1.NewMockRuleTypeServiceClient(ctrl)
+			tt.mockSetup(mockClient)
+
+			ctx := cli.WithRPCClient(context.Background(), mockClient)
+
+			cmd := createCmd
+			cmd.SetContext(ctx)
+
+			err := cmd.Flags().Parse(tt.args)
+			require.NoError(t, err)
+
+			_ = viper.BindPFlags(cmd.Flags())
+			viper.Set("project", zeroUUID)
+
+			buf := new(bytes.Buffer)
+			cmd.SetOut(buf)
+			cmd.SetErr(buf)
+
+			oldStdout := os.Stdout
+			r, w, _ := os.Pipe()
+			os.Stdout = w


Can we extract a lot of these into a common setup function which each test can call? There may be a few lines (e.g. creating the mock) that might need changes, but right now this looks fairly identical to ruletype_apply_test.go.

If you're building up these common functions, I'd put them in a common_test.go, so they don't look like they are part of just one test file but then are used by other tests as well.

Oh, and if there's cleanup work to be done afterwards (if we can't get rid of the Stdout capture), you can return a "cleanup" function as part of the setup, and then t.Cleanup(cleanup) right after calling the common setup.

Refractored to common file in my recent commit

evankanderson · 2026-04-08T18:31:11Z

cmd/cli/app/ruletype/ruletype_test.go

+}
+
+//nolint:unparam // keep generic signature to match helper patterns in other test files
+func loadFixture(t *testing.T, filename string, target protoreflect.ProtoMessage) {


protoreflect.ProtoMessage can be spelled proto.Message from google.golang.org/protobuf/proto. We don't do that as often as we should, but since this is a new file, I'm going to encourage it here.

evankanderson · 2026-04-08T18:44:39Z

internal/util/cli/context.go

+func WithRPCClient(ctx context.Context, client any) context.Context {
+	return context.WithValue(ctx, rpcClientKey, client)
+}


Can we store multiple RPC clients in the context using a map keyed by the proto type? Either:

Suggested change

func WithRPCClient(ctx context.Context, client any) context.Context {

return context.WithValue(ctx, rpcClientKey, client)

}

func WithRPCClient(ctx context.Context, client any) context.Context {

rpcClients := cmp.Or(ctx.Value(rpcClientKey), make(map[string]any{}))

rpcClients[reflect.TypeOf(client).String()] = client

return context.WithValue(ctx, rpcClientKey, rpcClients)

}

or

Suggested change

func WithRPCClient(ctx context.Context, client any) context.Context {

return context.WithValue(ctx, rpcClientKey, client)

}

func WithRPCClient(ctx context.Context, client any) context.Context {

return context.WithValue(ctx, reflect.TypeOf(client), client)

}

DharunMR · 2026-04-08T19:27:16Z

@evankanderson Regarding the golden files I actually already have a -update flag implemented, It’s handled inside the checkGoldenFile helper. You can run the tests with go test ./... -update to automatically create and populate goldenfiles when there is any output or api changes. We just need to mention the path to goldenfile in testcase

evankanderson · 2026-04-08T20:14:40Z

@evankanderson Regarding the golden files I actually already have a -update flag implemented, It’s handled inside the checkGoldenFile helper. You can run the tests with go test ./... -update to automatically create and populate goldenfiles when there is any output or api changes. We just need to mention the path to goldenfile in testcase

Thanks! I think I missed that with the boilerplate in the tests and elsewhere. Overall, I really like this as a way to validate and refine the output from the CLI, thank you!

Signed-off-by: DharunMR <maddharun56@gmail.com>

DharunMR requested a review from a team as a code owner April 6, 2026 08:51

DharunMR force-pushed the testing-ruletype-cmd branch 6 times, most recently from 39a2ab5 to 59733e1 Compare April 6, 2026 11:31

evankanderson reviewed Apr 7, 2026

View reviewed changes

DharunMR force-pushed the testing-ruletype-cmd branch from 59733e1 to 7b33423 Compare April 7, 2026 17:14

DharunMR requested a review from evankanderson April 8, 2026 08:34

evankanderson reviewed Apr 8, 2026

View reviewed changes

DharunMR force-pushed the testing-ruletype-cmd branch 2 times, most recently from bf6e61a to 52360b7 Compare April 10, 2026 04:46

evankanderson mentioned this pull request Apr 10, 2026

fix(cli): clarify rule type deletion errors for bundles vs profiles #6310

Open

DharunMR requested a review from evankanderson April 10, 2026 14:56

DharunMR added 11 commits April 10, 2026 21:58

added test file for ruletype list

cb3a4ea

Signed-off-by: DharunMR <maddharun56@gmail.com>

added test file for ruletype get and delete

7fac676

Signed-off-by: DharunMR <maddharun56@gmail.com>

added test file for ruletype create

d59add6

Signed-off-by: DharunMR <maddharun56@gmail.com>

added test file for ruletype apply

9b4e0f5

Signed-off-by: DharunMR <maddharun56@gmail.com>

removed unwanted code from files

56561be

Signed-off-by: DharunMR <maddharun56@gmail.com>

fixed linting

0c6a1fc

Signed-off-by: DharunMR <maddharun56@gmail.com>

resolved conflits

e02ffed

Signed-off-by: DharunMR <maddharun56@gmail.com>

chore: update ruletype golden files for formatting and releasePhase

405bed5

test: sync ruletype golden files and fix severity validation

38d0ee6

implemented a context approach and removed global variable

a982b4c

Signed-off-by: DharunMR <maddharun56@gmail.com>

refractored the test execution to common file

c895561

Signed-off-by: DharunMR <maddharun56@gmail.com>

DharunMR force-pushed the testing-ruletype-cmd branch from 52360b7 to c895561 Compare April 10, 2026 16:52

Conversation

DharunMR commented Apr 6, 2026

Summary

Dependency Injection & Mocking Strategy

Deterministic Output Verification (Golden Files)

Testing

Proposed Future Roadmap & Refactoring

Uh oh!

evankanderson left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

DharunMR commented Apr 7, 2026

Uh oh!

coveralls commented Apr 7, 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!

Choose a reason for hiding this comment

Uh oh!

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

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!

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

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!

DharunMR commented Apr 8, 2026

Uh oh!

evankanderson commented Apr 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coveralls commented Apr 7, 2026 •

edited

Loading

DharunMR Apr 8, 2026 •

edited

Loading

DharunMR Apr 8, 2026 •

edited

Loading