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

[agentic-workflows-dev]

Overview

The test file pkg/workflow/mcp_utilities_test.go has been selected for quality review by the Testify Uber Super Expert. This file demonstrates excellent testify practices with comprehensive table-driven tests and proper assertion usage. This review identifies the file's strengths and suggests minor enhancements to achieve perfection.

Current State

• Test File: pkg/workflow/mcp_utilities_test.go
• Source File: pkg/workflow/mcp_utilities.go
• Test Functions: 3 test functions
• Lines of Code: 151 lines (test), 44 lines (source)
• Last Modified: 2026-01-26
• Test Coverage: ✅ 100% - All 2 functions in source file are tested

Test Quality Analysis

Strengths ✅

This test file is a model example of testify best practices and should be referenced as a template for other tests:

1. Perfect table-driven test structure - Uses comprehensive test tables with descriptive names covering edge cases
2. Proper testify assertion usage - Uses assert.Equal() and assert.Contains() with helpful messages
3. Excellent test organization - Uses t.Run() for subtests with clear, descriptive names
4. Complete coverage - Tests all functions (100% coverage) including edge cases like empty strings, special characters, and complex scenarios
5. Good test separation - Separate test for specific behavior (TestBuildDockerCommandWithExpandableVars_PreservesVariableExpansion)

Areas for Minor Enhancement 🎯

1. Add require.* for Critical Checks in Preserve Test

Current State:
The TestBuildDockerCommandWithExpandableVars_PreservesVariableExpansion function uses only assert.* assertions. While this works, using require.* for the first assertion would make the test fail-fast if the fundamental requirement isn't met.

Current Code:

func TestBuildDockerCommandWithExpandableVars_PreservesVariableExpansion(t *testing.T) {
    input := "docker run -v ${GITHUB_WORKSPACE}:/workspace"
    result := buildDockerCommandWithExpandableVars(input)

    // Both assertions use assert.*
    assert.Contains(t, result, "${GITHUB_WORKSPACE}", "Result should preserve GITHUB_WORKSPACE variable for expansion")
    assert.Contains(t, result, "\"${GITHUB_WORKSPACE}\"", "GITHUB_WORKSPACE should be in double quotes for safe expansion")
}

Recommended Change:

func TestBuildDockerCommandWithExpandableVars_PreservesVariableExpansion(t *testing.T) {
    input := "docker run -v ${GITHUB_WORKSPACE}:/workspace"
    result := buildDockerCommandWithExpandableVars(input)

    // Use require.* for the first critical check (fail-fast)
    require.Contains(t, result, "${GITHUB_WORKSPACE}", "Result should preserve GITHUB_WORKSPACE variable for expansion")
    
    // Use assert.* for the second check (still runs if we get here)
    assert.Contains(t, result, "\"${GITHUB_WORKSPACE}\"", "GITHUB_WORKSPACE should be in double quotes for safe expansion")
}

Why this matters: Following the pattern from specs/testing.md, use require.* for critical setup/preconditions that must pass before continuing. This makes test failures clearer - if the variable isn't preserved at all (first check), we don't need to check the quoting format (second check).

2. Consider Adding Negative Test Cases

Current State:
The tests comprehensively cover positive cases (valid inputs) and edge cases. Consider adding explicit tests for security-related edge cases.

Potential Additional Test Cases:

// Add to TestShellQuote table
{
    name:     "command injection attempt with semicolon",
    input:    "file; rm -rf /",
    expected: "'file; rm -rf /'",
},
{
    name:     "command injection attempt with pipe",
    input:    "file | cat /etc/passwd",
    expected: "'file | cat /etc/passwd'",
},
{
    name:     "multiple single quotes",
    input:    "it's a test's file",
    expected: "'it'\\''s a test'\\''s file'",
},

// Add to TestBuildDockerCommandWithExpandableVars table
{
    name:     "injection attempt in GITHUB_WORKSPACE context",
    input:    "${GITHUB_WORKSPACE}; rm -rf /",
    expected: "''\"${GITHUB_WORKSPACE}\"'; rm -rf /'",
},
{
    name:     "multiple variables mixed with GITHUB_WORKSPACE",
    input:    "${GITHUB_WORKSPACE}/src ${OTHER_VAR}/dst",
    expected: "''\"${GITHUB_WORKSPACE}\"'/src ${OTHER_VAR}/dst'",
},

