Skip to content

Migrate pkg/parser/frontmatter_helpers_test.go to testify assertions#21108

Merged
pelikhan merged 3 commits intomainfrom
copilot/testify-expert-improve-test-quality
Mar 15, 2026
Merged

Migrate pkg/parser/frontmatter_helpers_test.go to testify assertions#21108
pelikhan merged 3 commits intomainfrom
copilot/testify-expert-improve-test-quality

Conversation

Copy link
Contributor

Copilot AI commented Mar 15, 2026

The only test file in pkg/parser/ not using testify — replaced 14 raw t.Fatal/t.Errorf calls and manual strings.Contains checks with require/assert, consistent with the rest of the package.

Changes

  • Imports: removed "strings", added testify/assert and testify/require
  • TestUpdateWorkflowFrontmatter:
    • t.Fatalf/t.Errorfrequire.NoError / assert.Error
    • strings.Contains + t.Errorfassert.Contains
    • Added verbose bool field and a verbose=true test case that verifies the frontmatter update is applied correctly
  • TestEnsureToolsSection: replaced 15-line manual nested-map comparison with assert.Equal; nil/type-assertion guards → require.NotNil / require.True
  • TestReconstructWorkflowFile: t.Errorf + early return → require.NoError + assert.Equal
  • TestQuoteCronExpressions: not added — already exists in quote_cron_expressions_test.go with 18 cases

Before / after (tools verification):

// Before
if !strings.Contains(content, "new-tool:") {
    t.Errorf("Expected 'new-tool:' in updated content, got: %s", content)
}

// After
assert.Contains(t, content, "new-tool:", "updated file should contain the new tool key")
// Before — 15 lines of manual map walking
for key, expectedValue := range tt.expectedTools {
    if actualValue, exists := tools[key]; !exists {
        t.Errorf("Expected tool '%s' not found", key)
    } else { ... }
}

// After
assert.Equal(t, tt.expectedTools, tools, "tools section should match expected state")

Copilot AI and others added 2 commits March 15, 2026 15:13
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
@pelikhan pelikhan marked this pull request as ready for review March 15, 2026 19:32
Copilot AI review requested due to automatic review settings March 15, 2026 19:32
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Migrates pkg/parser/frontmatter_helpers_test.go to use testify/assert and testify/require for more consistent, readable test assertions across pkg/parser.

Changes:

  • Replaced t.Fatal/t.Errorf and manual strings.Contains checks with require/assert.
  • Added a new “verbose” test case wiring through the verbose argument to UpdateWorkflowFrontmatter.
  • Simplified EnsureToolsSection verification by using assert.Equal and require guards.

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

You can also share your feedback on Copilot code review. Take the survey.

Comment on lines +20 to +26
updateFunc func(frontmatter map[string]any) error
expectedContent string
expectError bool
verbose bool
Comment on lines +127 to +138
name: "Verbose mode does not affect output",
initialContent: `---
engine: claude
---
# Test`,
updateFunc: func(frontmatter map[string]any) error {
frontmatter["engine"] = "copilot"
return nil
},
expectError: false,
verbose: true,
},
}
}
// Verify returned tools matches the frontmatter entry and expected content
assert.Equal(t, frontmatterTools, tools, "returned tools should be the same reference as frontmatter['tools']")
@pelikhan
Copy link
Contributor

@copilot apply comments

…e test, prove reference identity by mutation

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Mar 15, 2026

@copilot apply comments

Done in fa6f75a:

  1. Removed unused expectedContent — field and all populated values dropped from the struct; no dead strings remain.
  2. Verbose test renamed + stderr captured — case is now "Verbose mode emits info message to stderr" and uses testutil.CaptureStderr to assert "Updated workflow file" appears in stderr, then checks the frontmatter update was actually applied.
  3. Reference identity proved by mutation — replaced the misleading assert.Equal(frontmatterTools, tools, "same reference…") with a probe: inserts a sentinel key into tools and asserts it's visible via frontmatter["tools"], which only passes if they're the same underlying map.

Copilot AI requested a review from pelikhan March 15, 2026 19:44
@pelikhan pelikhan merged commit 8c9d307 into main Mar 15, 2026
53 checks passed
@pelikhan pelikhan deleted the copilot/testify-expert-improve-test-quality branch March 15, 2026 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants