Add tests for swagger datasource generation by PrinceAsiedu · Pull Request #6219 · mindersec/minder

PrinceAsiedu · 2026-03-31T13:30:55Z

Summary

Add coverage for mindev datasource generate in cmd/dev/app/datasource/generate_test.go.

This exercises Swagger v2 datasource generation across the main success
paths and the expected error cases called out in #5300.

What changed

added table-driven unit tests for datasource generation
verified generated YAML by unmarshaling into the datasource proto
covered:
- single endpoint with no parameters
- path parameters
- POST/PUT body handling
- multiple endpoints
- empty/invalid Swagger documents
- unsupported header/body parameters
- duplicate/conflicting generated operation names

Follow-up fixes included

While writing the tests, a few issues in the generator surfaced and were fixed:

preserve templated Swagger paths like /users/{id} instead of emitting URL-escaped paths
return a clear error for unsupported parameter locations
return a clear error when two operations collapse to the same generated name
fix required field encoding in the generated input schema so it is accepted by structpb

Closes #5300

Add unit tests for the `mindev datasource generate` command covering Swagger v2 datasource generation and expected failures. The new tests cover: - single endpoints without parameters - path parameters - POST/PUT body handling - multiple endpoints - invalid or empty Swagger input - unsupported header/body parameters - duplicate generated operation names This also fixes a few generator issues found while adding coverage: - preserve templated path params instead of URL-escaping `{...}` - reject duplicate generated operation names - reject unsupported parameter locations with clear errors - encode required input schema fields in a structpb-compatible form Signed-off-by: Prince Oforh Asiedu <prince14asiedu@gmail.com>

Copilot

Pull request overview

Adds unit test coverage for mindev datasource generate (Swagger v2 → Minder REST datasource YAML), and updates the generator to handle several edge/error cases more explicitly.

Changes:

Added table-driven tests validating generated datasource YAML by unmarshaling into the datasource proto.
Updated path joining to preserve templated Swagger paths (e.g. /users/{id}) and added validation for unsupported parameter locations.
Added generator safeguards for duplicate generated operation names and fixed required encoding to be structpb-compatible.

Reviewed changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.

File	Description
cmd/dev/app/datasource/generate.go	Adjusts datasource generation logic (path joining, parameter validation, duplicate op-name detection, required-field encoding).
cmd/dev/app/datasource/generate_test.go	Introduces success/error-path tests for Swagger datasource generation and CLI parsing behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-31T13:35:22Z

