Skip to content

Add test coverage for concurrent artifact downloads in logs orchestrator#8679

Merged
pelikhan merged 3 commits intomainfrom
copilot/add-test-coverage-concurrent-downloads
Jan 2, 2026
Merged

Add test coverage for concurrent artifact downloads in logs orchestrator#8679
pelikhan merged 3 commits intomainfrom
copilot/add-test-coverage-concurrent-downloads

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Jan 2, 2026

The concurrent download implementation in logs_orchestrator.go lacks tests verifying panic recovery, result collection, resource cleanup, and concurrency limit enforcement.

Changes

Added pkg/cli/logs_orchestrator_test.go with 13 test cases covering:

  • Concurrent execution: Multiple downloads complete, all results collected regardless of completion order
  • Context cancellation: Pre-cancelled and mid-execution cancellation handled gracefully
  • Resource management: No goroutine leaks verified via goroutine counting
  • Concurrency limits: GH_AW_MAX_CONCURRENT_DOWNLOADS enforcement tested (1, 5, 10, 100)
  • Error propagation: Failed downloads and skipped runs tracked correctly
  • Edge cases: Empty input, mixed conclusions (success/failure/cancelled/timed_out/skipped), verbose mode
  • Result structure: LogsPath generation, DatabaseID preservation, field population

Implementation Notes

Tests verify orchestration behavior without mocking. Panic recovery is documented as automatically handled by the conc/pool library, which guarantees:

  • Panics in one task don't affect others
  • Stack traces preserved and re-raised after all tasks complete
  • Results returned in submission order
  • Graceful shutdown on context cancellation

All tests pass in ~1.8s without network dependencies.

Original prompt

This section details on the original issue you should resolve

<issue_title>[plan] Add test coverage for concurrent artifact downloads</issue_title>
<issue_description>## Objective

Add comprehensive test coverage for concurrent download behavior in logs_orchestrator.go to prevent regressions in panic recovery, result ordering, and resource cleanup.

Context

The concurrent download implementation lacks tests verifying:

  • Multiple downloads complete successfully
  • Panics in one task don't break others
  • Results maintain expected order
  • Proper resource cleanup on errors
  • Concurrency limits are respected

Approach

Create test file pkg/cli/logs_orchestrator_test.go with table-driven tests covering:

  1. Successful concurrent downloads

    • Multiple artifacts download in parallel
    • Results collected correctly
    • Order preserved
  2. Panic recovery

    • One download panics, others continue
    • Panic is re-raised after all complete
    • Stack traces preserved
  3. Result ordering

    • Results match submission order
    • No race conditions
  4. Resource cleanup

    • All goroutines complete
    • No leaks on errors
    • Proper cleanup on panics
  5. Concurrency limits

    • Max goroutines respected
    • Pool doesn't exceed limit

Files to Create/Modify

  • Create: pkg/cli/logs_orchestrator_test.go
  • Reference: pkg/cli/logs_orchestrator.go (implementation)

Test Structure

func TestConcurrentArtifactDownload(t *testing.T) {
    tests := []struct {
        name          string
        runs          []*WorkflowRun
        maxConcurrent int
        expectedErr   bool
        setup         func(*testing.T)
    }{
        {
            name: "successful parallel downloads",
            runs: createMockRuns(5),
            maxConcurrent: 3,
            expectedErr: false,
        },
        {
            name: "panic in one download",
            // ... test panic recovery
        },
        // ... more test cases
    }
    
    for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            // test implementation
        })
    }
}

Acceptance Criteria

AI generated by Plan Command for discussion #8587

Comments on the Issue (you are @copilot in this section)


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Copilot AI and others added 2 commits January 2, 2026 21:02
- Create logs_orchestrator_test.go with 13 test cases
- Test successful concurrent downloads with multiple artifacts
- Test result collection and ID verification
- Test context cancellation (pre-cancelled and partial)
- Test resource cleanup (no goroutine leaks)
- Test concurrency limit enforcement via environment variable
- Test LogsPath generation
- Test error handling during downloads
- Test mixed run conclusions (success, failure, cancelled, etc.)
- Test verbose mode consistency
- Test result structure validation
- Document panic recovery behavior (handled by conc pool)
- All tests passing with make test-unit

Co-authored-by: mnkiefer <8320933+mnkiefer@users.noreply.github.com>
…ds implemented

Co-authored-by: mnkiefer <8320933+mnkiefer@users.noreply.github.com>
Copilot AI changed the title [WIP] Add test coverage for concurrent artifact downloads Add test coverage for concurrent artifact downloads in logs orchestrator Jan 2, 2026
Copilot AI requested a review from mnkiefer January 2, 2026 21:04
@pelikhan pelikhan marked this pull request as ready for review January 2, 2026 21:05
@pelikhan pelikhan merged commit f38287b into main Jan 2, 2026
4 checks passed
@pelikhan pelikhan deleted the copilot/add-test-coverage-concurrent-downloads branch January 2, 2026 21:05
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.

[plan] Add test coverage for concurrent artifact downloads

3 participants