feat: implement mindev test cli command and rule loading by name by krrish175-byte · Pull Request #6551 · mindersec/minder

krrish175-byte · 2026-06-27T11:51:30Z

Summary

This PR implements the CLI Scaffolding for the mindev Rule Testing Framework, wiring up the underlying pkg/ruletest engine into an accessible CLI command and making the framework fully CI/CD ready.

Addressing deferred tech debt from Phase 2, this PR also introduces "Rule Loading by Name", eliminating the need to hardcode file paths in Starlark scripts by dynamically pre-loading all YAML rule definitions from the target directory into an in-memory registry.

Key Features:

New mindev test CLI Command: Added a test subcommand to mindev. Developers can now execute their Starlark test files across entire directories by running mindev test [directories...].
Rule Loading by Name: The eval() builtin has been updated to seamlessly look up and load rules directly by name from the pre-loaded YAML registry.
Graceful Fallback: If eval() cannot find the specified rule by name in the registry, it safely falls back to resolving it as a file path, ensuring backward compatibility.
CI/CD Readiness: The CLI aggregates the results of all Starlark tests executed. If any test fails, mindev test exits with a non-zero exit code (1), making it perfectly suited for automated CI environments.
Part of the rule testing framework implementation.

Fixes #6507

Testing

Built the developer tooling locally (make build) and verified the new test cobra command registers properly.
Ran the newly created command against the sample test data (./mindev test ./pkg/ruletest/testdata).
Verified that the CLI correctly parses and aggregates all test execution results.
Ensured that intentionally failing tests properly trigger the 1 exit code and bubble up the one or more tests failed error for CI environments.

coveralls · 2026-06-27T18:06:37Z

coverage: 60.832% (+0.1%) from 60.724% — krrish175-byte:feat/cli-scaffolding into mindersec:main

evankanderson

You've added a bunch of code in pkg/ruletest -- can we restructure the existing rule execution tests to cover that code as well?

evankanderson · 2026-06-27T22:33:24Z

