Refactor threat detection tests: testify assertions, table-driven patterns, helper functions

[Copilot]

Improves test quality in threat_detection_file_access_test.go by migrating to testify assertions, table-driven patterns, and adding coverage for previously untested step generation functions.

Changes

• Testify migration: Replace manual error checks with require.* for setup (file I/O, config creation) and assert.* for validations (string containment, output verification)

• Table-driven refactoring: Convert string validation tests into table-driven subtests with descriptive names and assertion messages

• Helper functions: Add createTestCompiler(t) and createTestWorkflowData(t, threatConfig) to reduce test boilerplate and improve maintainability

• New test coverage: Add comprehensive tests for step generation functions:

  • buildDownloadArtifactStep - validates artifact names and download paths
  • buildEchoAgentOutputsStep - validates output echoing with custom job names
  • buildThreatDetectionAnalysisStep - validates environment variable configuration

• Improved naming: Rename tests to behavior-focused patterns (e.g., TestThreatDetectionSteps_UseFilePathReferences)

Example

Before:

if !strings.Contains(stepsString, "setup_threat_detection.cjs") {
    t.Error("Expected threat detection to require setup_threat_detection.cjs file")
}

After:

tests := []struct {
    name        string
    shouldExist bool
    substring   string
    message     string
}{
    {
        name:        "requires setup script",
        shouldExist: true,
        substring:   "setup_threat_detection.cjs",
        message:     "threat detection steps should include setup_threat_detection.cjs for script execution",
    },
    // ... more test cases
}

for _, tt := range tests {
    t.Run(tt.name, func(t *testing.T) {
        if tt.shouldExist {
            assert.Contains(t, stepsString, tt.substring, tt.message)
        } else {
            assert.NotContains(t, stepsString, tt.substring, tt.message)
        }
    })
}

Note: parseThreatDetectionConfig and buildThreatDetectionJob are already comprehensively tested in threat_detection_test.go and not duplicated here.

Original prompt

---

This section details on the original issue you should resolve

<issue_title>[testify-expert] Improve Test Quality: pkg/workflow/threat_detection_file_access_test.go</issue_title>
<issue_description>### Overview

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

Current State

• Test File: pkg/workflow/threat_detection_file_access_test.go
• Source File: pkg/workflow/threat_detection.go
• Test Functions: 3 test functions
• Lines of Code: 117 lines
• Source Functions: 12 exported functions (methods on *Compiler)

Test Quality Analysis

Strengths ✅

1. Clear Test Names: Test function names clearly describe what they verify (e.g., TestThreatDetectionUsesFilePathNotInline, TestThreatDetectionHasBashReadTools)
2. Good Comments: Each test has a descriptive comment explaining its purpose
3. File Reading: TestThreatDetectionTemplateUsesFilePath correctly tests actual template file content

Areas for Improvement 🎯

1. Testify Assertions

Current Issues:

• Lines 30, 34, 39, 44, 49: Using t.Error() and t.Errorf() instead of testify assertions
• Line 91: Using t.Fatalf() for file read errors instead of require.NoError()
• Lines 97, 101, 105, 110, 114: Using t.Error() for string content checks

Recommended Changes:

// ❌ CURRENT (anti-pattern)
if !strings.Contains(stepsString, "setup_threat_detection.cjs") {
    t.Error("Expected threat detection to require setup_threat_detection.cjs file")
}

if strings.Contains(stepsString, "const templateContent = `# Threat Detection Analysis") {
    t.Error("Expected threat detection to read template from file, not pass it inline")
}

data, err := os.ReadFile(templatePath)
if err != nil {
    t.Fatalf("Failed to read threat detection template file: %v", err)
}

// ✅ IMPROVED (testify)
require.Contains(t, stepsString, "setup_threat_detection.cjs",
    "threat detection should require setup_threat_detection.cjs file")

assert.NotContains(t, stepsString, "const templateContent = `# Threat Detection Analysis",
    "threat detection should read template from file, not pass it inline")

data, err := os.ReadFile(templatePath)
require.NoError(t, err, "should read threat detection template file")

Why this matters: Testify provides clearer error messages, better test output, and is the standard used throughout this codebase (see scratchpad/testing.md). Using require.* for setup failures stops the test immediately, while assert.* continues checking other conditions.

2. Table-Driven Tests

Current Issues:

• TestThreatDetectionUsesFilePathNotInline checks multiple string conditions that could be table-driven
• TestThreatDetectionTemplateUsesFilePath has multiple similar string checks

Recommended Changes:

// ✅ IMPROVED - Table-driven test for string checks
func TestThreatDetectionStepGeneration(t *testing.T) {
    compiler := NewCompiler()
    data := &WorkflowData{
        Name:            "Test Workflow",
        Description:     "Test Description",
        MarkdownContent: "Test markdown content",
        SafeOutputs: &SafeOutputsConfig{
            ThreatDetection: &ThreatDetectionConfig{},
        },
    }

    steps := compiler.buildThreatDetectionSteps(data, "agent")
    stepsString := strings.Join(steps, "")

    tests := []struct {
        name        string
        shouldExist bool
        substring   string
        message     string
    }{
        {
            name:        "requires setup script",
            shouldExist: true,
            substring:   "setup_threat_detection.cjs",
            message:     "should require setup_threat_detection.cjs file",
        },
        {
            name:        "no inline template",
            shouldExist: false,
            substring:   "const templateContent = `# Threat Detection Analysis",
            message:     "should read template from file, not pass it inline",
        },
        {
            name:        "calls main without parameters",
            shouldExist: true,
            substring:   "await main()",
            message:     "should call main function without parameters",
        },
        {
            name:        "no environment variable for agent output",
            shouldExist: false,
            substring:   "AGENT_OUTPUT: ${{ needs.agent.outputs.output }}",
            message:     "should not pass agent output via environment variable",
        },
        {
            name:        "no old AGENT_OUTPUT replacement",
            shouldExist: false,
            substring:   ".replace(/{AGENT_OUTPUT}/g, process.env.AGENT_OUTPUT",
            message:     "should not replace {AGENT_OUTPUT} with environment variable",
        },
    }

    fo...

</details>



<!-- START COPILOT CODING AGENT SUFFIX -->

- Fixes github/gh-aw#14103

<!-- START COPILOT CODING AGENT TIPS -->
---

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more [Copilot coding agent tips](https://gh.io/copilot-coding-agent-tips) in the docs.

[Copilot]

Pull request overview

Refactors pkg/workflow/threat_detection_file_access_test.go to improve test readability and coverage around threat-detection step generation by adopting testify assertions, table-driven subtests, and shared helpers.

Changes:

• Migrates manual error/string checks to require.* (setup) and assert.* (validation).
• Refactors multiple string assertions into table-driven subtests with clearer intent.
• Adds helper constructors plus new coverage for step-generation helpers (buildDownloadArtifactStep, buildEchoAgentOutputsStep, buildThreatDetectionAnalysisStep).

---

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

[Copilot]

In the download-artifact step test, agent-output is hard-coded even though the production code uses constants.AgentOutputArtifactName. Using the constant here will keep the test aligned if the artifact name ever changes and avoids duplicating magic strings.