From fd6a9cf0683ea00cf4ba8e1f2979e072de7d8d62 Mon Sep 17 00:00:00 2001 From: Eric Cornelissen Date: Sun, 15 Dec 2024 12:29:55 +0100 Subject: [PATCH] Improve tests with inherently named tests Refactor nearly all tests by using `map[string]TestCase` for the list of test cases. This finishes some ongoing refactoring from recent commits. This change has several benefits, including compiler enforced unique test names, required names (can't forget to add it to the struct), simpler `TestCase` types. Additionally, this change makes it more likely to end up with more descriptive test names, which is already observed in a few cases here. Signed-off-by: Eric Cornelissen --- analyze_test.go | 147 ++++++++++++++---------------------- cmd/ades/output_test.go | 91 ++++++++-------------- matchers_test.go | 28 +++---- parse_test.go | 114 +++++++++++----------------- rules_test.go | 162 +++++++++++++++++++++------------------- 5 files changed, 231 insertions(+), 311 deletions(-) diff --git a/analyze_test.go b/analyze_test.go index 235ffb1..fd1f1f0 100644 --- a/analyze_test.go +++ b/analyze_test.go @@ -21,15 +21,13 @@ import ( func TestAnalyzeManifest(t *testing.T) { type TestCase struct { - name string manifest Manifest matcher ExprMatcher want int } - testCases := []TestCase{ - { - name: "Non-composite manifest", + testCases := map[string]TestCase{ + "Non-composite manifest": { manifest: Manifest{ Runs: ManifestRuns{ Using: "node16", @@ -44,8 +42,7 @@ func TestAnalyzeManifest(t *testing.T) { matcher: AllMatcher, want: 0, }, - { - name: "Safe manifest", + "Safe manifest": { manifest: Manifest{ Runs: ManifestRuns{ Using: "composite", @@ -60,8 +57,7 @@ func TestAnalyzeManifest(t *testing.T) { matcher: AllMatcher, want: 0, }, - { - name: "Problem in first of two steps in manifest", + "Problem in first of two steps in manifest": { manifest: Manifest{ Runs: ManifestRuns{ Using: "composite", @@ -80,8 +76,7 @@ func TestAnalyzeManifest(t *testing.T) { matcher: AllMatcher, want: 1, }, - { - name: "Problem in second of two steps in manifest", + "Problem in second of two steps in manifest": { manifest: Manifest{ Runs: ManifestRuns{ Using: "composite", @@ -100,8 +95,7 @@ func TestAnalyzeManifest(t *testing.T) { matcher: AllMatcher, want: 1, }, - { - name: "Problem in all steps in manifest", + "Problem in all steps in manifest": { manifest: Manifest{ Runs: ManifestRuns{ Using: "composite", @@ -122,8 +116,8 @@ func TestAnalyzeManifest(t *testing.T) { }, } - for _, tt := range testCases { - t.Run(tt.name, func(t *testing.T) { + for name, tt := range testCases { + t.Run(name, func(t *testing.T) { t.Parallel() violations := AnalyzeManifest(&tt.manifest, tt.matcher) @@ -149,15 +143,13 @@ func TestAnalyzeManifest(t *testing.T) { func TestAnalyzeWorkflow(t *testing.T) { type TestCase struct { - name string workflow Workflow matcher ExprMatcher want int } - testCases := []TestCase{ - { - name: "Safe workflow", + testCases := map[string]TestCase{ + "Safe workflow": { workflow: Workflow{ Jobs: map[string]WorkflowJob{ "safe": { @@ -174,8 +166,7 @@ func TestAnalyzeWorkflow(t *testing.T) { matcher: AllMatcher, want: 0, }, - { - name: "Problem in first of two jobs in workflow", + "Problem in first of two jobs in workflow": { workflow: Workflow{ Jobs: map[string]WorkflowJob{ "unsafe": { @@ -201,8 +192,7 @@ func TestAnalyzeWorkflow(t *testing.T) { matcher: AllMatcher, want: 1, }, - { - name: "Problem in second of two jobs in workflow", + "Problem in second of two jobs in workflow": { workflow: Workflow{ Jobs: map[string]WorkflowJob{ "safe": { @@ -228,8 +218,7 @@ func TestAnalyzeWorkflow(t *testing.T) { matcher: AllMatcher, want: 1, }, - { - name: "Problem in all jobs in workflow", + "Problem in all jobs in workflow": { workflow: Workflow{ Jobs: map[string]WorkflowJob{ "unsafe": { @@ -261,8 +250,8 @@ func TestAnalyzeWorkflow(t *testing.T) { }, } - for _, tt := range testCases { - t.Run(tt.name, func(t *testing.T) { + for name, tt := range testCases { + t.Run(name, func(t *testing.T) { t.Parallel() violations := AnalyzeWorkflow(&tt.workflow, tt.matcher) @@ -288,7 +277,6 @@ func TestAnalyzeWorkflow(t *testing.T) { func TestAnalyzeJob(t *testing.T) { type TestCase struct { - name string id string job WorkflowJob matcher ExprMatcher @@ -296,10 +284,9 @@ func TestAnalyzeJob(t *testing.T) { wantId string } - testCases := []TestCase{ - { - name: "Safe unnamed job", - id: "job-id", + testCases := map[string]TestCase{ + "Safe unnamed job": { + id: "job-id", job: WorkflowJob{ Name: "", Steps: []JobStep{ @@ -312,9 +299,8 @@ func TestAnalyzeJob(t *testing.T) { matcher: AllMatcher, wantCount: 0, }, - { - name: "Safe named job", - id: "job-id", + "Safe named job": { + id: "job-id", job: WorkflowJob{ Name: "Safe", Steps: []JobStep{ @@ -327,9 +313,8 @@ func TestAnalyzeJob(t *testing.T) { matcher: AllMatcher, wantCount: 0, }, - { - name: "Unnamed job with unsafe step", - id: "job-id", + "Unnamed job with unsafe step": { + id: "job-id", job: WorkflowJob{ Name: "", Steps: []JobStep{ @@ -343,9 +328,8 @@ func TestAnalyzeJob(t *testing.T) { wantCount: 1, wantId: "job-id", }, - { - name: "Named job with unsafe step", - id: "job-id", + "Named job with unsafe step": { + id: "job-id", job: WorkflowJob{ Name: "Unsafe", Steps: []JobStep{ @@ -359,9 +343,8 @@ func TestAnalyzeJob(t *testing.T) { wantCount: 1, wantId: "Unsafe", }, - { - name: "Unnamed job with unsafe and safe steps", - id: "job-id", + "Unnamed job with unsafe and safe steps": { + id: "job-id", job: WorkflowJob{ Name: "", Steps: []JobStep{ @@ -383,8 +366,7 @@ func TestAnalyzeJob(t *testing.T) { wantCount: 1, wantId: "job-id", }, - { - name: "Named job with unsafe and safe steps", + "Named job with unsafe and safe steps": { job: WorkflowJob{ Name: "Unsafe", Steps: []JobStep{ @@ -408,8 +390,8 @@ func TestAnalyzeJob(t *testing.T) { }, } - for _, tt := range testCases { - t.Run(tt.name, func(t *testing.T) { + for name, tt := range testCases { + t.Run(name, func(t *testing.T) { t.Parallel() violations := analyzeJob(tt.id, &tt.job, tt.matcher) @@ -432,7 +414,6 @@ func TestAnalyzeJob(t *testing.T) { func TestAnalyzeStep(t *testing.T) { type TestCase struct { - name string id int step JobStep matcher ExprMatcher @@ -440,25 +421,22 @@ func TestAnalyzeStep(t *testing.T) { wantStepId string } - testCases := []TestCase{ - { - name: "Unnamed step that does nothing", + testCases := map[string]TestCase{ + "Unnamed step that does nothing": { step: JobStep{ Name: "", }, matcher: AllMatcher, wantCount: 0, }, - { - name: "Named step that does nothing", + "Named step that does nothing": { step: JobStep{ Name: "Doesn't run", }, matcher: AllMatcher, wantCount: 0, }, - { - name: "Unnamed step without violation", + "Unnamed step without violation": { step: JobStep{ Name: "", Run: "echo 'Hello world!'", @@ -466,8 +444,7 @@ func TestAnalyzeStep(t *testing.T) { matcher: AllMatcher, wantCount: 0, }, - { - name: "Named step without violation", + "Named step without violation": { step: JobStep{ Name: "Run something", Run: "echo 'Hello world!'", @@ -475,9 +452,8 @@ func TestAnalyzeStep(t *testing.T) { matcher: AllMatcher, wantCount: 0, }, - { - name: "Unnamed step with one violation", - id: 42, + "Unnamed step with one violation": { + id: 42, step: JobStep{ Name: "", Run: "echo 'Hello ${{ inputs.name }}!'", @@ -486,8 +462,7 @@ func TestAnalyzeStep(t *testing.T) { wantCount: 1, wantStepId: "#42", }, - { - name: "Named step with one violation", + "Named step with one violation": { step: JobStep{ Name: "Greet person", Run: "echo 'Hello ${{ inputs.name }}!'", @@ -496,9 +471,8 @@ func TestAnalyzeStep(t *testing.T) { wantCount: 1, wantStepId: "Greet person", }, - { - name: "Unnamed step with two violation", - id: 3, + "Unnamed step with two violation": { + id: 3, step: JobStep{ Name: "", Run: "echo 'Hello ${{ inputs.name }}! How is your ${{ steps.id.outputs.day }}'", @@ -507,9 +481,8 @@ func TestAnalyzeStep(t *testing.T) { wantCount: 2, wantStepId: "#3", }, - { - name: "Named step with two violation", - id: 1, + "Named step with two violation": { + id: 1, step: JobStep{ Name: "Greet person today", Run: "echo 'Hello ${{ inputs.name }}! How is your ${{ steps.id.outputs.day }}'", @@ -518,27 +491,24 @@ func TestAnalyzeStep(t *testing.T) { wantCount: 2, wantStepId: "Greet person today", }, - { - name: "Uses step with unknown action", - id: 1, + "Uses step with unknown action": { + id: 1, step: JobStep{ Uses: "this/is-not@a-real-action", }, matcher: AllMatcher, wantCount: 0, }, - { - name: "Uses step with known action, no violations", - id: 1, + "Uses step with known action, no violations": { + id: 1, step: JobStep{ Uses: "actions/github-script@v6", }, matcher: AllMatcher, wantCount: 0, }, - { - name: "Uses step with known action, no violations", - id: 1, + "Uses step with known action, one violation": { + id: 1, step: JobStep{ Uses: "actions/github-script@v6", With: map[string]string{ @@ -551,8 +521,8 @@ func TestAnalyzeStep(t *testing.T) { }, } - for _, tt := range testCases { - t.Run(tt.name, func(t *testing.T) { + for name, tt := range testCases { + t.Run(name, func(t *testing.T) { t.Parallel() violations := analyzeStep(tt.id, &tt.step, tt.matcher) @@ -575,35 +545,30 @@ func TestAnalyzeStep(t *testing.T) { func TestAnalyzeString(t *testing.T) { type TestCase struct { - name string value string matcher ExprMatcher want []Violation } - testCases := []TestCase{ - { - name: "Simple and safe", + testCases := map[string]TestCase{ + "Simple and safe": { value: "echo 'Hello world!'", matcher: AllMatcher, want: []Violation{}, }, - { - name: "Multiline and safe", + "Multiline and safe": { value: "echo 'Hello'\necho 'world!'", matcher: AllMatcher, want: []Violation{}, }, - { - name: "One violations", + "One violations": { value: "echo 'Hello ${{ input.name }}!'", matcher: AllMatcher, want: []Violation{ {Problem: "${{ input.name }}"}, }, }, - { - name: "Two violations", + "Two violations": { value: "echo '${{ input.greeting }} ${{ input.name }}!'", matcher: AllMatcher, want: []Violation{ @@ -613,8 +578,8 @@ func TestAnalyzeString(t *testing.T) { }, } - for _, tt := range testCases { - t.Run(tt.name, func(t *testing.T) { + for name, tt := range testCases { + t.Run(name, func(t *testing.T) { t.Parallel() violations := analyzeString(tt.value, tt.matcher) diff --git a/cmd/ades/output_test.go b/cmd/ades/output_test.go index d79bc7c..fa2f25c 100644 --- a/cmd/ades/output_test.go +++ b/cmd/ades/output_test.go @@ -23,21 +23,18 @@ import ( func TestPrintJson(t *testing.T) { type TestCase struct { - name string violations func() map[string]map[string][]ades.Violation want string } - testCases := []TestCase{ - { - name: "No targets", + testCases := map[string]TestCase{ + "No targets": { violations: func() map[string]map[string][]ades.Violation { return make(map[string]map[string][]ades.Violation) }, want: `{"problems":[]}`, }, - { - name: "target without files", + "target without files": { violations: func() map[string]map[string][]ades.Violation { m := make(map[string]map[string][]ades.Violation) m["foobar"] = make(map[string][]ades.Violation) @@ -45,8 +42,7 @@ func TestPrintJson(t *testing.T) { }, want: `{"problems":[]}`, }, - { - name: "target with files without violations", + "target with files without violations": { violations: func() map[string]map[string][]ades.Violation { m := make(map[string]map[string][]ades.Violation) m["foo"] = make(map[string][]ades.Violation) @@ -55,8 +51,7 @@ func TestPrintJson(t *testing.T) { }, want: `{"problems":[]}`, }, - { - name: "target with files with violations", + "target with files with violations": { violations: func() map[string]map[string][]ades.Violation { m := make(map[string]map[string][]ades.Violation) m["foo"] = make(map[string][]ades.Violation) @@ -72,8 +67,8 @@ func TestPrintJson(t *testing.T) { }, } - for _, tt := range testCases { - t.Run(tt.name, func(t *testing.T) { + for name, tt := range testCases { + t.Run(name, func(t *testing.T) { t.Parallel() if got, want := printJson(tt.violations()), tt.want; got != want { @@ -85,40 +80,33 @@ func TestPrintJson(t *testing.T) { func TestPrintProjectViolations(t *testing.T) { type TestCase struct { - violations func() map[string][]ades.Violation + violations map[string][]ades.Violation want string } testCases := map[string]TestCase{ "No files": { - violations: func() map[string][]ades.Violation { - return make(map[string][]ades.Violation) - }, + violations: map[string][]ades.Violation{}, want: `Ok `, }, "File without violations": { - violations: func() map[string][]ades.Violation { - m := make(map[string][]ades.Violation, 1) - m["workflow.yml"] = make([]ades.Violation, 0) - return m + violations: map[string][]ades.Violation{ + "workflow.yml": {}, }, want: `Ok `, }, "Workflow with a violation": { - violations: func() map[string][]ades.Violation { - m := make(map[string][]ades.Violation, 1) - m["workflow.yml"] = []ades.Violation{ + violations: map[string][]ades.Violation{ + "workflow.yml": { { JobId: "4", StepId: "2", Problem: "${{ foo.bar }}", RuleId: "ADES100", }, - } - - return m + }, }, want: `Detected 1 violation(s) in "workflow.yml": 1 in job "4": @@ -126,9 +114,8 @@ func TestPrintProjectViolations(t *testing.T) { `, }, "Workflow with multiple violations in the same job": { - violations: func() map[string][]ades.Violation { - m := make(map[string][]ades.Violation, 1) - m["workflow.yml"] = []ades.Violation{ + violations: map[string][]ades.Violation{ + "workflow.yml": { { JobId: "3", StepId: "6", @@ -141,9 +128,7 @@ func TestPrintProjectViolations(t *testing.T) { Problem: "${{ foo.baz }}", RuleId: "ADES101", }, - } - - return m + }, }, want: `Detected 2 violation(s) in "workflow.yml": 2 in job "3": @@ -152,9 +137,8 @@ func TestPrintProjectViolations(t *testing.T) { `, }, "Workflow with multiple violations in different jobs": { - violations: func() map[string][]ades.Violation { - m := make(map[string][]ades.Violation, 1) - m["workflow.yml"] = []ades.Violation{ + violations: map[string][]ades.Violation{ + "workflow.yml": { { JobId: "4", StepId: "2", @@ -167,9 +151,7 @@ func TestPrintProjectViolations(t *testing.T) { Problem: "${{ foo.baz }}", RuleId: "ADES101", }, - } - - return m + }, }, want: `Detected 2 violation(s) in "workflow.yml": 1 in job "3": @@ -179,26 +161,22 @@ func TestPrintProjectViolations(t *testing.T) { `, }, "Manifest with a violation": { - violations: func() map[string][]ades.Violation { - m := make(map[string][]ades.Violation, 1) - m["action.yml"] = []ades.Violation{ + violations: map[string][]ades.Violation{ + "action.yml": { { StepId: "7", Problem: "${{ foo.bar }}", RuleId: "ADES100", }, - } - - return m + }, }, want: `Detected 1 violation(s) in "action.yml": step "7" contains "${{ foo.bar }}" (ADES100) `, }, "Manifest with multiple violations": { - violations: func() map[string][]ades.Violation { - m := make(map[string][]ades.Violation, 1) - m["action.yml"] = []ades.Violation{ + violations: map[string][]ades.Violation{ + "action.yml": { { StepId: "4", Problem: "${{ foo.bar }}", @@ -209,9 +187,7 @@ func TestPrintProjectViolations(t *testing.T) { Problem: "${{ foo.baz }}", RuleId: "ADES101", }, - } - - return m + }, }, want: `Detected 2 violation(s) in "action.yml": step "4" contains "${{ foo.bar }}" (ADES100) @@ -219,26 +195,23 @@ func TestPrintProjectViolations(t *testing.T) { `, }, "Project with multiple workflows": { - violations: func() map[string][]ades.Violation { - m := make(map[string][]ades.Violation, 2) - m["workflow-a.yml"] = []ades.Violation{ + violations: map[string][]ades.Violation{ + "workflow-a.yml": { { JobId: "4", StepId: "2", Problem: "${{ foo.bar }}", RuleId: "ADES100", }, - } - m["workflow-b.yml"] = []ades.Violation{ + }, + "workflow-b.yml": { { JobId: "3", StepId: "14", Problem: "${{ foo.baz }}", RuleId: "ADES101", }, - } - - return m + }, }, want: `Detected 1 violation(s) in "workflow-a.yml": 1 in job "4": @@ -254,7 +227,7 @@ Detected 1 violation(s) in "workflow-b.yml": t.Run(name, func(t *testing.T) { t.Parallel() - if got, want := printProjectViolations(tt.violations()), tt.want; got != want { + if got, want := printProjectViolations(tt.violations), tt.want; got != want { t.Errorf("Unexpected output\n=== GOT ===\n%s\n=== WANT ===\n%s", got, want) } }) diff --git a/matchers_test.go b/matchers_test.go index fe36f8f..bfb63b2 100644 --- a/matchers_test.go +++ b/matchers_test.go @@ -26,68 +26,68 @@ func TestAllMatcher(t *testing.T) { want []string } - testCases := []TestCase{ - { + testCases := map[string]TestCase{ + "arbitrary expression ": { value: "${{ foo.bar }}", want: []string{ "${{ foo.bar }}", }, }, - { + "input expression": { value: "${{ input.greeting }}", want: []string{ "${{ input.greeting }}", }, }, - { + "matrix expression": { value: "${{ matrix.runtime }}", want: []string{ "${{ matrix.runtime }}", }, }, - { + "vars expression": { value: "${{ vars.command }}", want: []string{ "${{ vars.command }}", }, }, - { + "secrets expression": { value: "${{ secrets.value }}", want: []string{ "${{ secrets.value }}", }, }, - { + "github.event.issue.title": { value: "${{ github.event.issue.title }}", want: []string{ "${{ github.event.issue.title }}", }, }, - { + "github.event.discussion.body": { value: "${{ github.event.discussion.body }}", want: []string{ "${{ github.event.discussion.body }}", }, }, - { + "github.event.pages[*].page_name": { value: "${{ github.event.pages[0].page_name }}", want: []string{ "${{ github.event.pages[0].page_name }}", }, }, - { + "github.event.commits[*].author.email": { value: "${{ github.event.commits[1].author.email }}", want: []string{ "${{ github.event.commits[1].author.email }}", }, }, - { + "github.head_ref": { value: "${{ github.head_ref }}", want: []string{ "${{ github.head_ref }}", }, }, - { + "github.event.workflow_run.pull_requests[*].head.ref": { value: "${{ github.event.workflow_run.pull_requests[2].head.ref }}", want: []string{ "${{ github.event.workflow_run.pull_requests[2].head.ref }}", @@ -95,8 +95,8 @@ func TestAllMatcher(t *testing.T) { }, } - for _, tt := range testCases { - t.Run(tt.value, func(t *testing.T) { + for name, tt := range testCases { + t.Run(name, func(t *testing.T) { t.Parallel() s := fmt.Sprintf("echo '%s'", tt.value) diff --git a/parse_test.go b/parse_test.go index c83de38..703a703 100644 --- a/parse_test.go +++ b/parse_test.go @@ -20,14 +20,12 @@ import "testing" func TestParseWorkflow(t *testing.T) { t.Run("Success", func(t *testing.T) { type TestCase struct { - name string yaml string want Workflow } - testCases := []TestCase{ - { - name: "Workflow with 'run:'", + testCases := map[string]TestCase{ + "Workflow with 'run:'": { yaml: ` jobs: example: @@ -58,8 +56,7 @@ jobs: }, }, }, - { - name: "Workflow with 'actions/github-script'", + "Workflow with 'actions/github-script'": { yaml: ` jobs: example: @@ -95,8 +92,7 @@ jobs: }, }, }, - { - name: "No names", + "No names": { yaml: ` jobs: example: @@ -122,8 +118,7 @@ jobs: }, }, }, - { - name: "No names", + "Version annotation": { yaml: ` jobs: example: @@ -146,8 +141,8 @@ jobs: }, } - for _, tt := range testCases { - t.Run(tt.name, func(t *testing.T) { + for name, tt := range testCases { + t.Run(name, func(t *testing.T) { t.Parallel() workflow, err := ParseWorkflow([]byte(tt.yaml)) @@ -200,27 +195,23 @@ jobs: t.Run("Error", func(t *testing.T) { type TestCase struct { - name string yaml string } - testCases := []TestCase{ - { - name: "Invalid 'jobs' value", + testCases := map[string]TestCase{ + "Invalid 'jobs' value": { yaml: ` jobs: 3.14 `, }, - { - name: "Invalid 'steps' value", + "Invalid 'steps' value": { yaml: ` jobs: example: steps: 42 `, }, - { - name: "Invalid 'env' value", + "Invalid 'env' value": { yaml: ` jobs: example: @@ -228,8 +219,7 @@ jobs: - env: 1.618 `, }, - { - name: "Invalid 'with' value", + "Invalid 'with' value": { yaml: ` jobs: example: @@ -239,8 +229,8 @@ jobs: }, } - for _, tt := range testCases { - t.Run(tt.name, func(t *testing.T) { + for name, tt := range testCases { + t.Run(name, func(t *testing.T) { t.Parallel() _, err := ParseWorkflow([]byte(tt.yaml)) @@ -255,14 +245,12 @@ jobs: func TestParseManifest(t *testing.T) { t.Run("Success", func(t *testing.T) { type TestCase struct { - name string yaml string want Manifest } - testCases := []TestCase{ - { - name: "Non-composite manifest", + testCases := map[string]TestCase{ + "Non-composite manifest": { yaml: ` runs: using: node16 @@ -274,8 +262,7 @@ runs: }, }, }, - { - name: "Manifest with 'run:'", + "Manifest with 'run:'": { yaml: ` runs: using: composite @@ -303,8 +290,7 @@ runs: }, }, }, - { - name: "Manifest with 'actions/github-script'", + "Manifest with 'actions/github-script'": { yaml: ` runs: using: composite @@ -339,8 +325,8 @@ runs: }, } - for _, tt := range testCases { - t.Run(tt.name, func(t *testing.T) { + for name, tt := range testCases { + t.Run(name, func(t *testing.T) { t.Parallel() manifest, err := ParseManifest([]byte(tt.yaml)) @@ -381,17 +367,14 @@ runs: t.Run("Error", func(t *testing.T) { type TestCase struct { - name string yaml string } - testCases := []TestCase{ - { - name: "Invalid 'runs' value", + testCases := map[string]TestCase{ + "Invalid 'runs' value": { yaml: `runs: 3.14`, }, - { - name: "Invalid 'steps' value", + "Invalid 'steps' value": { yaml: ` runs: steps: 3.14 @@ -399,8 +382,8 @@ runs: }, } - for _, tt := range testCases { - t.Run(tt.name, func(t *testing.T) { + for name, tt := range testCases { + t.Run(name, func(t *testing.T) { t.Parallel() _, err := ParseManifest([]byte(tt.yaml)) @@ -415,14 +398,12 @@ runs: func TestParseUses(t *testing.T) { t.Run("Success", func(t *testing.T) { type TestCase struct { - name string step JobStep want StepUses } - testCases := []TestCase{ - { - name: "Full version tag", + testCases := map[string]TestCase{ + "Full version tag": { step: JobStep{ Uses: "foobar@v1.2.3", }, @@ -431,8 +412,7 @@ func TestParseUses(t *testing.T) { Ref: "v1.2.3", }, }, - { - name: "Major version tag", + "Major version tag": { step: JobStep{ Uses: "hello-world@v2", }, @@ -441,8 +421,7 @@ func TestParseUses(t *testing.T) { Ref: "v2", }, }, - { - name: "Full SHA", + "Full SHA": { step: JobStep{ Uses: "actions/checkout@2a08af6587712680d7d485082f61ed6cdb72280a", }, @@ -451,8 +430,7 @@ func TestParseUses(t *testing.T) { Ref: "2a08af6587712680d7d485082f61ed6cdb72280a", }, }, - { - name: "Unconventional tag (no 'v' prefix)", + "Unconventional tag (no 'v' prefix)": { step: JobStep{ Uses: "actions/upload-artifact@3.1.4", }, @@ -461,8 +439,7 @@ func TestParseUses(t *testing.T) { Ref: "3.1.4", }, }, - { - name: "short name", + "short name": { step: JobStep{ Uses: "a@3.1.4", }, @@ -471,8 +448,7 @@ func TestParseUses(t *testing.T) { Ref: "3.1.4", }, }, - { - name: "1 character version", + "1 character version": { step: JobStep{ Uses: "actions/download-artifact@7", }, @@ -481,8 +457,7 @@ func TestParseUses(t *testing.T) { Ref: "7", }, }, - { - name: "with comment", + "with comment": { step: JobStep{ Uses: "actions/checkout@0ad4b8fadaa221de15dcec353f45205ec38ea70b", UsesComment: "v4", @@ -495,8 +470,8 @@ func TestParseUses(t *testing.T) { }, } - for _, tt := range testCases { - t.Run(tt.name, func(t *testing.T) { + for name, tt := range testCases { + t.Run(name, func(t *testing.T) { t.Parallel() uses, err := ParseUses(&tt.step) @@ -521,37 +496,32 @@ func TestParseUses(t *testing.T) { t.Run("Error", func(t *testing.T) { type TestCase struct { - name string step JobStep } - testCases := []TestCase{ - { - name: "No 'uses' value", + testCases := map[string]TestCase{ + "No 'uses' value": { step: JobStep{}, }, - { - name: "Invalid 'uses' value", + "Invalid 'uses' value": { step: JobStep{ Uses: "foobar", }, }, - { - name: "Missing version", + "Missing version": { step: JobStep{ Uses: "foobar@", }, }, - { - name: "Missing name", + "Missing name": { step: JobStep{ Uses: "@v1.2.3", }, }, } - for _, tt := range testCases { - t.Run(tt.name, func(t *testing.T) { + for name, tt := range testCases { + t.Run(name, func(t *testing.T) { t.Parallel() _, err := ParseUses(&tt.step) diff --git a/rules_test.go b/rules_test.go index 91e1cc5..d5852f8 100644 --- a/rules_test.go +++ b/rules_test.go @@ -529,14 +529,13 @@ func TestAllRules(t *testing.T) { ids := make([]string, 0) for _, tt := range testCases { - id := tt.id - ids = append(ids, id) + ids = append(ids, tt.id) t.Run(tt.title, func(t *testing.T) { t.Parallel() - if !idExpr.MatchString(id) { - t.Errorf("The ID did not match the expected format (got %q)", id) + if !idExpr.MatchString(tt.id) { + t.Errorf("The ID did not match the expected format (got %q)", tt.id) } }) } @@ -555,7 +554,7 @@ func TestAllRules(t *testing.T) { t.Run("description", func(t *testing.T) { for _, tt := range testCases { - t.Run(tt.title, func(t *testing.T) { + t.Run(tt.id, func(t *testing.T) { t.Parallel() if len(tt.description) == 0 { @@ -580,12 +579,13 @@ func TestAllRules(t *testing.T) { func TestExplain(t *testing.T) { t.Run("Existing rules", func(t *testing.T) { - for _, r := range allRules() { - tt := r.id - t.Run(tt, func(t *testing.T) { + testCases := allRules() + + for _, tt := range testCases { + t.Run(tt.id, func(t *testing.T) { t.Parallel() - explanation, err := Explain(tt) + explanation, err := Explain(tt.id) if err != nil { t.Fatalf("Unexpected error: %#v", err) } @@ -598,13 +598,15 @@ func TestExplain(t *testing.T) { }) t.Run("Missing rules", func(t *testing.T) { - testCases := []string{ - "ADES000", - "foobar", + type TestCase = string + + testCases := map[string]TestCase{ + "valid but unknown id": "ADES000", + "invalid id": "foobar", } - for _, tt := range testCases { - t.Run(tt, func(t *testing.T) { + for name, tt := range testCases { + t.Run(name, func(t *testing.T) { t.Parallel() _, err := Explain(tt) @@ -618,15 +620,16 @@ func TestExplain(t *testing.T) { func TestFix(t *testing.T) { t.Run("Existing rules", func(t *testing.T) { - for _, r := range allRules() { - tt := r.id - violation := Violation{ - RuleId: tt, - } + testCases := allRules() - t.Run(tt, func(t *testing.T) { + for _, tt := range testCases { + t.Run(tt.id, func(t *testing.T) { t.Parallel() + violation := Violation{ + RuleId: tt.id, + } + _, err := Fix(&violation) if err != nil { t.Fatalf("Unexpected error: %#v", err) @@ -636,19 +639,21 @@ func TestFix(t *testing.T) { }) t.Run("Missing rules", func(t *testing.T) { - testCases := []string{ - "ADES000", - "foobar", - } + type TestCase = string - for _, tt := range testCases { - violation := Violation{ - RuleId: tt, - } + testCases := map[string]TestCase{ + "valid but unknown id": "ADES000", + "invalid id": "foobar", + } - t.Run(tt, func(t *testing.T) { + for name, tt := range testCases { + t.Run(name, func(t *testing.T) { t.Parallel() + violation := Violation{ + RuleId: tt, + } + _, err := Fix(&violation) if err == nil { t.Fatal("Expected an error, got none") @@ -660,15 +665,16 @@ func TestFix(t *testing.T) { func TestSuggestion(t *testing.T) { t.Run("Existing rules", func(t *testing.T) { - for _, r := range allRules() { - tt := r.id - violation := Violation{ - RuleId: tt, - } + testCases := allRules() - t.Run(tt, func(t *testing.T) { + for _, tt := range testCases { + t.Run(tt.id, func(t *testing.T) { t.Parallel() + violation := Violation{ + RuleId: tt.id, + } + suggestion, err := Suggestion(&violation) if err != nil { t.Fatalf("Unexpected error: %#v", err) @@ -682,19 +688,21 @@ func TestSuggestion(t *testing.T) { }) t.Run("Missing rules", func(t *testing.T) { - testCases := []string{ - "ADES000", - "foobar", - } + type TestCase = string - for _, tt := range testCases { - violation := Violation{ - RuleId: tt, - } + testCases := map[string]TestCase{ + "valid but unknown id": "ADES000", + "invalid id": "foobar", + } - t.Run(tt, func(t *testing.T) { + for name, tt := range testCases { + t.Run(name, func(t *testing.T) { t.Parallel() + violation := Violation{ + RuleId: tt, + } + _, err := Suggestion(&violation) if err == nil { t.Fatal("Expected an error, got none") @@ -705,36 +713,38 @@ func TestSuggestion(t *testing.T) { } func TestFindRule(t *testing.T) { - t.Run("Existing rules", func(t *testing.T) { - for _, rs := range actionRules { - for _, r := range rs { - tt := r.rule.id - t.Run(tt, func(t *testing.T) { + t.Run("Action rules", func(t *testing.T) { + for _, testCases := range actionRules { + for _, tt := range testCases { + t.Run(tt.rule.id, func(t *testing.T) { t.Parallel() - r, err := findRule(tt) + r, err := findRule(tt.rule.id) if err != nil { - t.Fatalf("Couldn't find rule %q", tt) + t.Fatalf("Couldn't find rule %q", tt.rule.id) } - if r.id != tt { + if r.id != tt.rule.id { t.Errorf("Unexpected rule found: %#v", r) } }) } } + }) + + t.Run("Step rules", func(t *testing.T) { + testCases := stepRules - for _, r := range stepRules { - tt := r.rule.id - t.Run(tt, func(t *testing.T) { + for _, tt := range testCases { + t.Run(tt.rule.id, func(t *testing.T) { t.Parallel() - r, err := findRule(tt) + r, err := findRule(tt.rule.id) if err != nil { - t.Fatalf("Couldn't find rule %q", tt) + t.Fatalf("Couldn't find rule %q", tt.rule.id) } - if r.id != tt { + if r.id != tt.rule.id { t.Errorf("Unexpected rule found: %#v", r) } }) @@ -742,13 +752,15 @@ func TestFindRule(t *testing.T) { }) t.Run("Missing rules", func(t *testing.T) { - testCases := []string{ - "ADES000", - "foobar", + type TestCase = string + + testCases := map[string]TestCase{ + "valid but unknown id": "ADES000", + "invalid id": "foobar", } - for _, tt := range testCases { - t.Run(tt, func(t *testing.T) { + for name, tt := range testCases { + t.Run(name, func(t *testing.T) { t.Parallel() _, err := findRule(tt) @@ -988,8 +1000,8 @@ func TestFixAddEnvVar(t *testing.T) { } ) - testCases := []TestCase{ - { + testCases := map[string]TestCase{ + "no environment variables yet": { step: JobStep{ Uses: "foo/bar@v1", Env: nil, @@ -1007,7 +1019,7 @@ func TestFixAddEnvVar(t *testing.T) { }, }, }, - { + "one environment variable already": { step: JobStep{ Env: map[string]string{ "foo": "bar", @@ -1022,7 +1034,7 @@ func TestFixAddEnvVar(t *testing.T) { }, }, }, - { + "two environment variables already": { step: JobStep{ Env: map[string]string{ "foo": "bar", @@ -1043,8 +1055,8 @@ func TestFixAddEnvVar(t *testing.T) { }, } - for _, tt := range testCases { - t.Run(tt.name+" "+tt.value, func(t *testing.T) { + for name, tt := range testCases { + t.Run(name, func(t *testing.T) { t.Parallel() fs := fixAddEnvVar(tt.step, tt.name, tt.value) @@ -1077,8 +1089,8 @@ func TestFixReplaceIn(t *testing.T) { } ) - testCases := []TestCase{ - { + testCases := map[string]TestCase{ + "basic example": { s: "hello foobar world!", old: "foobar", new: "foobaz", @@ -1087,7 +1099,7 @@ func TestFixReplaceIn(t *testing.T) { new: "hello foobaz world!", }, }, - { + "example with regular expression meta characters": { s: "Hello world! (Hola mundo!)", old: "!", new: "", @@ -1096,7 +1108,7 @@ func TestFixReplaceIn(t *testing.T) { new: "Hello world (Hola mundo)", }, }, - { + "example where replacing is a no-op": { s: "This does not contain the string to replace", old: "foobar", new: "foobaz", @@ -1107,8 +1119,8 @@ func TestFixReplaceIn(t *testing.T) { }, } - for _, tt := range testCases { - t.Run(tt.s, func(t *testing.T) { + for name, tt := range testCases { + t.Run(name, func(t *testing.T) { t.Parallel() f := fixReplaceIn(tt.s, tt.old, tt.new)