Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions pkg/cli/contribution_check_workflow_contract_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
)

func TestContributionCheckWorkflowSafeOutputContract(t *testing.T) {
t.Parallel()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

These two top-level tests now run in parallel while both read live workflow source and compiled lock files from the repository root, which makes them coupled to any other test mutating those files and undermines the “safe to parallelize” claim in the PR.

💡 Why this should stay serial

The justification in the PR body says these files were only parallelized when they had no shared paths or cross-test ordering concerns, but both tests here resolve the same repo root and inspect the same workflow artifacts on disk. That is not isolated test input; it is shared process-visible state.

Even if this file’s two tests only read today, marking them parallel bakes in hidden coupling to the rest of the package test suite and makes future workflow-rewriting tests much easier to break. The conservative move is to leave repository-fixture readers serial unless the input is copied into t.TempDir() first.

func TestContributionCheckWorkflowSafeOutputContract(t *testing.T) {
    // keep serial, or copy the workflow files into a temp fixture first
}

repoRoot, err := gitutil.FindGitRoot()
if err != nil {
t.Skipf("Skipping test: not in a git repository: %v", err)
Expand All @@ -36,6 +37,7 @@ func TestContributionCheckWorkflowSafeOutputContract(t *testing.T) {
}

func TestContributionCheckWorkflowAllowsRequiredShellCommands(t *testing.T) {
t.Parallel()
repoRoot, err := gitutil.FindGitRoot()
if err != nil {
t.Skipf("Skipping test: not in a git repository: %v", err)
Expand Down
9 changes: 9 additions & 0 deletions pkg/cli/copilot_agent_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
)

func TestCopilotCodingAgentDetector_IsGitHubCopilotCodingAgent(t *testing.T) {
t.Parallel()
tests := []struct {
name string
setupFunc func(string) error
Expand Down Expand Up @@ -96,6 +97,7 @@ func TestCopilotCodingAgentDetector_IsGitHubCopilotCodingAgent(t *testing.T) {

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
// Create temporary directory for test
tmpDir, err := os.MkdirTemp("", "copilot-agent-test-*")
if err != nil {
Expand Down Expand Up @@ -125,6 +127,7 @@ func TestCopilotCodingAgentDetector_IsGitHubCopilotCodingAgent(t *testing.T) {
}

func TestParseCopilotCodingAgentLogMetrics(t *testing.T) {
t.Parallel()
tests := []struct {
name string
logContent string
Expand Down Expand Up @@ -213,6 +216,7 @@ Task step 1 complete

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
metrics := ParseCopilotCodingAgentLogMetrics(tt.logContent, false)

if tt.expectedTurns > 0 && metrics.Turns != tt.expectedTurns {
Expand All @@ -235,6 +239,7 @@ Task step 1 complete
}

func TestExtractToolName(t *testing.T) {
t.Parallel()
tests := []struct {
name string
line string
Expand Down Expand Up @@ -311,6 +316,7 @@ func TestExtractToolName(t *testing.T) {

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
result := extractToolName(tt.line)
if result != tt.expected {
t.Errorf("Expected '%s', got '%s'", tt.expected, result)
Expand All @@ -320,6 +326,7 @@ func TestExtractToolName(t *testing.T) {
}

func TestIntegration_CopilotCodingAgentWithAudit(t *testing.T) {
t.Parallel()
// Create a temporary directory that simulates a GitHub Copilot coding agent run
// NOTE: GitHub Copilot coding agent runs do NOT have aw_info.json (that's for agentic workflows)
tmpDir, err := os.MkdirTemp("", "copilot-agent-integration-*")
Expand Down Expand Up @@ -373,6 +380,7 @@ Tool call: github_create_pr
}

func TestReadLogHeader(t *testing.T) {
t.Parallel()
tmpDir, err := os.MkdirTemp("", "log-header-test-*")
if err != nil {
t.Fatalf("Failed to create temp dir: %v", err)
Expand All @@ -397,6 +405,7 @@ func TestReadLogHeader(t *testing.T) {
}

func TestWorkflowLogMetricsConversion(t *testing.T) {
t.Parallel()
// Test that our metrics are compatible with workflow.LogMetrics
logContent := `
Task iteration 1
Expand Down
3 changes: 3 additions & 0 deletions pkg/cli/copilot_metrics_fix_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import (
// The fix adds an explicit filepath.SkipDir return when the walk visits a directory
// named "workflow-logs", so only the agent artifact files are counted.
func TestExtractLogMetricsExcludesWorkflowLogsDir(t *testing.T) {
t.Parallel()
tempDir := t.TempDir()

// Simulate a Copilot-CLI run directory
Expand Down Expand Up @@ -72,6 +73,7 @@ func TestExtractLogMetricsExcludesWorkflowLogsDir(t *testing.T) {
// counted "User:"/"Human:"/"Query:" patterns that do not appear in Copilot CLI debug logs.
// The fix counts each "[DEBUG] data:" block as one API response (one turn).
func TestCopilotDebugLogTurnsExtraction(t *testing.T) {
t.Parallel()
tempDir := t.TempDir()

awInfoContent := `{"engine_id": "copilot"}`
Expand Down Expand Up @@ -143,6 +145,7 @@ func TestCopilotDebugLogTurnsExtraction(t *testing.T) {
// TestCopilotDebugLogMultipleToolCalls verifies that multiple "Executing tool:" lines
// produce correct call counts in ToolCalls.
func TestCopilotDebugLogMultipleToolCalls(t *testing.T) {
t.Parallel()
tempDir := t.TempDir()

awInfoContent := `{"engine_id": "copilot"}`
Expand Down
4 changes: 4 additions & 0 deletions pkg/cli/copilot_token_extraction_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
// 2. Log file contains JSON blocks with token usage
// 3. extractLogMetrics correctly parses and accumulates token counts
func TestCopilotTokenExtractionFromLogs(t *testing.T) {
t.Parallel()
tempDir := t.TempDir()

// Create aw_info.json with copilot engine
Expand Down Expand Up @@ -107,6 +108,7 @@ func TestCopilotTokenExtractionFromLogs(t *testing.T) {

// TestCopilotTokenExtractionWithSingleResponse tests extraction with just one API response
func TestCopilotTokenExtractionWithSingleResponse(t *testing.T) {
t.Parallel()
tempDir := t.TempDir()

// Create aw_info.json with copilot engine
Expand Down Expand Up @@ -142,6 +144,7 @@ func TestCopilotTokenExtractionWithSingleResponse(t *testing.T) {

// TestCopilotTokenExtractionWithNoUsageData tests when logs don't contain usage data
func TestCopilotTokenExtractionWithNoUsageData(t *testing.T) {
t.Parallel()
tempDir := t.TempDir()

// Create aw_info.json with copilot engine
Expand Down Expand Up @@ -171,6 +174,7 @@ func TestCopilotTokenExtractionWithNoUsageData(t *testing.T) {
// TestCopilotTokenExtractionWithRealLogData tests token extraction with actual log data
// from workflow run 20696085597 (Smoke Copilot test)
func TestCopilotTokenExtractionWithRealLogData(t *testing.T) {
t.Parallel()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This test now runs in parallel even though it depends on a fixed host path outside the test sandbox, so another concurrent test/job touching the same /tmp/run-20696085597/... tree can make it nondeterministic.

💡 Why this is a real flake risk

TestCopilotTokenExtractionWithRealLogData is no longer self-contained once t.Parallel() is added: it reads from a hard-coded absolute path instead of test-local fixtures. That means the test now races any other parallel work in the same process or runner that creates, deletes, or rewrites that tree, and the failure mode is exactly the kind of CI-only heisenbug that is painful to reproduce.

A safer fix is to keep this specific test serial, or better, move the sample log under a testdata fixture so the test only reads repository-controlled input.

func TestCopilotTokenExtractionWithRealLogData(t *testing.T) {
    // no t.Parallel() until the hard-coded external path is removed
}

// This test validates real log data if available
realLogPath := "/tmp/run-20696085597/sandbox/agent/logs/session-dd1eedf4-2b6d-4373-942c-1447d5a6e00a.log"

Expand Down
8 changes: 8 additions & 0 deletions pkg/cli/drain3_integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
)

func TestBuildDrain3Insights_NoEvents(t *testing.T) {
t.Parallel()
// A ProcessedRun with no meaningful events should return no insights.
processedRun := ProcessedRun{}
metrics := MetricsData{}
Expand All @@ -20,6 +21,7 @@ func TestBuildDrain3Insights_NoEvents(t *testing.T) {
}

func TestBuildDrain3Insights_BasicRun(t *testing.T) {
t.Parallel()
processedRun := ProcessedRun{
Run: WorkflowRun{
DatabaseID: 42,
Expand Down Expand Up @@ -51,6 +53,7 @@ func TestBuildDrain3Insights_BasicRun(t *testing.T) {
}

func TestBuildDrain3Insights_WithErrors(t *testing.T) {
t.Parallel()
processedRun := ProcessedRun{
Run: WorkflowRun{
DatabaseID: 99,
Expand Down Expand Up @@ -83,6 +86,7 @@ func TestBuildDrain3Insights_WithErrors(t *testing.T) {
}

func TestBuildDrain3Insights_StageSequenceEvidence(t *testing.T) {
t.Parallel()
processedRun := ProcessedRun{
Run: WorkflowRun{
DatabaseID: 7,
Expand Down Expand Up @@ -111,6 +115,7 @@ func TestBuildDrain3Insights_StageSequenceEvidence(t *testing.T) {
}

func TestBuildDrain3InsightsMultiRun_Empty(t *testing.T) {
t.Parallel()
insights := buildDrain3InsightsMultiRun(nil)
assert.Empty(t, insights, "expected no insights for nil runs slice")

Expand All @@ -119,6 +124,7 @@ func TestBuildDrain3InsightsMultiRun_Empty(t *testing.T) {
}

func TestBuildDrain3InsightsMultiRun_MultipleRuns(t *testing.T) {
t.Parallel()
runs := []ProcessedRun{
{
Run: WorkflowRun{
Expand Down Expand Up @@ -167,6 +173,7 @@ func TestBuildDrain3InsightsMultiRun_MultipleRuns(t *testing.T) {
}

func TestBuildAgentEventsFromProcessedRun(t *testing.T) {
t.Parallel()
pr := ProcessedRun{
Run: WorkflowRun{
DatabaseID: 5,
Expand Down Expand Up @@ -200,6 +207,7 @@ func TestBuildAgentEventsFromProcessedRun(t *testing.T) {
}

func TestBuildDrain3Insights_IncludedInAuditData(t *testing.T) {
t.Parallel()
// Verify that buildAuditData appends drain3 insights to ObservabilityInsights.
processedRun := ProcessedRun{
Run: WorkflowRun{
Expand Down
12 changes: 12 additions & 0 deletions pkg/cli/effective_tokens_compliance_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import (
// T-ET-001: Single invocation with all four token classes produces correct base_weighted_tokens.
// Spec §4.3: base_weighted_tokens = (w_in × I) + (w_cache × C) + (w_out × O) + (w_reason × R)
func TestETCompliance_T_ET_001_SingleInvocationBaseWeightedTokens(t *testing.T) {
t.Parallel()
weights := types.TokenClassWeights{
Input: 1.0,
CachedInput: 0.1,
Expand All @@ -46,6 +47,7 @@ func TestETCompliance_T_ET_001_SingleInvocationBaseWeightedTokens(t *testing.T)
// T-ET-002: Single invocation ET equals m × base_weighted_tokens.
// Spec §4.4: effective_tokens = m × base_weighted_tokens
func TestETCompliance_T_ET_002_SingleInvocationEffectiveTokens(t *testing.T) {
t.Parallel()
weights := types.TokenClassWeights{
Input: 1.0,
CachedInput: 0.1,
Expand All @@ -61,6 +63,7 @@ func TestETCompliance_T_ET_002_SingleInvocationEffectiveTokens(t *testing.T) {
// T-ET-003: Zero-value token classes do not affect the result.
// Spec §4.3: zero-valued classes contribute zero to the sum.
func TestETCompliance_T_ET_003_ZeroValueTokenClasses(t *testing.T) {
t.Parallel()
weights := types.TokenClassWeights{
Input: 1.0,
CachedInput: 0.1,
Expand All @@ -75,6 +78,7 @@ func TestETCompliance_T_ET_003_ZeroValueTokenClasses(t *testing.T) {
// T-ET-004: Custom weights are applied when default weights are overridden.
// Spec §4.2: implementations MAY override default weights but MUST disclose them.
func TestETCompliance_T_ET_004_CustomWeightsApplied(t *testing.T) {
t.Parallel()
custom := types.TokenClassWeights{
Input: 2.0, // overridden
CachedInput: 0.5, // overridden
Expand All @@ -93,6 +97,7 @@ func TestETCompliance_T_ET_004_CustomWeightsApplied(t *testing.T) {
// T-ET-010: Multi-invocation ET_total equals the sum of per-invocation ET values.
// Spec §5.1: ET_total = Σ (m_i × base_weighted_tokens_i)
func TestETCompliance_T_ET_010_MultiInvocationETTotal(t *testing.T) {
t.Parallel()
weights := types.TokenClassWeights{Input: 1.0, CachedInput: 0.1, Output: 4.0, Reasoning: 4.0}

// Invocation 1: model-a, m=2.0, I=500, C=200, O=150, R=0 → base=1120, ET=2240
Expand All @@ -114,6 +119,7 @@ func TestETCompliance_T_ET_010_MultiInvocationETTotal(t *testing.T) {
// T-ET-011: raw_total_tokens equals the sum of all raw tokens across all invocations.
// Spec §5.2: raw_total_tokens = Σ (I_i + C_i + O_i + R_i)
func TestETCompliance_T_ET_011_RawTotalTokens(t *testing.T) {
t.Parallel()
// Invocation 1: I=500, C=200, O=150, R=0 → raw=850
// Invocation 2: I=300, C=0, O=100, R=0 → raw=400
// Invocation 3: I=200, C=100, O=250, R=0 → raw=550
Expand All @@ -125,6 +131,7 @@ func TestETCompliance_T_ET_011_RawTotalTokens(t *testing.T) {
// T-ET-012: total_invocations count includes root, sub-agents, and tool-triggered calls.
// Spec §5.3: all invocations (root + sub-agents + tool-triggered) MUST be counted.
func TestETCompliance_T_ET_012_TotalInvocationsCount(t *testing.T) {
t.Parallel()
// Simulated invocation list: 1 root + 2 sub-agents = 3 total
invocationIDs := []string{"root", "retrieval", "synthesis"}
assert.Len(t, invocationIDs, 3, "T-ET-012: total_invocations must include root + all sub-agents")
Expand All @@ -137,6 +144,7 @@ func TestETCompliance_T_ET_012_TotalInvocationsCount(t *testing.T) {
// T-ET-020: Root node has parent_id = null.
// Spec §6.2: the root invocation MUST have parent_id = null.
func TestETCompliance_T_ET_020_RootNodeParentIDNull(t *testing.T) {
t.Parallel()
type invocationNode struct {
ID string
ParentID *string
Expand All @@ -148,6 +156,7 @@ func TestETCompliance_T_ET_020_RootNodeParentIDNull(t *testing.T) {
// T-ET-021: All sub-agent nodes reference a valid parent_id.
// Spec §6.3: each sub-agent invocation MUST reference a valid parent_id.
func TestETCompliance_T_ET_021_SubAgentParentIDValid(t *testing.T) {
t.Parallel()
parentID := "root"
type invocationNode struct {
ID string
Expand All @@ -163,6 +172,7 @@ func TestETCompliance_T_ET_021_SubAgentParentIDValid(t *testing.T) {
// usage.input_tokens, usage.cached_input_tokens, usage.output_tokens,
// usage.reasoning_tokens, derived.base_weighted_tokens, derived.effective_tokens.
func TestETCompliance_T_ET_022_NodeSchemaRequiredFields(t *testing.T) {
t.Parallel()
type modelInfo struct {
Name string `json:"name"`
CopilotMultiplier float64 `json:"copilot_multiplier"`
Expand Down Expand Up @@ -207,6 +217,7 @@ func TestETCompliance_T_ET_022_NodeSchemaRequiredFields(t *testing.T) {
// T-ET-030: Summary object is present in all conforming responses.
// Spec §7: a conforming response MUST include a summary object.
func TestETCompliance_T_ET_030_SummaryObjectPresent(t *testing.T) {
t.Parallel()
type summaryObject struct {
TotalInvocations int `json:"total_invocations"`
RawTotalTokens int `json:"raw_total_tokens"`
Expand All @@ -233,6 +244,7 @@ func TestETCompliance_T_ET_030_SummaryObjectPresent(t *testing.T) {
// T-ET-031: Summary values are consistent with per-invocation data.
// Spec §7: summary.effective_tokens MUST equal Σ per-invocation effective_tokens.
func TestETCompliance_T_ET_031_SummaryConsistentWithInvocations(t *testing.T) {
t.Parallel()
weights := types.TokenClassWeights{Input: 1.0, CachedInput: 0.1, Output: 4.0, Reasoning: 4.0}

perInvocationET := []float64{
Expand Down
Loading