Skip to content

refactor(workflow): migrate resolve_test.go to testify assertions#34511

Merged
pelikhan merged 2 commits into
mainfrom
copilot/improve-test-quality-resolve-test
May 24, 2026
Merged

refactor(workflow): migrate resolve_test.go to testify assertions#34511
pelikhan merged 2 commits into
mainfrom
copilot/improve-test-quality-resolve-test

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented May 24, 2026

pkg/workflow/resolve_test.go had zero testify usage — relying entirely on manual if/t.Fatal/t.Errorf patterns — plus duplicated setup boilerplate across six test functions and a buggy custom contains helper that reimplemented strings.Contains.

Changes

  • Imports: Add testify/assert and testify/require
  • setupWorkflowDir helper: Extracts the repeated temp-dir + MkdirAll + t.Chdir boilerplate into a single helper returning the workflows path
  • Assertion migration:
    • t.Fatal(err)require.NoError(t, err, "...")
    • if err == nil { t.Errorf(...) }require.Error / assert.Error
    • if result != expected { t.Errorf(...) }assert.Equal / assert.Contains
    • All assertions include contextual failure messages
  • TestResolveWorkflowName_ExistingAgenticWorkflow: Replace os.Chdir + defer os.Chdir with t.Chdir
  • helpers_test.go (new): Relocates contains/findSubstring out of resolve_test.go — they're still referenced by step_order_validation_test.go and step_order_validation_bug_detection_test.go, so removing them entirely would break the build

Before / after:

// Before
err = os.MkdirAll(workflowsDir, 0755)
if err != nil {
    t.Fatal(err)
}
// ...
if result != tt.expectedWorkflowName {
    t.Errorf("Expected workflow name %q, got %q", tt.expectedWorkflowName, result)
}

// After
workflowsDir := setupWorkflowDir(t)
// ...
assert.Equal(t, tt.expectedWorkflowName, result,
    "ResolveWorkflowName(%q) should return correct workflow name", tt.workflowInput)

Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Copilot AI changed the title [WIP] Improve test quality of pkg/workflow/resolve_test.go refactor(workflow): migrate resolve_test.go to testify assertions May 24, 2026
Copilot AI requested a review from gh-aw-bot May 24, 2026 18:48
@pelikhan pelikhan marked this pull request as ready for review May 24, 2026 19:04
Copilot AI review requested due to automatic review settings May 24, 2026 19:04
Copy link
Copy Markdown
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

Refactors pkg/workflow/resolve_test.go to use testify/assert + testify/require, reducing repetitive boilerplate and standardizing assertion style for workflow resolution tests.

Changes:

  • Added setupWorkflowDir helper to centralize temp-dir + workflow-dir setup for multiple tests.
  • Migrated manual if/t.Fatal/t.Errorf assertions to testify assertions throughout the file.
  • Moved the legacy contains / findSubstring test helpers into a new helpers_test.go to keep other tests compiling.
Show a summary per file
File Description
pkg/workflow/resolve_test.go Migrates assertions to testify and extracts shared temp workflow-dir setup.
pkg/workflow/helpers_test.go Adds a shared (deprecated) contains helper used by other tests in the package.

Copilot's findings

Tip

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

Comments suppressed due to low confidence (1)

pkg/workflow/resolve_test.go:223

  • Using t.Chdir(projectRoot) changes the process working directory for the remainder of the test (global state). To keep the test hermetic and avoid cross-test interference, consider instead setting GH_AW_WORKFLOWS_DIR to an absolute path (e.g., filepath.Join(projectRoot, constants.GetWorkflowDir())) and using absolute paths for the os.Stat checks.
	t.Chdir(projectRoot)
  • Files reviewed: 2/2 changed files
  • Comments generated: 3

Comment on lines +27 to +33
// changes the working directory to it, and returns the path to the workflows directory.
func setupWorkflowDir(t *testing.T) string {
t.Helper()
tempDir := testutil.TempDir(t, "test-*")
workflowsDir := filepath.Join(tempDir, constants.GetWorkflowDir())
require.NoError(t, os.MkdirAll(workflowsDir, 0755), "failed to create workflows directory")
t.Chdir(tempDir)
Comment on lines 310 to +314
t.Setenv("GH_AW_FEATURES", tt.featureValue)
}

result := shouldSkipFirewallWorkflow(tt.workflowName)
if result != tt.shouldSkip {
t.Errorf("shouldSkipFirewallWorkflow(%q) with GH_AW_FEATURES=%q = %v, expected %v",
tt.workflowName, tt.featureValue, result, tt.shouldSkip)
}
assert.Equal(t, tt.shouldSkip, result,
Comment on lines +5 to +21
// contains checks if a string contains a substring.
// Deprecated: prefer strings.Contains or assert.Contains in new tests.
func contains(s, substr string) bool {
return len(s) >= len(substr) && (s == substr || len(substr) == 0 ||
(len(s) > len(substr) && s[len(s)-len(substr):] == substr) ||
(len(s) > len(substr) && s[:len(substr)] == substr) ||
(len(s) > len(substr) && findSubstring(s, substr)))
}

func findSubstring(s, substr string) bool {
for i := 0; i <= len(s)-len(substr); i++ {
if s[i:i+len(substr)] == substr {
return true
}
}
return false
}
@pelikhan pelikhan merged commit af46b9e into main May 24, 2026
4 checks passed
@pelikhan pelikhan deleted the copilot/improve-test-quality-resolve-test branch May 24, 2026 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[testify-expert] Improve Test Quality: pkg/workflow/resolve_test.go

4 participants