Skip to content

Add cross-binary integration for OSM ingest → API HTTP read#93

Merged
koinsaari merged 1 commit into
mainfrom
test/cross-binary-osm-api-integration
Jun 8, 2026
Merged

Add cross-binary integration for OSM ingest → API HTTP read#93
koinsaari merged 1 commit into
mainfrom
test/cross-binary-osm-api-integration

Conversation

@koinsaari

Copy link
Copy Markdown
Contributor

Summary

  • Adds one integration test that drives a real OSM ingest, then reads the resulting place over real HTTP through the full middleware stack
  • Catches drift in the shared persistence shape between cmd/ingestion and cmd/api (GORM tags, JSONB scanners, response converters) that single-binary integration tests cannot
  • Pinned POI matches cmd/ingestion/main_integration_test.go — any fixture or transformer change flushes both tests in the same PR

Test plan

  • `go vet -tags integration ./...` clean
  • `golangci-lint run --build-tags integration ./cmd/api/...` clean (only pre-existing issues in places_integration_test.go remain)
  • `go test -tags integration -timeout 300s ./cmd/api/...` — 65 passed
  • `go test ./...` — 254 passed

🤖 Generated with Claude Code

@koinsaari koinsaari changed the title test(api): cross-binary integration for OSM ingest → API HTTP read Add cross-binary integration for OSM ingest → API HTTP read Jun 8, 2026
@claude

claude Bot commented Jun 8, 2026

Copy link
Copy Markdown

Claude finished @koinsaari's task in 2m 36s —— View job


Code Review

  • Gather context (PR metadata, changed files)
  • Read REVIEW.md for review instructions
  • Read the new test file
  • Read related test files for comparison
  • Review the implementation
  • Post review findings
  • Set commit status (skipped — 403, integration lacks statuses:write)

No blocking issues. The test correctly catches persistence-shape drift between the ingestion and API binaries by writing via place.UpsertBatch (the ingestion path) and reading via a live HTTP request through the full middleware stack. All core invariants pass.

What the test exercises

The structure is sound:

  1. runOSMIngestForCrossBinary directly calls the shared internal/ packages (place.UpsertBatch, identity.Sweeper) — the same code paths cmd/ingestion uses at runtime.
  2. handlerForServer (with auth middleware, not handlerNoAuth) stands up the full cmd/api stack — consistent with other GET-endpoint tests such as TestHandleGetPlace_ReturnsPlaceWithAccessibility.
  3. The pinned osm_id=521143390 assertion matches cmd/ingestion/main_integration_test.go's pinnedPOIs[0], so any transformer or fixture change flushes both tests together.

🟡 Nits

1. Sweep error should carry a diagnostic message (cmd/api/cross_binary_integration_test.go:70)

// current
if _, err := sweeper.Sweep(ctx, touched); err != nil {
    t.Logf("sweep: %v", err)
}

A sweep panic would surface as a confusing failure elsewhere. A short comment explaining why this is non-fatal (// sweep is opportunistic; the place is already persisted) would help future readers. Alternatively, t.Logf("sweep non-fatal (place already persisted): %v", err) makes the intent clear.

2. batchSize = 1000 is re-declared rather than imported (cmd/api/cross_binary_integration_test.go:35)

The canonical batch size lives in cmd/ingestion. A diverging value (e.g. tuned for memory) won't propagate here. The constant doesn't affect test correctness, but a comment noting it is intentionally decoupled would prevent a future reader from wondering if it needs to match.

3. Lat/lng are not verified after HTTP decode (cmd/api/cross_binary_integration_test.go:99–136)

Coordinates come through the geometry JSONB column too, and a broken Geometry.Scan would silently produce (0, 0). A two-line check would close that gap without needing exact values:

if got.Lat == 0 && got.Lng == 0 {
    t.Errorf("lat/lng both zero — geometry may not have scanned correctly")
}

4. Pluck("id", &touched) collects all DB IDs (cmd/api/cross_binary_integration_test.go:59–61)

After a fresh truncate + ingest this is equivalent to collecting RETURNING id values from UpsertBatch, so it's correct. A brief comment would prevent a reader from wondering if it's accidentally sweeping stale rows (// all IDs after fresh ingest equal the touched set).

Plus one similar item: the pinned OSM node ID 521143390 is hardcoded at line 84 rather than via a named constant — minor, and the PR description documents the coupling.


Runs the real OSM source against the Andorra fixture, then GETs the pinned
Supermercat Saint Moritz place over real HTTP through the full middleware
stack. Catches drift in the shared persistence shape between cmd/ingestion
and cmd/api that single-binary integration tests cannot.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@koinsaari koinsaari force-pushed the test/cross-binary-osm-api-integration branch from 0244ea5 to 4db18fd Compare June 8, 2026 15:29
@InWheelOrg InWheelOrg deleted a comment from claude Bot Jun 8, 2026
@koinsaari koinsaari merged commit 6cb07d6 into main Jun 8, 2026
8 of 9 checks passed
@koinsaari koinsaari deleted the test/cross-binary-osm-api-integration branch June 8, 2026 15:33
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.

1 participant