+func testSwagger(paths map[string]spec.PathItem) *spec.Swagger {
+	return &spec.Swagger{
+		SwaggerProps: spec.SwaggerProps{
+			Swagger:  "2.0",
+			BasePath: "/api/v1",
+			Info: &spec.Info{
+				InfoProps: spec.InfoProps{
+					Title: "users-api",
+				},
+			},
+			Paths: &spec.Paths{
+				Paths: paths,
+			},
+		},
+	}


The new tests validate generated REST defs but never assert the top-level DataSource.Name, even though it’s derived from swagger.Info.Title and is part of the generated YAML contract. Adding an assertion for ds.GetName() would strengthen coverage and would also catch that swaggerTitleToDataSourceName currently replaces a fully-valid title (e.g. users-api) with "-" due to using an anchored regexp with ReplaceAllString.

coveralls · 2026-03-31T13:42:00Z

coverage: 58.306%. remained the same
when pulling f593a8a on PrinceAsiedu:test/datasource-generate-swagger
into 62b505d on mindersec:main.

Signed-off-by: Prince Oforh Asiedu <prince14asiedu@gmail.com>

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Signed-off-by: Prince Oforh Asiedu <prince14asiedu@gmail.com>

…andling in tests Signed-off-by: Prince Oforh Asiedu <prince14asiedu@gmail.com>

evankanderson

This looks very good overall! Thanks for adding the coverage (which seems to have motivated some improvements in the actual tool as well)!

evankanderson · 2026-03-31T17:07:39Z

+	return &ds, nil
+}
+
+func testSwagger(paths map[string]spec.PathItem) *spec.Swagger {


Another option here would be to create a testdata directory, and store actual sample Swagger JSON documents in the directory. I don't feel strongly about that approach, but it has the advantage of being able to add real-world examples with less Go typing boilerplate.

evankanderson · 2026-03-31T17:09:55Z

+			assert: func(t *testing.T, ds *minderv1.DataSource) {
+				t.Helper()
+				assert.Equal(t, "users-api", ds.GetName())
+				require.NotNil(t, ds.GetRest())
+				require.Len(t, ds.GetRest().GetDef(), 1)
+
+				def := ds.GetRest().GetDef()["get_users"]
+				require.NotNil(t, def)
+				assert.Equal(t, "GET", def.GetMethod())
+				assert.Equal(t, "/api/v1/users", def.GetEndpoint())
+				assert.Equal(t, "json", def.GetParse())
+				assert.Empty(t, def.GetInputSchema().AsMap())


I know we do this in a bunch of our tests, but this sort of helper function in a table test is a little bit of a "smell". In this case, you might look at using go-cmp or protocmp to allow specifying the expected result (with a side effect of getting nice-looking diffs when there's a problem).

Using prototext, this could be:

Suggested change

assert: func(t *testing.T, ds *minderv1.DataSource) {

t.Helper()

assert.Equal(t, "users-api", ds.GetName())

require.NotNil(t, ds.GetRest())

require.Len(t, ds.GetRest().GetDef(), 1)

def := ds.GetRest().GetDef()["get_users"]

require.NotNil(t, def)

assert.Equal(t, "GET", def.GetMethod())

assert.Equal(t, "/api/v1/users", def.GetEndpoint())

assert.Equal(t, "json", def.GetParse())

assert.Empty(t, def.GetInputSchema().AsMap())

expected: `

name: users-api

rest [

def : {

key: "get_users"

value: {

method: "GET"

endpoint: "/api/v1/users"

parse: "json"

}

}

]

`,

evankanderson · 2026-03-31T17:18:10Z

+				"/users/id/":  pathItem("GET", op()),
+				"/users/{id}": pathItem("GET", op(param("id", "path"))),
+			}),
+			wantErr: `duplicate generated operation name "get_users_id_"`,


❤️ wantErr as a string to test the error messages. Thanks!

evankanderson · 2026-03-31T17:23:02Z

+	if out := cmd.OutOrStdout(); out != os.Stdout {
+		if f, ok := out.(*os.File); ok {
+			prev := os.Stdout
+			os.Stdout = f
+			defer func() {
+				os.Stdout = prev
+			}()
+		}
+	}


Rather than overwriting os.Stdout, let's pass the file from cmd.OutOrStdout to the writers, and use Command.SetOut() in the tests to override Stdout. This avoids mutating global state across tests run with t.Parallel().

I think this is just adding an argument to writeDataSourceToFile

evankanderson · 2026-03-31T17:23:51Z

+func joinPaths(basepath, path string) string {
+	basepath = strings.TrimSuffix(basepath, "/")
+	path = strings.TrimPrefix(path, "/")
+	if path == "" {
+		return basepath
+	}
+
+	return basepath + "/" + path
+}


url.JoinPath should do this. Is it not?

evankanderson · 2026-03-31T17:26:40Z

+		if !slices.Contains([]string{"path", "query"}, p.In) {
+			return fmt.Errorf("unsupported parameter %q in %q", p.Name, p.In)


We could support e.g. headers, but we don't at the moment, right? It seems worth a comment to indicate whether this is a limitation of the generation tool, or a limit of the underlying datasource API.

evankanderson · 2026-03-31T17:28:20Z

+	os.Stdout = stdoutFile
+	defer func() {
+		os.Stdout = originalStdout
+	}()


Prefer to use Command.SetOut here -- all you need is an io.Writer, so this could simply be a bytes.Buffer rather than needing any actual files at all. (and then you won't need the lock!)

evankanderson · 2026-03-31T17:29:57Z

+	stdoutFile, err := os.CreateTemp(t.TempDir(), "stdout-*.yaml")
+	require.NoError(t, err)
+
+	stdoutMu.Lock()
+	defer stdoutMu.Unlock()
+
+	originalStdout := os.Stdout
+	os.Stdout = stdoutFile
+	defer func() {
+		os.Stdout = originalStdout
+	}()


Again, prefer Command.SetOut here.

evankanderson

Add a comment when this is ready for review; at a minimum, we should use Command.SetOut rather than overwriting os.Stdout, but you may want to review the other comments as well.

evankanderson

Marking as "Request changes" (for at least cmd.Stdout), so I can more easily track which PRs need maintainer attention again.

github-actions · 2026-05-30T05:02:34Z

This PR needs additional information before we can continue. It is now marked as stale because it has been open for 30 days with no activity. Please provide the necessary details to continue or it will be closed in 30 days.

evankanderson · 2026-06-04T13:43:52Z

@PrinceAsiedu -- are you likely to get back to this PR?

PrinceAsiedu requested a review from a team as a code owner March 31, 2026 13:30

Copilot AI review requested due to automatic review settings March 31, 2026 13:30

Copilot started reviewing on behalf of PrinceAsiedu March 31, 2026 13:31 View session

Copilot AI reviewed Mar 31, 2026

View reviewed changes

PrinceAsiedu and others added 4 commits March 31, 2026 13:43

test(datasource): improve formatting in swagger datasource tests

3418e92

Signed-off-by: Prince Oforh Asiedu <prince14asiedu@gmail.com>

Update cmd/dev/app/datasource/generate.go

b237068

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

fix(datasource): improve swagger title sanitization and update tests

8d5bf39

Signed-off-by: Prince Oforh Asiedu <prince14asiedu@gmail.com>

test(datasource): enhance parallel execution and simplify parameter h…

4471023

…andling in tests Signed-off-by: Prince Oforh Asiedu <prince14asiedu@gmail.com>

evankanderson reviewed Mar 31, 2026

View reviewed changes

Merge branch 'main' into test/datasource-generate-swagger

f593a8a

evankanderson reviewed Apr 3, 2026

View reviewed changes

evankanderson requested changes Apr 28, 2026

View reviewed changes

github-actions Bot added the Stale label May 30, 2026

github-actions Bot removed the Stale label Jun 5, 2026

		if !slices.Contains([]string{"path", "query"}, p.In) {
		return fmt.Errorf("unsupported parameter %q in %q", p.Name, p.In)

Conversation

PrinceAsiedu commented Mar 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

What changed

Follow-up fixes included

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

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

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!

evankanderson left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented May 30, 2026

Uh oh!

evankanderson commented Jun 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

PrinceAsiedu commented Mar 31, 2026 •

edited

Loading

coveralls commented Mar 31, 2026 •

edited

Loading