[GH-ISSUE #426] Harden test infrastructure and split internal/asc package #121

New issue

Closed

opened 2026-02-26 21:33:35 +03:00 by kerem · 0 comments

kerem commented

2026-02-26 21:33:35 +03:00

Owner

Originally created by @rudrankriyam on GitHub (Feb 5, 2026).
Original GitHub issue: https://github.com/rudrankriyam/App-Store-Connect-CLI/issues/426

Context

The codebase has grown to 60+ command domains, ~607 production files, and 173 test files in 18 days. The architecture and patterns are solid, but three infrastructure gaps are emerging as the surface area grows.

1. Systematic pagination test coverage

Problem: Pagination paths are tested case-by-case in individual commands. The PaginateAll engine in internal/asc/client_pagination.go is central to many commands, but most commands that use it don't test the multi-page path — they only test single-page responses. When we hardened the publish group resolver this week, we found the pagination path had zero test coverage.

Scope:

Audit all callers of asc.PaginateAll (there are at least 5-6: beta groups list, testflight sync, publish group resolver, beta testers, enabled versions, etc.)
For each caller, ensure there's at least one test that returns a links.next URL and verifies the second page is fetched and aggregated
Add a dedicated unit test for PaginateAll itself that covers: single page, multi-page, API error on page N, repeated URL detection (ErrRepeatedPaginationURL), and type mismatch between pages
Consider a shared test helper like paginatedResponse(page1Data, page2Data) to reduce boilerplate

Files to start with:

internal/asc/client_pagination.go — the engine
internal/cli/testflight/beta_groups.go — uses PaginateAll
internal/cli/testflight/testflight_sync.go — uses PaginateAll
internal/cli/publish/group_resolver.go — uses PaginateAll (already tested as of this week)

2. Split `internal/asc` into sub-packages

Problem: internal/asc is the HTTP client, type definitions, pagination engine, upload orchestrator, and domain-specific request/response types all in one package. As the API surface keeps growing, this creates:

Large files that are hard to navigate (e.g., client.go, client_types.go, client_options.go)
Tight coupling between unrelated concerns (upload chunking shouldn't need to know about Game Center types)
Harder onboarding for contributors scanning the package

Proposed structure:

internal/asc/
├── client.go              # Core Client struct, auth, HTTP helpers (stays)
├── client_http.go         # Request/response, BaseURL, do() (stays)
├── types/
│   ├── resources.go       # Resource[T], Response[T], Links, ResourceType constants
│   ├── apps.go            # App-related attribute types
│   ├── builds.go          # Build-related attribute types
│   ├── testflight.go      # BetaGroupAttributes, BetaTesterAttributes, etc.
│   ├── gamecenter.go      # Game Center types
│   ├── subscriptions.go   # IAP/subscription types
│   └── ...
├── pagination/
│   ├── paginate.go        # PaginateAll, PaginatedResponse interface
│   └── paginate_test.go
├── upload/
│   ├── operations.go      # ExecuteUploadOperations, chunked upload
│   ├── builds.go          # Build upload types and requests
│   └── ...
└── options/
    ├── beta_groups.go     # BetaGroupsOption, WithBetaGroupsLimit, etc.
    └── ...

Approach:

This is a large refactor. Do it incrementally — start by extracting types/ (the least coupled), then pagination/, then upload/
Use type aliases in the root asc package during migration so callers don't break
Each extraction should be its own PR with passing tests

Guideline: Read docs/GO_STANDARDS.md before starting. Prefer moving types first since they have no behavior and the compiler will catch all breakages.

3. Opt-in integration test harness against real API

Problem: All HTTP tests mock at the transport level (roundTripFunc), which is good for speed and determinism but doesn't catch real API contract changes. The internal/update package already has an ASC_UPDATE_INTEGRATION env-gated test — we should generalize this pattern.

Proposed design:

Gate all integration tests behind ASC_INTEGRATION_TEST=true (skip by default)
Use the FoundationLab app (ID 6747745091) as the sandbox target since it exists and has beta groups

Create a shared helper in internal/cli/cmdtest/ or a new internal/testutil/ package:

func SkipUnlessIntegration(t *testing.T) {
    if os.Getenv("ASC_INTEGRATION_TEST") != "true" {
        t.Skip("set ASC_INTEGRATION_TEST=true to run integration tests")
    }
}

Start with smoke tests for read-only operations (no mutations):
- asc apps list returns the expected app
- asc testflight beta-groups list --app 6747745091 returns groups
- asc builds list --app 6747745091 returns builds
- Group name resolution resolves "Alpha" and "Beta" to valid IDs
Add a CI workflow that runs integration tests on a schedule (not on every PR) using repository secrets for ASC_KEY_ID, ASC_ISSUER_ID, and ASC_PRIVATE_KEY_B64
Never test destructive operations (create/delete/submit) in integration — only reads and resolution logic

Files to reference:

internal/update/update_integration_test.go — existing pattern
internal/cli/cmdtest/game_center_enabled_versions_test.go — setupAuth() and roundTripFunc helpers to reuse

Priority order

Pagination tests — lowest risk, highest value, can be done independently
Integration test harness — medium effort, high confidence gain
Package split — largest effort, do last when the other two are stable

Labels

testing, refactor, infrastructure

Originally created by @rudrankriyam on GitHub (Feb 5, 2026). Original GitHub issue: https://github.com/rudrankriyam/App-Store-Connect-CLI/issues/426 ## Context The codebase has grown to 60+ command domains, ~607 production files, and 173 test files in 18 days. The architecture and patterns are solid, but three infrastructure gaps are emerging as the surface area grows. --- ## 1. Systematic pagination test coverage **Problem:** Pagination paths are tested case-by-case in individual commands. The `PaginateAll` engine in `internal/asc/client_pagination.go` is central to many commands, but most commands that use it don't test the multi-page path — they only test single-page responses. When we hardened the `publish` group resolver this week, we found the pagination path had zero test coverage. **Scope:** - Audit all callers of `asc.PaginateAll` (there are at least 5-6: beta groups list, testflight sync, publish group resolver, beta testers, enabled versions, etc.) - For each caller, ensure there's at least one test that returns a `links.next` URL and verifies the second page is fetched and aggregated - Add a dedicated unit test for `PaginateAll` itself that covers: single page, multi-page, API error on page N, repeated URL detection (`ErrRepeatedPaginationURL`), and type mismatch between pages - Consider a shared test helper like `paginatedResponse(page1Data, page2Data)` to reduce boilerplate **Files to start with:** - `internal/asc/client_pagination.go` — the engine - `internal/cli/testflight/beta_groups.go` — uses `PaginateAll` - `internal/cli/testflight/testflight_sync.go` — uses `PaginateAll` - `internal/cli/publish/group_resolver.go` — uses `PaginateAll` (already tested as of this week) --- ## 2. Split `internal/asc` into sub-packages **Problem:** `internal/asc` is the HTTP client, type definitions, pagination engine, upload orchestrator, and domain-specific request/response types all in one package. As the API surface keeps growing, this creates: - Large files that are hard to navigate (e.g., `client.go`, `client_types.go`, `client_options.go`) - Tight coupling between unrelated concerns (upload chunking shouldn't need to know about Game Center types) - Harder onboarding for contributors scanning the package **Proposed structure:** ``` internal/asc/ ├── client.go # Core Client struct, auth, HTTP helpers (stays) ├── client_http.go # Request/response, BaseURL, do() (stays) ├── types/ │ ├── resources.go # Resource[T], Response[T], Links, ResourceType constants │ ├── apps.go # App-related attribute types │ ├── builds.go # Build-related attribute types │ ├── testflight.go # BetaGroupAttributes, BetaTesterAttributes, etc. │ ├── gamecenter.go # Game Center types │ ├── subscriptions.go # IAP/subscription types │ └── ... ├── pagination/ │ ├── paginate.go # PaginateAll, PaginatedResponse interface │ └── paginate_test.go ├── upload/ │ ├── operations.go # ExecuteUploadOperations, chunked upload │ ├── builds.go # Build upload types and requests │ └── ... └── options/ ├── beta_groups.go # BetaGroupsOption, WithBetaGroupsLimit, etc. └── ... ``` **Approach:** - This is a large refactor. Do it incrementally — start by extracting `types/` (the least coupled), then `pagination/`, then `upload/` - Use type aliases in the root `asc` package during migration so callers don't break - Each extraction should be its own PR with passing tests **Guideline:** Read `docs/GO_STANDARDS.md` before starting. Prefer moving types first since they have no behavior and the compiler will catch all breakages. --- ## 3. Opt-in integration test harness against real API **Problem:** All HTTP tests mock at the transport level (`roundTripFunc`), which is good for speed and determinism but doesn't catch real API contract changes. The `internal/update` package already has an `ASC_UPDATE_INTEGRATION` env-gated test — we should generalize this pattern. **Proposed design:** - Gate all integration tests behind `ASC_INTEGRATION_TEST=true` (skip by default) - Use the FoundationLab app (ID `6747745091`) as the sandbox target since it exists and has beta groups - Create a shared helper in `internal/cli/cmdtest/` or a new `internal/testutil/` package: ```go func SkipUnlessIntegration(t *testing.T) { if os.Getenv("ASC_INTEGRATION_TEST") != "true" { t.Skip("set ASC_INTEGRATION_TEST=true to run integration tests") } } ``` - Start with smoke tests for read-only operations (no mutations): - `asc apps list` returns the expected app - `asc testflight beta-groups list --app 6747745091` returns groups - `asc builds list --app 6747745091` returns builds - Group name resolution resolves "Alpha" and "Beta" to valid IDs - Add a CI workflow that runs integration tests on a schedule (not on every PR) using repository secrets for `ASC_KEY_ID`, `ASC_ISSUER_ID`, and `ASC_PRIVATE_KEY_B64` - **Never** test destructive operations (create/delete/submit) in integration — only reads and resolution logic **Files to reference:** - `internal/update/update_integration_test.go` — existing pattern - `internal/cli/cmdtest/game_center_enabled_versions_test.go` — `setupAuth()` and `roundTripFunc` helpers to reuse --- ## Priority order 1. **Pagination tests** — lowest risk, highest value, can be done independently 2. **Integration test harness** — medium effort, high confidence gain 3. **Package split** — largest effort, do last when the other two are stable ## Labels `testing`, `refactor`, `infrastructure`