Why this matters: These functions are used for shell quoting and Docker command construction - security-critical operations. Explicitly testing injection attempts documents the security behavior and prevents regressions.

3. Add Test for $GITHUB_WORKSPACE (Without Braces)

Current State:
The source file comment mentions both ${GITHUB_WORKSPACE} (with braces) and $GITHUB_WORKSPACE (without braces):

// buildDockerCommandWithExpandableVars builds a properly quoted docker command
// that allows ${GITHUB_WORKSPACE} and $GITHUB_WORKSPACE to be expanded at runtime

However, the current implementation and tests only handle ${GITHUB_WORKSPACE} (with braces).

Recommended Test Addition:

func TestBuildDockerCommandWithExpandableVars_UnbracedVariable(t *testing.T) {
    // Test that $GITHUB_WORKSPACE (without braces) is handled
    input := "docker run -v $GITHUB_WORKSPACE:/workspace"
    result := buildDockerCommandWithExpandableVars(input)
    
    // Current behavior: treats it as a regular shell character
    assert.Equal(t, "'docker run -v $GITHUB_WORKSPACE:/workspace'", result, 
        "Unbraced $GITHUB_WORKSPACE should be quoted normally")
    
    // Alternatively, if it should be preserved like ${GITHUB_WORKSPACE}:
    // assert.Contains(t, result, "$GITHUB_WORKSPACE", 
    //     "Unbraced variable should also be preserved for expansion")
}

Why this matters: The comment claims to support both forms, but the code only handles the braced form. This test either:

1. Documents the current behavior (unbraced form is treated as a regular $ character and gets quoted)
2. Reveals a missing feature that should be implemented

Implementation Guidelines

Priority Order

1. Low: Add require.* for first assertion in preserve test (~1 min)
2. Low: Add security-focused negative test cases (~10 min)
3. Low: Add test for unbraced $GITHUB_WORKSPACE to clarify behavior (~5 min)

Best Practices Already Followed ✅

• ✅ Uses table-driven tests with t.Run() and descriptive names
• ✅ Uses testify assertions (assert.Equal, assert.Contains) with helpful messages
• ✅ Tests cover positive cases, edge cases, and boundary conditions
• ✅ Test organization is clear and maintainable
• ✅ 100% function coverage

Testing Commands

# Run tests for this file
go test -v ./pkg/workflow -run TestShellQuote
go test -v ./pkg/workflow -run TestBuildDockerCommandWithExpandableVars

# Run tests with coverage
go test -cover ./pkg/workflow

# Run all tests
make test-unit

Acceptance Criteria

• First assertion in TestBuildDockerCommandWithExpandableVars_PreservesVariableExpansion uses require.*
• Additional security-focused test cases added to test tables
• Test added for unbraced $GITHUB_WORKSPACE to document/test current behavior
• All tests pass: make test-unit
• Code continues to follow excellent patterns from specs/testing.md

Additional Context

Why This File Was Selected:

This file demonstrates exemplary testify practices and serves as a reference implementation. The suggestions above are minor enhancements to an already excellent test suite. Other test files in the repository should follow this file's patterns:

• Comprehensive table-driven tests
• Descriptive test case names
• Proper use of assert.* with messages
• Good coverage of edge cases

Repository Testing Guidelines: See specs/testing.md for comprehensive testing patterns

Example Tests: This file itself serves as an excellent example for other tests in the repository

Testify Documentation: https://github.com/stretchr/testify

---

Priority: Low
Effort: Small (~20 minutes)
Expected Impact: Minor enhancements to already excellent test quality; serves as reference for other tests

Files Involved:

• Test file: pkg/workflow/mcp_utilities_test.go
• Source file: pkg/workflow/mcp_utilities.go

AI generated by Daily Testify Uber Super Expert

[github-actions]

🍪 Issue Monster has assigned this to Copilot!

I've identified this issue as a good candidate for automated resolution and assigned it to the Copilot agent.

The Copilot agent will analyze the issue and create a pull request with the fix.

Om nom nom! 🍪

AI generated by Issue Monster