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

[github-actions]

The test file pkg/workflow/agentic_engine_test.go has been selected for quality improvement by the Testify Uber Super Expert. This issue provides specific, actionable recommendations to enhance test quality using testify best practices.

Current State

• Test File: pkg/workflow/agentic_engine_test.go
• Source File: pkg/workflow/agentic_engine.go
• Test Functions: 1 (TestEngineRegistry)
• Lines of Code: 80 lines
• Untested Methods: Register, GetAllAgentManifestFolders, GetAllAgentManifestFiles, GetGlobalEngineRegistry

Test Quality Analysis

Strengths ✅

• Tests cover the main happy-path and error-path scenarios for EngineRegistry
• Tests are isolated and use a fresh registry via NewEngineRegistry()
• Engine list validation is done with a loop to avoid brittleness on order

🎯 Areas for Improvement

1. Testify Assertions

The entire file uses plain Go t.Errorf / t.Error instead of testify, which is the project standard.

Current Issues (examples from file):

// ❌ CURRENT
if err != nil {
    t.Errorf("Expected to find claude engine, got error: %v", err)
}
if claudeEngine.GetID() != "claude" {
    t.Errorf("Expected claude engine ID, got '%s'", claudeEngine.GetID())
}
if registry.IsValidEngine("nonexistent") {
    t.Error("Expected nonexistent to be invalid engine")
}
_, err = registry.GetEngine("nonexistent")
if err == nil {
    t.Error("Expected error when getting non-existent engine")
}

Recommended Changes:

// ✅ IMPROVED
claudeEngine, err := registry.GetEngine("claude")
require.NoError(t, err, "GetEngine('claude') should succeed")
assert.Equal(t, "claude", claudeEngine.GetID(), "engine ID should match")

assert.False(t, registry.IsValidEngine("nonexistent"), "nonexistent engine should be invalid")

_, err = registry.GetEngine("nonexistent")
assert.Error(t, err, "GetEngine('nonexistent') should return an error")

Why this matters: Testify's require.NoError stops the test immediately on failure, preventing a nil-pointer panic on the next line. assert.* provides cleaner, more readable output in CI logs.

2. Table-Driven Tests

The file contains repetitive patterns for checking multiple engine IDs that are ideal candidates for table-driven tests.

Current Issues:

// Repeated pattern for every engine
claudeEngine, err := registry.GetEngine("claude")
if err != nil { t.Errorf(...) }
if claudeEngine.GetID() != "claude" { t.Errorf(...) }

codexEngine, err := registry.GetEngine("codex")
if err != nil { t.Errorf(...) }
if codexEngine.GetID() != "codex" { t.Errorf(...) }

Recommended Changes:

// ✅ Table-driven for per-engine checks
engines := []string{"claude", "codex", "copilot", "gemini", "opencode", "crush"}
for _, id := range engines {
    t.Run("engine/"+id, func(t *testing.T) {
        engine, err := registry.GetEngine(id)
        require.NoError(t, err, "GetEngine(%q) should succeed", id)
        assert.Equal(t, id, engine.GetID(), "engine ID should match requested ID")
        assert.True(t, registry.IsValidEngine(id), "IsValidEngine(%q) should be true", id)
    })
}

3. Test Coverage Gaps

The following exported methods/functions have no test coverage:

Method	Why it matters
Register	Core registry mutation — should verify custom engines can be added and retrieved
GetAllAgentManifestFolders	Returns filesystem paths used at runtime — should verify non-nil/non-empty
GetAllAgentManifestFiles	Same as above
GetGlobalEngineRegistry	Singleton accessor — should verify it returns a valid, non-nil registry with all engines

Recommended new test:

func TestEngineRegistry_Register(t *testing.T) {
    registry := NewEngineRegistry()
    initialCount := len(registry.GetSupportedEngines())

    // Register a custom engine
    // (mock or minimal CodingAgentEngine implementation)
    // ...

    assert.Len(t, registry.GetSupportedEngines(), initialCount+1,
        "registry should contain the newly registered engine")
}

func TestGetGlobalEngineRegistry(t *testing.T) {
    registry := GetGlobalEngineRegistry()
    require.NotNil(t, registry, "global registry should not be nil")
    assert.NotEmpty(t, registry.GetSupportedEngines(),
        "global registry should have at least one engine")
}

func TestEngineRegistry_GetAllAgentManifestFiles(t *testing.T) {
    registry := NewEngineRegistry()
    files := registry.GetAllAgentManifestFiles()
    assert.NotNil(t, files, "manifest files slice should not be nil")
}

4. Test Organization — Single Monolithic Test

