-
Notifications
You must be signed in to change notification settings - Fork 110
test(cli): support JSON stream decoding in golden tests to fix protojson instability #6433
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
c77eef1
7d59843
9799a9e
5327f7d
3b3d185
0ed1af1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,14 +4,12 @@ | |
| package artifact | ||
|
|
||
| import ( | ||
| "context" | ||
| "fmt" | ||
| "strings" | ||
| "time" | ||
|
|
||
| "github.com/spf13/cobra" | ||
| "github.com/spf13/viper" | ||
| "google.golang.org/grpc" | ||
|
|
||
| "github.com/mindersec/minder/cmd/cli/app" | ||
| "github.com/mindersec/minder/internal/util" | ||
|
|
@@ -25,12 +23,22 @@ var listCmd = &cobra.Command{ | |
| Use: "list", | ||
| Short: "List artifacts from a provider", | ||
| Long: `The artifact list subcommand will list artifacts from a provider.`, | ||
| RunE: cli.GRPCClientWrapRunE(listCommand), | ||
| RunE: listCommand, | ||
| } | ||
|
|
||
| // listCommand is the artifact list subcommand | ||
| func listCommand(ctx context.Context, cmd *cobra.Command, _ []string, conn *grpc.ClientConn) error { | ||
| client := minderv1.NewArtifactServiceClient(conn) | ||
| func listCommand(cmd *cobra.Command, _ []string) error { | ||
| if err := viper.BindPFlags(cmd.Flags()); err != nil { | ||
| return fmt.Errorf("error binding flags: %w", err) | ||
| } | ||
|
|
||
| client, cleanup, err := cli.GetCLIClient(cmd, minderv1.NewArtifactServiceClient) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| defer cleanup() | ||
|
|
||
| ctx := cmd.Context() | ||
|
|
||
| provider := viper.GetString("provider") | ||
| project := viper.GetString("project") | ||
|
|
@@ -74,17 +82,24 @@ func listCommand(ctx context.Context, cmd *cobra.Command, _ []string, conn *grpc | |
| } | ||
| t.Render() | ||
| case app.JSON: | ||
| out, err := util.GetJsonFromProto(artifactList) | ||
| if err != nil { | ||
| return cli.MessageAndError("Error getting json from proto", err) | ||
| // Emit each artifact as a separate JSON object (JSON stream) | ||
| for _, art := range artifactList.Results { | ||
| out, err := util.GetJsonFromProto(art) | ||
| if err != nil { | ||
| return cli.MessageAndError("Error getting json from proto", err) | ||
| } | ||
| if _, err := cmd.OutOrStdout().Write([]byte(out + "\n")); err != nil { | ||
| return cli.MessageAndError("Error writing json to stdout", err) | ||
| } | ||
| } | ||
| cmd.Println(out) | ||
| case app.YAML: | ||
| out, err := util.GetYamlFromProto(artifactList) | ||
| if err != nil { | ||
| return cli.MessageAndError("Error getting yaml from proto", err) | ||
| } | ||
| cmd.Println(out) | ||
| if _, err := cmd.OutOrStdout().Write([]byte(out + "\n")); err != nil { | ||
| return cli.MessageAndError("Error writing yaml to stdout", err) | ||
| } | ||
|
Comment on lines
-87
to
+102
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is basically the same thing as |
||
| } | ||
|
|
||
| return nil | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| // SPDX-FileCopyrightText: Copyright 2026 The Minder Authors | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| package artifact | ||
|
|
||
| import ( | ||
| "context" | ||
| "testing" | ||
|
|
||
| "go.uber.org/mock/gomock" | ||
|
|
||
| "github.com/mindersec/minder/internal/util/cli" | ||
| minderv1 "github.com/mindersec/minder/pkg/api/protobuf/go/minder/v1" | ||
| mockv1 "github.com/mindersec/minder/pkg/api/protobuf/go/minder/v1/mock" | ||
| ) | ||
|
|
||
| //nolint:paralleltest // Uses global viper/Stdout state | ||
| func TestArtifactListCommand(t *testing.T) { | ||
| setupSuccess := func(t *testing.T, ctrl *gomock.Controller) context.Context { | ||
| t.Helper() | ||
|
|
||
| artifactClient := mockv1.NewMockArtifactServiceClient(ctrl) | ||
|
|
||
| listResp := &minderv1.ListArtifactsResponse{} | ||
| cli.LoadFixture(t, "mock_artifact_list.json", listResp) | ||
|
|
||
| artifactClient.EXPECT(). | ||
| ListArtifacts(gomock.Any(), gomock.Any()). | ||
| Return(listResp, nil). | ||
| Times(1) | ||
|
|
||
| ctx := cli.WithCLIClient[minderv1.ArtifactServiceClient](context.Background(), artifactClient) | ||
| return ctx | ||
| } | ||
|
|
||
| tests := []cli.CmdTestCase{ | ||
| { | ||
| Name: "list artifacts - json output", | ||
| Args: []string{"artifact", "list", "-o", "json"}, | ||
| MockSetup: setupSuccess, | ||
| GoldenFileName: "artifact_list.json", | ||
| }, | ||
| } | ||
|
|
||
| cli.RunCmdTests(t, tests, ArtifactCmd) | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| { | ||
| "results": [ | ||
| { | ||
| "artifactPk": "1", | ||
| "name": "artifact-1", | ||
| "type": "image", | ||
| "owner": "owner-1", | ||
| "repository": "org/repo", | ||
| "visibility": "public", | ||
| "createdAt": "2024-01-02T15:04:05Z" | ||
| }, | ||
| { | ||
| "artifactPk": "2", | ||
| "name": "artifact-2", | ||
| "type": "image", | ||
| "owner": "owner-2", | ||
| "repository": "org/repo2", | ||
| "visibility": "private", | ||
| "createdAt": "2024-02-02T15:04:05Z" | ||
| } | ||
| ] | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| {"artifactPk":"1","name":"artifact-1","type":"image","owner":"owner-1","repository":"org/repo","visibility":"public","createdAt":"2024-01-02T15:04:05Z"} | ||
| {"artifactPk":"2","name":"artifact-2","type":"image","owner":"owner-2","repository":"org/repo2","visibility":"private","createdAt":"2024-02-02T15:04:05Z"} |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,6 +22,12 @@ func WithRPCClient[T any](ctx context.Context, client T) context.Context { | |
| return context.WithValue(ctx, key, client) | ||
| } | ||
|
|
||
| // WithCLIClient is an alias for WithRPCClient kept for backwards/alternate usage | ||
| // by tests or callers expecting a "CLI"-named helper. | ||
| func WithCLIClient[T any](ctx context.Context, client T) context.Context { | ||
| return WithRPCClient[T](ctx, client) | ||
| } | ||
|
|
||
|
Comment on lines
+25
to
+30
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand why we wouldn't just switch over to one consistent name here -- this is in |
||
| // GetRPCClient extracts the generic RPC client from the provided context. | ||
| func GetRPCClient[T any](ctx context.Context) (T, bool) { | ||
| key := rpcKey{clientType: reflect.TypeOf((*T)(nil)).Elem()} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,8 +8,10 @@ import ( | |
| "context" | ||
| "encoding/json" | ||
| "flag" | ||
| "io" | ||
| "os" | ||
| "path/filepath" | ||
| "strings" | ||
| "testing" | ||
|
|
||
| "github.com/spf13/cobra" | ||
|
|
@@ -122,13 +124,54 @@ func checkGoldenFile(t *testing.T, filename string, actual string) { | |
| expected, err := os.ReadFile(goldenPath) | ||
| require.NoError(t, err, "could not read golden file. Run 'go test ./... -update' to generate it") | ||
|
|
||
| // Try standard JSON comparison first | ||
| if json.Valid(expected) && json.Valid([]byte(actual)) { | ||
| // if it's valid json compare the objects (ignores spaces/newlines) | ||
| require.JSONEq(t, string(expected), actual, "JSON Output does not match golden file") | ||
| } else { | ||
| // if it's a table, txt, or yaml fallback to exact string matching | ||
| require.Equal(t, string(expected), actual, "Output does not match golden file") | ||
| return | ||
| } | ||
|
Comment on lines
+127
to
131
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm concerned that using Taking the output from protojson and running it through the standard Go
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's less efficient, but we're talking about some in-memory operations on strings <10KB vs filesystem output, so the extra parsing is probably in the noise. |
||
|
|
||
| // Try to handle JSON streams (newline-delimited JSON) | ||
| // Attempt to decode newline-delimited JSON (JSON stream). | ||
| // If decoding fails, input is treated as non-JSON output. | ||
| expectedParsed, expectedErr := tryParseJSONStream(string(expected)) | ||
| actualParsed, actualErr := tryParseJSONStream(actual) | ||
|
|
||
| // If both successfully parsed as JSON streams, compare them. | ||
| // This includes edge cases like empty arrays [] which are valid JSON. | ||
| if expectedErr == nil && actualErr == nil { | ||
| require.Equal(t, expectedParsed, actualParsed, "JSON stream output does not match golden file") | ||
| return | ||
| } | ||
|
|
||
| // Otherwise do exact string matching (for tables, txt, yaml, etc.) | ||
| require.Equal(t, string(expected), actual, "Output does not match golden file") | ||
| } | ||
|
|
||
| // tryParseJSONStream attempts to parse newline-delimited JSON (JSON stream output from CLI). | ||
| // Returns empty slice and an error if input is not a JSON stream (e.g., YAML, table, etc). | ||
| func tryParseJSONStream(input string) ([]interface{}, error) { | ||
| // Try to decode as JSON stream directly without heuristics. | ||
| // If the input is not JSON, the decoder will fail on the first non-JSON line. | ||
| dec := json.NewDecoder(strings.NewReader(input)) | ||
|
|
||
| var result []interface{} | ||
|
|
||
| for { | ||
| var obj interface{} | ||
| err := dec.Decode(&obj) | ||
|
|
||
| if err == io.EOF { | ||
| break | ||
| } | ||
| if err != nil { | ||
| // Not a JSON stream (could be YAML, table, plain text, etc.) | ||
| return nil, err | ||
| } | ||
|
|
||
| result = append(result, obj) | ||
| } | ||
|
|
||
| return result, nil | ||
| } | ||
|
|
||
| // LoadFixture reads a JSON file from the "fixture" directory and unmarshals | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,73 @@ | ||
| // SPDX-FileCopyrightText: Copyright 2026 The Minder Authors | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| package cli | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| // TestTryParseJSONStream_Stream tests parsing of newline-delimited JSON (the main use case). | ||
| func TestTryParseJSONStream_Stream(t *testing.T) { | ||
| t.Parallel() | ||
| input := `{"id":1} | ||
| {"id":2}` | ||
|
|
||
| result, err := tryParseJSONStream(input) | ||
| require.NoError(t, err) | ||
| require.Len(t, result, 2) | ||
|
|
||
| // Verify actual content was parsed correctly | ||
| obj1 := result[0].(map[string]interface{}) | ||
| require.Equal(t, float64(1), obj1["id"]) | ||
|
|
||
| obj2 := result[1].(map[string]interface{}) | ||
| require.Equal(t, float64(2), obj2["id"]) | ||
| } | ||
|
|
||
| // TestTryParseJSONStream_SingleObject tests parsing of a single JSON object (backward compatibility). | ||
| func TestTryParseJSONStream_SingleObject(t *testing.T) { | ||
| t.Parallel() | ||
| input := `{"id":1}` | ||
|
|
||
| result, err := tryParseJSONStream(input) | ||
| require.NoError(t, err) | ||
| require.Len(t, result, 1) | ||
|
|
||
| // Verify actual content was parsed correctly | ||
| obj := result[0].(map[string]interface{}) | ||
| require.Equal(t, float64(1), obj["id"]) | ||
| } | ||
|
|
||
| // TestTryParseJSONStream_NonJSON tests that non-JSON input (like YAML) is rejected. | ||
| func TestTryParseJSONStream_NonJSON(t *testing.T) { | ||
| t.Parallel() | ||
| input := ` | ||
| name: sachin | ||
| age: 22 | ||
| ` | ||
|
|
||
| _, err := tryParseJSONStream(input) | ||
| require.Error(t, err) | ||
| } | ||
|
|
||
| // TestTryParseJSONStream_EmptyArray tests the edge case of empty JSON array. | ||
| func TestTryParseJSONStream_EmptyArray(t *testing.T) { | ||
| t.Parallel() | ||
| input := `[]` | ||
|
|
||
| result, err := tryParseJSONStream(input) | ||
| require.NoError(t, err) | ||
| require.Len(t, result, 1) // decoder treats [] as one decoded value | ||
| } | ||
|
|
||
| // TestTryParseJSONStream_InvalidJSON tests that malformed JSON is properly rejected. | ||
| func TestTryParseJSONStream_InvalidJSON(t *testing.T) { | ||
| t.Parallel() | ||
| input := `{"id":1` | ||
|
|
||
| _, err := tryParseJSONStream(input) | ||
| require.Error(t, err) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine for the output to be JSON-LD; it also seems fine to define that we'll only emit a single JSON element.
In either case, it would be great to record CLI patterns somewhere -- maybe in
DEVELOPMENT.md? Right now, we're not totally consistent about any of the following:--fileIt would be great to have a "design philosophy" around these that people could reference when making changes, such that the CLI starts to feel more consistently and thoughtfully designed.
I don't think it needs to happen in this PR, but it would be useful to start making some of these decisions explicit.