+			runner := ruletest.NewRunner()
+			hasFailures := false
+
+			for _, dir := range args {


Can we accept files or directories?

Also, this feels like it might be a reasonable function in runner.go (where you could test it more easily).

evankanderson · 2026-06-27T22:34:08Z

+			hasFailures := false
+
+			for _, dir := range args {
+				fmt.Printf("Running tests in %s...\n", dir)


use cmd.Printf() so that we can override the cmd.Output and get this output from the command.

evankanderson · 2026-06-27T22:34:36Z

+				if err != nil {
+					return fmt.Errorf("error running tests in %s: %w", dir, err)
+				}


Not sure -- do we want to exit-early here, or collect errors from remaining directories?

evankanderson · 2026-06-27T23:53:19Z

 func (r *Runner) RunFile(filename string, src any) ([]TestResult, error) {
+	return r.runFileWithRules(filename, src, nil)
+}


Why not just add the ruleTypes argument to RunFile?

evankanderson · 2026-06-29T15:47:02Z

+			return nil, fmt.Errorf("error opening file: %s", yf)
+		}
+		rt, err := fileconvert.ReadResourceTyped[*minderv1.RuleType](decoder)
+		closer.Close()


I think it's okay to not handle errors from Close in this case, but the linter does not, so you'll need to do something to make it okay with that.

Also, I'd prefer the defer Close() pattern, just in case extra logic or returns get added in later.

(Yes, we could be deferring a bunch of filehandle closes, but those should be fairly cheap)

evankanderson · 2026-06-29T15:48:44Z


-func (r *Runner) runOneTest(name string, fn *starlark.Function, fileSystem fs.FS) TestResult {
-	tr := r.newTestCaseRunner(name, fileSystem)
+func (r *Runner) runOneTest(name string, fn *starlark.Function, fileSystem fs.FS, ruleTypes map[string]*minderv1.RuleType) TestResult {


The linter wants you to fold the line at 130 characters.

evankanderson · 2026-06-29T15:49:56Z

+
+	var allResults []TestResult
+	for _, file := range files {
+		results, err := r.runFileWithRules(file, nil, ruleTypes)


Rather than carrying around ruleTypes, I'd look to see if there's an object it might naturally live with that could encompass "test files in a directory".

evankanderson · 2026-06-29T15:50:33Z

+	ruleTypes, err := loadRulesFromDir(dir)
+	if err != nil {
+		t.Fatalf("loading rules: %v", err)
+	}
+
 	files, err := DiscoverFiles(dir)


This looks like RunDir. Why different?

Actually, why not simply call RunPaths, and then assert on the errors and results?

krrish175-byte · 2026-06-29T17:20:00Z

Key Updates:

Added mindev test CLI command: Implemented a new test subcommand in mindev (via cmd/dev/app/test) that accepts multiple file or directory paths.
Enhanced Test Runner (pkg/ruletest/runner.go):
- Added the RunPaths function to allow executing tests across a list of directories and files seamlessly.
- Implemented automatic rule loading (loadRulesFromDir). The test runner now dynamically loads rule definitions alongside the tests, resolving panics that occurred when tests referenced rule names that were missing from the context.
CLI Output Formatting: mindev test now aggregates test results and outputs a clean, structured list of PASS/FAIL results with nested failure messages for easy debugging.
Test Coverage: Updated runner_test.go to maintain full test coverage and explicitly test execution behavior without panicking on uninitialized framework pointers.

(Note: The filesystem mocking features are staged locally and will be pushed as a separate commit once this foundational work is settled.)

evankanderson

A few comments -- I feel like your structure is close to optimal, but you can probably remove some near-duplicate code and get better test coverage with a few more tweaks.

evankanderson · 2026-06-29T17:44:50Z

+	minderv1 "github.com/mindersec/minder/pkg/api/protobuf/go/minder/v1"
+	"github.com/mindersec/minder/pkg/fileconvert"
 	"go.starlark.net/starlark"
 	"go.starlark.net/starlarktest"
 	"go.starlark.net/syntax"


GCI wants the minder imports after the other third-party imports, in a separate section.

Suggested change

minderv1 "github.com/mindersec/minder/pkg/api/protobuf/go/minder/v1"

"github.com/mindersec/minder/pkg/fileconvert"

"go.starlark.net/starlark"

"go.starlark.net/starlarktest"

"go.starlark.net/syntax"

"go.starlark.net/starlark"

"go.starlark.net/starlarktest"

"go.starlark.net/syntax"

minderv1 "github.com/mindersec/minder/pkg/api/protobuf/go/minder/v1"

"github.com/mindersec/minder/pkg/fileconvert"

evankanderson · 2026-06-29T17:50:30Z

 	globals, err := tr.runFile(filename, src)
 	if err != nil {
-		if evalErr, ok := errors.AsType[*starlark.EvalError](err); ok {
+		if evalErr, ok := err.(*starlark.EvalError); ok {


Why the change from errors.AsType?

evankanderson · 2026-06-29T17:56:10Z

-// directory, reporting results through t.
-func (r *Runner) RunDir(t *testing.T, dir string) {
+// directory. It also discovers and loads any *.yaml rule files in the directory.
+func (r *Runner) RunDir(dir string) ([]TestResult, error) {


Not sure if you want to make this function private or roll it into RunPaths

evankanderson · 2026-06-29T17:57:21Z

+	ruleTypes, err := loadRulesFromDir(dir)
+	if err != nil {
+		t.Fatalf("loading rules: %v", err)
+	}
+
 	files, err := DiscoverFiles(dir)


Actually, why not simply call RunPaths, and then assert on the errors and results?

evankanderson · 2026-06-29T18:00:07Z

+func TestTestDir(t *testing.T) {
+	t.Parallel()
+	r := NewRunner()
+	dir := t.TempDir()
+	content := []byte(`
+def test_pass():
+    assert.eq(1, 1)
+`)
+	if err := os.WriteFile(filepath.Join(dir, "pass.star"), content, 0644); err != nil {
+		t.Fatal(err)
+	}
+	r.TestDir(t, dir)
+}


What would it take to change all the above tests that use RunFile to have a single invocation of TestDir (or RunPaths) and then have specific tests which check for specific results?

I suspect that results might want to become something more sophisticated than a string (e.g. a file name + test name + output), but I think you might find that gives you benefits when it comes to creating e.g. XML output.

Still hoping that this becomes:

r := NewRunner() r.TestDir(t, "testdata")

And that we can get good coverage from just that and the file contents in testdata.

(We might even be able to delete some of the other tests!)

- Fix GCI import ordering in runner.go (minder imports in separate section) - Restore errors.AsType in RunFile for proper error-wrapping support - Make RunDir private; TestDir now delegates to RunPaths to remove duplication - Rename ruleNameOrPath to ruleName in builtinEval for clarity - Update mindev test Use field to accept files or directories

evankanderson

Sorry for the delay; I got sniped by #6558 and several life things (like getting a bike repaired).

This is looking better, but I'm going to push for even more integration testing in the pkg/ruletest to make sure we don't break the CLI by accident.

evankanderson · 2026-07-01T04:28:11Z

+	}
+
+	rulePath := ruleName
+	if !filepath.IsAbs(rulePath) {


Why this check? What would happen if the rulename started with / on Unix?

evankanderson · 2026-07-01T04:30:48Z

+	rulePath := ruleName
+	if !filepath.IsAbs(rulePath) {
+		callerFrame := thread.CallFrame(1)
+		if callerFile := callerFrame.Pos.Filename(); callerFile != "" {
+			rulePath = filepath.Join(filepath.Dir(callerFile), rulePath)
+		}
+	}
+
+	decoder, closer := fileconvert.DecoderForFile(rulePath)
+	if decoder == nil {
+		return nil, fmt.Errorf("rule %q not found in loaded rule types and no file found at path %s", ruleName, rulePath)
+	}
+	defer closer.Close()
+
+	rt, err := fileconvert.ReadResourceTyped[*minderv1.RuleType](decoder)
+	if err != nil {
+		return nil, fmt.Errorf("failed to parse rule type: %w", err)
+	}
+
+	return rt, nil


I'm worried about this code being hit unexpectedly, and things "working" when the primary path is broken.

Alternatively, I'm worried that we have no coverage for this path, and that it's code that seems like it does something but doesn't.

Is there a good reason to keep this code vs removing it and updating the test cases?

evankanderson · 2026-07-01T04:33:15Z

+// RunFile executes a single Starlark test file. If src is non-nil, it is
+// used as the file contents.


The old comment was better. (It explained the testing action more clearly, and better explained the values that src can take.)

evankanderson · 2026-07-01T04:36:55Z

+// into a map of RuleTypes keyed by rule name.
+func loadRulesFromDir(dir string) (map[string]*minderv1.RuleType, error) {
+	ruleTypes := make(map[string]*minderv1.RuleType)
+	yamlFiles, err := filepath.Glob(filepath.Join(dir, "*.yaml"))


#6558 will mean you'll need to expand this a little bit. Feel free to adjust e.g. ResourcesFromPaths to have a non-recursive version, or a recursion boolean (actually, it looks like it is ExpandFileArgs that would need adjustment).

evankanderson · 2026-07-01T04:47:37Z

+func TestTestDir(t *testing.T) {
+	t.Parallel()
+	r := NewRunner()
+	dir := t.TempDir()
+	content := []byte(`
+def test_pass():
+    assert.eq(1, 1)
+`)
+	if err := os.WriteFile(filepath.Join(dir, "pass.star"), content, 0644); err != nil {
+		t.Fatal(err)
+	}
+	r.TestDir(t, dir)
+}


(We might even be able to delete some of the other tests!)

evankanderson · 2026-07-01T04:57:13Z

+	return func(tk *TestKit) {
+		tk.mockFS = fs
+	}
+}


Can you adjust testkit as follows (rather than having separate paths for the WithGitDir and this? Also, I think "WithGitFiles" might be a better name here, to parallel the "Dir" option -- I don't feel strongly, but both are providing a mock FS?

The way I'd do this would be:

rename tk.gitDir to tk.gitFS, and change the type to billy.Filesystem

WithGitDir would more-eagerly create the osfs.New(dir)

This method would create a memfs, populate it, and assign it to tk.gitFS. Note that we only need to return a billy.Filesystem here, we don't need a full Git storage by using the Ingester interface.

evankanderson · 2026-07-01T04:58:03Z

+			for _, res := range results {
+				if len(res.Failures) > 0 {
+					hasFailures = true
+					cmd.Printf("FAIL: %s\n", res.Name)


I think you'll want the filename here as well as the test name, particularly if the same test name could occur in multiple *.star files.

evankanderson · 2026-07-01T04:59:49Z

+
+			if len(results) == 0 {
+				cmd.Printf("No tests found\n")
+				return err


err is nil here, so you could just return nil.

evankanderson · 2026-07-01T05:00:39Z

+				return fmt.Errorf("one or more tests failed")
+			}
+
+			return err


err is also non-nil here (was last assigned on 28, then checked on 29)

- Remove t.Parallel() from TestDir to prevent panic when caller already called it - Add Filename field to TestResult; TestDir groups sub-tests by file/name - Restore more descriptive RunFile doc comment explaining src parameter - Remove path fallback from rule lookup: eval() now only accepts rule names, not file paths, resolving the dual-typed argument complexity - Update testdata/eval.star to reference rule by name (branch_protection_reviews) instead of file path (rule_type_sample.yaml) - Load ruleTypes in TestRunEvalFile so name-based lookup works in tests - Remove nil guard from mapToProto: empty entity maps now correctly flow through to the engine instead of silently returning nil

- Addressed Evan's feedback on testkit mocking (refactored to gitFS/WithGitFiles) - Fixed 'err' return values in cmd/dev/app/test/test.go - Added filename prefix to CLI test output - Fixed defer Close() pattern in ruletest/runner.go - Updated RunFile godoc

- Add test_fail_ prefix support to TestDir - Simplify runner_test.go to use TestDir - Rename intentionally failing tests in sample.star to use test_fail_ prefix

evankanderson

There are a few lint errors hanging around, but this is looking close.

I'd still like to see runner_test.go call r.TestDir(t, "testdata") if that works -- it feels like it would make adding new test cases really simple.

evankanderson · 2026-07-02T13:20:13Z

+		if ruleType, ok := tr.ruleTypes[ruleName]; ok {
+			return ruleType, nil
+		}


Given that you're storing *minderv1.RuleType, you don't need the ,ok = formulation, since a nil pointer could just be the error case below:

Suggested change

if ruleType, ok := tr.ruleTypes[ruleName]; ok {

return ruleType, nil

}

if ruleType := tr.ruleTypes[ruleName]; ruleType != nil {

return ruleType, nil

}

evankanderson · 2026-07-02T13:21:59Z

 		},
+		{
+			name: "invalid mock_fs type",
+			args: starlark.Tuple{starlark.String("rule.yaml")},


(I know we don't really use this here, but this should be "fs_check", right?)

evankanderson · 2026-07-02T13:29:56Z

+// Clone Implements the Git trait. If gitFS is set, it initializes an
+// in-memory Git repository backed by that filesystem.
+func (tk *TestKit) Clone(_ context.Context, _ string, _ string) (*git.Repository, error) {
+	if tk.gitFS == nil {
+		return nil, ErrNotIngesterOverridden
+	}
+
+	storer := memory.NewStorage()
+
+	repo, err := git.Init(storer, tk.gitFS)
+	if err != nil {
+		return nil, err
+	}
+
+	w, err := repo.Worktree()
+	if err != nil {
+		return nil, err
+	}
+
+	// Add all files from the worktree
+	status, err := w.Status()
+	if err != nil {
+		return nil, err
+	}
+	for path := range status {
+		if _, err := w.Add(path); err != nil {
+			continue
+		}
+	}
+
+	_, err = w.Commit("Initial commit", &git.CommitOptions{
+		Author: &object.Signature{
+			Name:  "TestKit",
+			Email: "testkit@minder.test",
+			When:  time.Now(),
+		},
+	})
+	if err != nil {
+		return nil, err
+	}
+
+	return repo, nil


Do we still need this? I don't think we do.

evankanderson

Looks good except for the lint errors. I'm particularly pleased at how runner_test.go was able to be simplified but still have good coverage.

evankanderson · 2026-07-02T20:50:33Z

-		if !d.IsDir() && strings.HasSuffix(d.Name(), ".star") {
-			files = append(files, path)
+		if rt != nil && rt.Name != "" {
+			ruleTypes[rt.Name] = rt


(for a subsequent PR)

We should return an error if ruletypes[rt.Name] already exists.

evankanderson · 2026-07-02T20:54:07Z

+	var dirs []string
+	for dir := range filesByDir {
+		dirs = append(dirs, dir)
 	}
+	sort.Strings(dirs)


This looks like a use-case for the Go generic map functions introduced a year or so ago:

Suggested change

var dirs []string

for dir := range filesByDir {

dirs = append(dirs, dir)

}

sort.Strings(dirs)

dirs = slices.Sorted(maps.Keys(filesByDir))

evankanderson · 2026-07-02T20:56:06Z

+// TestDir discovers and executes all *.star test files under the given
+// directory, reporting results through t. It also loads *.yaml rules.
+//nolint:tparallel // TestDir acts as a test runner helper, caller is responsible for t.Parallel
+func (r *Runner) TestDir(t *testing.T, dir string) {


The linter doesn't like the //nolint: directive right after the other docs. You might just move this into runner_test, not export it, or export with a name that doesn't start with "Test*"

evankanderson · 2026-07-02T20:56:43Z

This diff is beautiful, IMO.

evankanderson · 2026-07-02T20:57:30Z

+		result := result
+		name := result.Filename + "/" + result.Name
+		t.Run(name, func(t *testing.T) {
+			if strings.HasPrefix(result.Name, "test_fail_") {


GCI wants this anonymous function to call t.Parallel() after calling t.Run() to produce a subtest.

krrish175-byte requested a review from a team as a code owner June 27, 2026 11:51

krrish175-byte force-pushed the feat/cli-scaffolding branch from cd66b53 to cde4c28 Compare June 27, 2026 17:54

krrish175-byte force-pushed the feat/cli-scaffolding branch from 011fa89 to cde4c28 Compare June 28, 2026 08:59

evankanderson reviewed Jun 29, 2026

View reviewed changes

feat: implement mindev test cli command and rule loading by name

8f49124

krrish175-byte force-pushed the feat/cli-scaffolding branch from cde4c28 to 8f49124 Compare June 29, 2026 16:41

Merge branch 'main' into feat/cli-scaffolding

2c9d69f

evankanderson reviewed Jun 29, 2026

View reviewed changes

krrish175-byte force-pushed the feat/cli-scaffolding branch 2 times, most recently from 8242a0e to eefe8c0 Compare June 29, 2026 19:55

krrish175-byte force-pushed the feat/cli-scaffolding branch from eefe8c0 to 0b195db Compare June 30, 2026 04:51

evankanderson reviewed Jul 1, 2026

View reviewed changes

krrish175-byte added 4 commits July 1, 2026 12:03

Merge branch 'main' into feat/cli-scaffolding

1671dbe

Address remaining PR comments from Evan

873de46

- Add test_fail_ prefix support to TestDir - Simplify runner_test.go to use TestDir - Rename intentionally failing tests in sample.star to use test_fail_ prefix

evankanderson reviewed Jul 2, 2026

View reviewed changes

feat: integrate filesystem mocking in ruletest eval

ff1ceff

evankanderson reviewed Jul 2, 2026

View reviewed changes

krrish175-byte added 2 commits July 3, 2026 13:24

chore: fix PR comments from latest review

1991991

chore: fix golangci-lint errors (gci and revive)

f7beb5a

evankanderson approved these changes Jul 3, 2026

View reviewed changes

evankanderson merged commit 860fb8f into mindersec:main Jul 3, 2026
34 of 35 checks passed

krrish175-byte deleted the feat/cli-scaffolding branch July 3, 2026 19:49

		// RunFile executes a single Starlark test file. If src is non-nil, it is
		// used as the file contents.

Uh oh!

Conversation

krrish175-byte commented Jun 27, 2026

Summary

Testing

Uh oh!

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

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!

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!

krrish175-byte commented Jun 29, 2026

Uh oh!

evankanderson left a comment

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!

evankanderson left a comment

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!

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!

evankanderson left a comment

Choose a reason for hiding this comment

Uh oh!

coveralls commented Jun 27, 2026 •

edited

Loading