The current TestEngineRegistry function tests 7 different behaviours in a single function. This makes it hard to tell which assertion failed and why.

Recommended: Split into t.Run subtests:

func TestEngineRegistry(t *testing.T) {
    t.Run("built-in engines are registered", func(t *testing.T) { ... })
    t.Run("get engine by ID succeeds", func(t *testing.T) { ... })
    t.Run("get non-existent engine returns error", func(t *testing.T) { ... })
    t.Run("IsValidEngine", func(t *testing.T) { ... })
    t.Run("default engine is copilot", func(t *testing.T) { ... })
    t.Run("GetEngineByPrefix matches prefix", func(t *testing.T) { ... })
    t.Run("GetEngineByPrefix no match returns error", func(t *testing.T) { ... })
}

5. Missing Assertion Messages

Most assertions lack context messages, making CI output hard to parse.

// ❌ No context on failure
if !found { t.Errorf("Expected engine '%s' to be registered", engineID) }

// ✅ testify with context
assert.True(t, found, "engine %q should be registered in the default registry", engineID)

📋 Implementation Guidelines

Priority Order

1. High: Replace all t.Errorf/t.Error with require.* / assert.*
2. High: Add TestGetGlobalEngineRegistry (global singleton is runtime-critical)
3. Medium: Split TestEngineRegistry into t.Run subtests
4. Medium: Add coverage for Register, GetAllAgentManifestFolders, GetAllAgentManifestFiles
5. Low: Consolidate per-engine repeated blocks into a table-driven loop

Best Practices from scratchpad/testing.md

• ✅ Use require.* for setup that must not fail (e.g., GetEngine)
• ✅ Use assert.* for property checks (e.g., ID equality, bool flags)
• ✅ Write table-driven tests with t.Run() and descriptive names
• ✅ Always include helpful assertion messages

Testing Commands

# Run this specific test
go test -v ./pkg/workflow/ -run TestEngineRegistry

# Run all workflow unit tests
go test -v ./pkg/workflow/

# Full validation
make test-unit

Acceptance Criteria

• All t.Error / t.Errorf replaced with require.* / assert.* from testify
• TestEngineRegistry split into t.Run subtests or replaced with table-driven tests
• GetGlobalEngineRegistry, Register, GetAllAgentManifestFolders, and GetAllAgentManifestFiles have test coverage
• All assertions include a descriptive message parameter
• Tests pass: go test -v ./pkg/workflow/ -run TestEngineRegistry
• Code follows patterns in scratchpad/testing.md

Additional Context

• Repository Testing Guidelines: See scratchpad/testing.md
• Testify Documentation: https://github.com/stretchr/testify
• Source file: pkg/workflow/agentic_engine.go (~22 KB, multiple exported methods)

---

Priority: Medium
Effort: Small (~1-2 hours)
Expected Impact: Cleaner CI output, faster failure diagnosis, coverage of critical global registry path

Files Involved:

• Test file: pkg/workflow/agentic_engine_test.go
• Source file: pkg/workflow/agentic_engine.go

References:

• Workflow run §24887664250

Generated by Daily Testify Uber Super Expert · ● 1M · ◷

• expires on Apr 26, 2026, 11:47 AM UTC

[github-actions]

🍪 Issue Monster selected this for Copilot!

I've identified this issue as a good candidate for automated resolution and requested assignment to the Copilot coding agent.

If assignment succeeds, the Copilot coding agent will analyze the issue and create a pull request with the fix.

Om nom nom! 🍪

🍪 Om nom nom by Issue Monster · ● 12.8K · ◷

[gh-aw-bot]

⚠️ Assignment failed: Failed to assign copilot coding agent to this issue.

Error: Failed to assign copilot via GraphQL

[gh-aw-bot]

⚠️ Assignment failed: Failed to assign copilot coding agent to this issue.

Error: Failed to assign copilot via GraphQL

[github-actions]

🍪 *Issue Monster selected this for Copilotecho BEGIN___COMMAND_OUTPUT_MARKER ; PS1= ; PS2= ; unset HISTFILE ; EC=0 ; echo ___BEGIN___COMMAND_DONE_MARKER___0 ; }

I've identified this issue as a good candidate for automated resolution and requested assignment to the Copilot coding agent.

If assignment succeeds, the Copilot coding agent will analyze the issue and create a pull request with the fix.

Om nom nom! 🍪

Caution

Security scanning requires review for Issue Monster

Details

Potential security threats were detected in the agent output. The workflow output should be reviewed before merging.

Review the workflow run logs for details.

🍪 Om nom nom by Issue Monster · ● 17.3K · ◷