diff --git a/.ci/magician/cmd/DIFF_COMMENT.md b/.ci/magician/cmd/DIFF_COMMENT.md index 99095de696bd..02a145bc402c 100644 --- a/.ci/magician/cmd/DIFF_COMMENT.md +++ b/.ci/magician/cmd/DIFF_COMMENT.md @@ -24,7 +24,22 @@ If you believe this detection to be incorrect please raise the concern with your If you intend to make this change you will need to wait for a [major release](https://www.terraform.io/plugin/sdkv2/best-practices/versioning#example-major-number-increments) window. An `override-breaking-change` label can be added to allow merging. {{end}} -{{.MissingTests}} + +{{if gt (len .MissingTests) 0}} +## Missing test report +Your PR includes resource fields which are not covered by any test. +{{ range $resourceName, $missingTestInfo := .MissingTests }} +Resource: `{{ $resourceName }}` ({{ len $missingTestInfo.Tests }} total tests) +Please add an acceptance test which includes these fields. The test should include the following: + +```hcl +{{ $missingTestInfo.SuggestedTest }} + +``` + +{{- end }} +{{end}} + {{- $errorsLength := len .Errors}} {{- if gt $errorsLength 0}} ## Errors diff --git a/.ci/magician/cmd/generate_comment.go b/.ci/magician/cmd/generate_comment.go index a8dccbf2435a..1422e582d17f 100644 --- a/.ci/magician/cmd/generate_comment.go +++ b/.ci/magician/cmd/generate_comment.go @@ -55,6 +55,11 @@ type BreakingChange struct { DocumentationReference string } +type MissingTestInfo struct { + SuggestedTest string + Tests []string +} + type Errors struct { Title string Errors []string @@ -64,7 +69,7 @@ type diffCommentData struct { PrNumber int Diffs []Diff BreakingChanges []BreakingChange - MissingTests string + MissingTests map[string]*MissingTestInfo Errors []Errors } @@ -465,17 +470,21 @@ func changedSchemaResources(diffProcessorPath string, rnr ExecRunner) ([]string, // Run the missing test detector and return the results. // Returns an empty string unless there are missing tests. // Error will be nil unless an error occurs during setup. -func detectMissingTests(diffProcessorPath, tpgbLocalPath string, rnr ExecRunner) (string, error) { +func detectMissingTests(diffProcessorPath, tpgbLocalPath string, rnr ExecRunner) (map[string]*MissingTestInfo, error) { if err := rnr.PushDir(diffProcessorPath); err != nil { - return "", err + return nil, err } output, err := rnr.Run("bin/diff-processor", []string{"detect-missing-tests", fmt.Sprintf("%s/google-beta/services", tpgbLocalPath)}, nil) if err != nil { - return "", err + return nil, err } - return output, rnr.PopDir() + var missingTests map[string]*MissingTestInfo + if err = json.Unmarshal([]byte(output), &missingTests); err != nil { + return nil, err + } + return missingTests, rnr.PopDir() } func formatDiffComment(data diffCommentData) (string, error) { diff --git a/.ci/magician/cmd/generate_comment_test.go b/.ci/magician/cmd/generate_comment_test.go index 91180c26369b..a246dac83b17 100644 --- a/.ci/magician/cmd/generate_comment_test.go +++ b/.ci/magician/cmd/generate_comment_test.go @@ -105,7 +105,7 @@ func TestExecGenerateComment(t *testing.T) { for method, expectedCalls := range map[string][][]any{ "PostBuildStatus": {{"123456", "terraform-provider-breaking-change-test", "success", "https://console.cloud.google.com/cloud-build/builds;region=global/build1;step=17?project=project1", "sha1"}}, - "PostComment": {{"123456", "Hi there, I'm the Modular magician. I've detected the following information about your changes:\n\n## Diff report\n\nYour PR generated some diffs in downstreams - here they are.\n\n`google` provider: [Diff](https://github.com/modular-magician/terraform-provider-google/compare/auto-pr-123456-old..auto-pr-123456) ( 2 files changed, 40 insertions(+))\n`google-beta` provider: [Diff](https://github.com/modular-magician/terraform-provider-google-beta/compare/auto-pr-123456-old..auto-pr-123456) ( 2 files changed, 40 insertions(+))\n`terraform-google-conversion`: [Diff](https://github.com/modular-magician/terraform-google-conversion/compare/auto-pr-123456-old..auto-pr-123456) ( 1 file changed, 10 insertions(+))\n\n## Missing test report\nYour PR includes resource fields which are not covered by any test.\n\nResource: `google_folder_access_approval_settings` (3 total tests)\nPlease add an acceptance test which includes these fields. The test should include the following:\n\n```hcl\nresource \"google_folder_access_approval_settings\" \"primary\" {\n uncovered_field = # value needed\n}\n\n```\n"}}, + "PostComment": {{"123456", "Hi there, I'm the Modular magician. I've detected the following information about your changes:\n\n## Diff report\n\nYour PR generated some diffs in downstreams - here they are.\n\n`google` provider: [Diff](https://github.com/modular-magician/terraform-provider-google/compare/auto-pr-123456-old..auto-pr-123456) ( 2 files changed, 40 insertions(+))\n`google-beta` provider: [Diff](https://github.com/modular-magician/terraform-provider-google-beta/compare/auto-pr-123456-old..auto-pr-123456) ( 2 files changed, 40 insertions(+))\n`terraform-google-conversion`: [Diff](https://github.com/modular-magician/terraform-google-conversion/compare/auto-pr-123456-old..auto-pr-123456) ( 1 file changed, 10 insertions(+))\n\n\n\n## Missing test report\nYour PR includes resource fields which are not covered by any test.\n\nResource: `google_folder_access_approval_settings` (3 total tests)\nPlease add an acceptance test which includes these fields. The test should include the following:\n\n```hcl\nresource \"google_folder_access_approval_settings\" \"primary\" {\n uncovered_field = # value needed\n}\n\n```\n"}}, "AddLabels": {{"123456", []string{"service/alloydb"}}}, } { if actualCalls, ok := gh.calledMethods[method]; !ok { @@ -214,7 +214,12 @@ func TestFormatDiffComment(t *testing.T) { }, "missing tests are displayed": { data: diffCommentData{ - MissingTests: "## Missing test report", + MissingTests: map[string]*MissingTestInfo{ + "resource": { + Tests: []string{"test-a", "test-b"}, + SuggestedTest: "x", + }, + }, }, expectedStrings: []string{ "## Diff report", diff --git a/.ci/magician/cmd/mock_runner_test.go b/.ci/magician/cmd/mock_runner_test.go index dd06fd0f2c8a..c3a1abccbde3 100644 --- a/.ci/magician/cmd/mock_runner_test.go +++ b/.ci/magician/cmd/mock_runner_test.go @@ -71,7 +71,7 @@ func NewMockRunner() MockRunner { "/mock/dir/magic-modules/tools/diff-processor bin/diff-processor [breaking-changes] map[]": "", "/mock/dir/magic-modules/tools/diff-processor make [build] " + sortedEnvString(diffProcessorEnv): "", "/mock/dir/magic-modules/tools/diff-processor bin/diff-processor [changed-schema-resources] map[]": "[\"google_alloydb_instance\"]", - "/mock/dir/magic-modules/tools/diff-processor bin/diff-processor [detect-missing-tests /mock/dir/tpgb/google-beta/services] map[]": "## Missing test report\nYour PR includes resource fields which are not covered by any test.\n\nResource: `google_folder_access_approval_settings` (3 total tests)\nPlease add an acceptance test which includes these fields. The test should include the following:\n\n```hcl\nresource \"google_folder_access_approval_settings\" \"primary\" {\n uncovered_field = # value needed\n}\n\n```\n", + "/mock/dir/magic-modules/tools/diff-processor bin/diff-processor [detect-missing-tests /mock/dir/tpgb/google-beta/services] map[]": `{"google_folder_access_approval_settings":{"SuggestedTest":"resource \"google_folder_access_approval_settings\" \"primary\" {\n uncovered_field = # value needed\n}","Tests":["a","b","c"]}}`, "/mock/dir/tgc git [diff origin/auto-pr-123456-old origin/auto-pr-123456 --shortstat] map[]": " 1 file changed, 10 insertions(+)\n", "/mock/dir/tgc git [fetch origin auto-pr-123456-old] map[]": "", "/mock/dir/tfoics git [diff origin/auto-pr-123456-old origin/auto-pr-123456 --shortstat] map[]": "", diff --git a/tools/diff-processor/cmd/detect_missing_tests.go b/tools/diff-processor/cmd/detect_missing_tests.go index 9417f767567d..044a7943d372 100644 --- a/tools/diff-processor/cmd/detect_missing_tests.go +++ b/tools/diff-processor/cmd/detect_missing_tests.go @@ -1,13 +1,13 @@ package cmd import ( + "encoding/json" newProvider "google/provider/new/google/provider" oldProvider "google/provider/old/google/provider" + "io" "fmt" "os" - "strings" - "text/template" "github.com/GoogleCloudPlatform/magic-modules/tools/diff-processor/detector" "github.com/GoogleCloudPlatform/magic-modules/tools/diff-processor/diff" @@ -20,11 +20,13 @@ const detectMissingTestsDesc = "Run the missing test detector using the given se type detectMissingTestsOptions struct { rootOptions *rootOptions + stdout io.Writer } func newDetectMissingTestsCmd(rootOptions *rootOptions) *cobra.Command { o := &detectMissingTestsOptions{ rootOptions: rootOptions, + stdout: os.Stdout, } return &cobra.Command{ Use: "detect-missing-tests SERVICES_DIR", @@ -49,27 +51,8 @@ func (o *detectMissingTestsOptions) run(args []string) error { if err != nil { return fmt.Errorf("error detecting missing tests: %v", err) } - if len(missingTests) > 0 { - funcs := template.FuncMap{ - "join": strings.Join, - "backTickAll": func(ss []string) []string { - rs := make([]string, len(ss)) - for i, s := range ss { - rs[i] = fmt.Sprintf("`%s`", s) - } - return rs - }, - } - outputTemplate, err := template.New("missing_test_output.tmpl").Funcs(funcs).ParseFiles("missing_test_output.tmpl") - if err != nil { - return fmt.Errorf("Error parsing missing test template file: %s", err) - } - if err := outputTemplate.Execute(os.Stdout, missingTests); err != nil { - return fmt.Errorf("Error executing missing test output template: %s", err) - } - for resourceName, missingTestInfo := range missingTests { - glog.Infof("%s tests parsed: %v", resourceName, missingTestInfo.Tests) - } + if err := json.NewEncoder(o.stdout).Encode(missingTests); err != nil { + return fmt.Errorf("error encoding json: %w", err) } return nil } diff --git a/tools/diff-processor/missing_test_output.tmpl b/tools/diff-processor/missing_test_output.tmpl deleted file mode 100644 index dda934cc1117..000000000000 --- a/tools/diff-processor/missing_test_output.tmpl +++ /dev/null @@ -1,11 +0,0 @@ -## Missing test report -Your PR includes resource fields which are not covered by any test. -{{ range $resourceName, $missingTestInfo := . }} -Resource: `{{ $resourceName }}` ({{ len $missingTestInfo.Tests }} total tests) -Please add an acceptance test which includes these fields. The test should include the following: - -```hcl -{{ $missingTestInfo.SuggestedTest }} -``` - -{{- end }}