-
Notifications
You must be signed in to change notification settings - Fork 1
Collection fixture factory redeclared in 4 integration test files — 12 identical padding fields duplicated (Code Review Finding 016) #151
Description
Finding
Collection fixture factory (makeCollection/makeCSAPICollection/makeFullCollection) redeclared in 4 integration test files with 12 identical padding fields — ~180 lines of duplicated boilerplate.
Review Source: Senior developer code review of clean-pr
Severity: P3-Minor
Category: Code Quality
Ownership: Ours
Problem Statement
Each of the 4 integration test files declares its own local factory function that returns the same OgcApiCollectionInfo shape. The 12 padding fields required by the interface (itemFormats, bulkDownloadLinks, jsonDownloadLink, crs, itemCount, queryables, sortables, mapTileFormats, vectorTileFormats, supportedTileMatrixSets) are copy-pasted identically across all 4 factories. Adding a new required field to OgcApiCollectionInfo requires updating all 4 factories independently.
Affected code:
| File | Function | Lines |
|---|---|---|
src/ogc-api/csapi/integration/discovery.spec.ts |
makeCSAPICollection |
37–88 |
src/ogc-api/csapi/integration/observation.spec.ts |
makeCollection |
24–59 |
src/ogc-api/csapi/integration/command.spec.ts |
makeCollection |
33–68 |
src/ogc-api/csapi/integration/navigation.spec.ts |
makeFullCollection |
48–98 |
Scenario:
// discovery.spec.ts — makeCSAPICollection (37–88)
function makeCSAPICollection(overrides: Partial = {}): OgcApiCollectionInfo {
return {
links: [
{ rel: 'self', type: '', title: '', href: 'https://api.example.com/collections/weather' },
{ rel: 'ogc-cs:systems', type: '', title: '', href: '/systems' },
// ... 8 more CSAPI links
],
title: 'Weather Stations',
id: 'weather',
itemFormats: [], // ← identical in all 4
bulkDownloadLinks: {}, // ← identical in all 4
jsonDownloadLink: '', // ← identical in all 4
crs: [], // ← identical in all 4
itemCount: 0, // ← identical in all 4
queryables: [], // ← identical in all 4
sortables: [], // ← identical in all 4
mapTileFormats: [], // ← identical in all 4
vectorTileFormats: [], // ← identical in all 4
supportedTileMatrixSets: [], // ← identical in all 4
...overrides,
};
}
// observation.spec.ts — makeCollection (24–59) — same 12 padding fields
// command.spec.ts — makeCollection (33–68) — same 12 padding fields
// navigation.spec.ts — makeFullCollection (48–98) — same 12 padding fieldsdiscovery.spec.ts and navigation.spec.ts are particularly similar — both declare all 9 CSAPI resource links with only cosmetic differences (collection id/title/description).
Note: pipeline.spec.ts does NOT have this pattern — it uses inline raw JSON fixtures, not OgcApiCollectionInfo factories.
Impact: Maintenance burden — any change to OgcApiCollectionInfo required fields must be propagated to 4 different locations. No runtime risk (test-only code).
Ownership Verification
The entire src/ogc-api/csapi/integration/ directory is fork-only code — it does not exist in the upstream camptocamp/ogc-client repository.
$ git ls-tree upstream/main -- src/ogc-api/csapi/integration/
(empty — directory does not exist in upstream)
The factories were introduced in commits 1bd70b5 (Phase 4, Task 1 — Issue #31) and 66926d7 (integration test suites).
Conclusion: This code is ours — 100% fork-introduced.
Files to Modify
| File | Action | Est. Lines | Purpose |
|---|---|---|---|
src/ogc-api/csapi/integration/_fixtures.ts |
Create | ~30 | Shared PADDING object, ALL_CSAPI_LINKS array, makeFullCsapiCollection factory |
src/ogc-api/csapi/integration/discovery.spec.ts |
Modify | ~-50 | Replace local makeCSAPICollection with import from _fixtures.ts |
src/ogc-api/csapi/integration/observation.spec.ts |
Modify | ~-35 | Replace local makeCollection with import from _fixtures.ts |
src/ogc-api/csapi/integration/command.spec.ts |
Modify | ~-35 | Replace local makeCollection with import from _fixtures.ts |
src/ogc-api/csapi/integration/navigation.spec.ts |
Modify | ~-50 | Replace local makeFullCollection with import from _fixtures.ts |
Proposed Solutions
Option A: Create a shared _fixtures.ts (Recommended)
// src/ogc-api/csapi/integration/_fixtures.ts
import type { OgcApiCollectionInfo } from '../../model.js';
/** Padding fields required by OgcApiCollectionInfo but irrelevant to CSAPI tests. */
const PADDING: Pick<
OgcApiCollectionInfo,
| 'itemFormats' | 'bulkDownloadLinks' | 'jsonDownloadLink'
| 'crs' | 'itemCount' | 'queryables' | 'sortables'
| 'mapTileFormats' | 'vectorTileFormats' | 'supportedTileMatrixSets'
> = {
itemFormats: [],
bulkDownloadLinks: {},
jsonDownloadLink: '',
crs: [],
itemCount: 0,
queryables: [],
sortables: [],
mapTileFormats: [],
vectorTileFormats: [],
supportedTileMatrixSets: [],
};
/** All 9 CSAPI resource rel links. */
export const ALL_CSAPI_LINKS = [
{ rel: 'ogc-cs:systems', type: '', title: '', href: '/systems' },
{ rel: 'ogc-cs:deployments', type: '', title: '', href: '/deployments' },
{ rel: 'ogc-cs:procedures', type: '', title: '', href: '/procedures' },
{ rel: 'ogc-cs:samplingFeatures', type: '', title: '', href: '/samplingFeatures' },
{ rel: 'ogc-cs:properties', type: '', title: '', href: '/properties' },
{ rel: 'ogc-cs:datastreams', type: '', title: '', href: '/datastreams' },
{ rel: 'ogc-cs:observations', type: '', title: '', href: '/observations' },
{ rel: 'ogc-cs:controlStreams', type: '', title: '', href: '/controlStreams' },
{ rel: 'ogc-cs:commands', type: '', title: '', href: '/commands' },
];
/** Factory for test collections with all CSAPI resources. */
export function makeFullCsapiCollection(
overrides: Partial = {}
): OgcApiCollectionInfo {
return {
...PADDING,
links: [
{ rel: 'self', type: '', title: '', href: 'https://api.example.com/collections/test' },
...ALL_CSAPI_LINKS,
],
id: 'test',
title: 'Test Collection',
description: 'Test CSAPI collection',
...overrides,
};
}Each spec file imports makeFullCsapiCollection (or a subset-links variant) instead of redeclaring.
Pros: Eliminates ~180 lines of boilerplate; single point of update when OgcApiCollectionInfo changes; consistent fixture shape across all tests.
Cons: Adds one new file.
Effort: Small | Risk: None (test-only change)
Option B: Leave as-is
Each spec file keeps its own factory. The duplication is contained to test scaffolding.
Pros: Each test file is self-contained; no shared test dependencies.
Cons: Maintenance burden grows with each new required field; 4-way sync required for changes.
Effort: None | Risk: None
Scope — What NOT to Touch
- ❌ Do NOT modify
pipeline.spec.ts(uses a different fixture pattern) - ❌ Do NOT modify production code — this is a test-only refactor
- ❌ Do NOT change the
OgcApiCollectionInfointerface - ❌ Do NOT refactor individual test logic — only extract the factory
Acceptance Criteria
-
_fixtures.tscreated withPADDING,ALL_CSAPI_LINKS,makeFullCsapiCollection - All 4 spec files import from
_fixtures.tsinstead of declaring local factories - No spec file contains its own 12-field padding object
- All integration tests pass (
npm test) - No lint errors (
npm run lint) - All modified files pass
npx prettier --check
Dependencies
Blocked by: Nothing
Blocks: Nothing
Related: None — this is an isolated test hygiene improvement
Operational Constraints
⚠️ MANDATORY: Before starting work on this issue, reviewdocs/governance/AI_OPERATIONAL_CONSTRAINTS.md.
Key constraints:
- Precedence: OGC specifications → AI Collaboration Agreement → This issue description → Existing code → Conversational context
- No scope expansion: Extract the fixture factories, nothing more
- Minimal diffs: Prefer the smallest change that satisfies the acceptance criteria
- Ask when unclear: If intent is ambiguous, stop and ask for clarification
Ownership-Specific Constraints
This code is ours:
- Fix on the current working branch
- Include in the next commit to
clean-prif the PR is still open - Ensure all 81 integration tests continue to pass
References
Original Finding & Affected Source Files
| # | Document | What It Provides |
|---|---|---|
| 0 | docs/code-review/016-pending-p3-integration-test-fixture-duplication.md |
Original code review finding with full analysis |
| 1 | src/ogc-api/csapi/integration/discovery.spec.ts:37–88 |
makeCSAPICollection — factory with all 9 CSAPI links + 12 padding fields |
| 2 | src/ogc-api/csapi/integration/observation.spec.ts:24–59 |
makeCollection — factory with 4 observation-relevant links + 12 padding fields |
| 3 | src/ogc-api/csapi/integration/command.spec.ts:33–68 |
makeCollection — factory with 4 command-relevant links + 12 padding fields |
| 4 | src/ogc-api/csapi/integration/navigation.spec.ts:48–98 |
makeFullCollection — factory with all 9 CSAPI links + 12 padding fields |
| 5 | src/ogc-api/model.ts |
OgcApiCollectionInfo interface — defines the 12 padding fields that are duplicated |
OGC Standards (fixture link rel values come from these)
| # | Document | What It Provides |
|---|---|---|
| 6 | OGC 23-001 — CSAPI Part 1 | Defines Part 1 resource types whose ogc-cs: link relations appear in all 4 fixture factories (systems, deployments, procedures, samplingFeatures, properties) |
| 7 | OGC 23-002 — CSAPI Part 2 | Defines Part 2 resource types whose ogc-cs: link relations appear in the fixtures (datastreams, observations, controlStreams, commands) |
| 8 | OGC API — Common (19-072) | Defines the collection metadata pattern (OgcApiCollectionInfo) that produces the 12 padding fields — the root cause of the duplication |
| 9 | OGC API — Features (17-069r4) | Features API collection pattern that OgcApiCollectionInfo extends — explains why fields like itemFormats, queryables, sortables exist |
Upstream Library (defines OgcApiCollectionInfo)
| # | Document | What It Provides |
|---|---|---|
| 10 | camptocamp/ogc-client | Upstream library that defines the OgcApiCollectionInfo interface in model.ts — the interface whose required fields cause the padding duplication |
| 11 | PR #114 (EDR) Analysis | EDR implementation blueprint — documents upstream test fixture JSON file patterns, not TypeScript factory patterns. Provides precedent for how upstream organizes fixture data files, which indirectly informs _fixtures.ts structuring |
| 12 | Architecture Patterns Analysis | Documents upstream production architecture patterns for adding new OGC API support — focuses on production code organization, not test fixtures specifically. Provides structural context that test file organization must align with |
| 13 | File Organization Strategy | Documents file naming, placement, and organization conventions — governs where _fixtures.ts should live and how it should be named |
| 14 | Code Reuse vs Duplication Strategy | Upstream code reuse vs duplication strategy for production code — duplication thresholds and extraction criteria are applicable by analogy to test fixture factory extraction, though the document focuses on production rather than test code |
| 15 | TypeScript Type System Design | Documents upstream TypeScript type patterns including Partial<T> usage — provides general type organization context, though does not discuss Pick<T,K> specifically (the typing proposed for the PADDING constant) |
| 16 | QueryBuilder Pattern Analysis | Upstream QueryBuilder implementation pattern — documents how QueryBuilder uses collection links for URL construction. Does not discuss test fixtures, but explains why integration test fixtures need specific link subsets to exercise the QueryBuilder |
| 17 | Upstream Library Expectations | Defines what camptocamp/ogc-client expects — test quality and organization standards the _fixtures.ts must meet |
Requirements Research (fixture content derives from these)
| # | Document | What It Provides |
|---|---|---|
| 18 | Full Implementation Scope Definition | Defines complete implementation scope for all 9 CSAPI resource types, format parsers, and capability requirements — establishes which resources exist and their relationships, providing context for what a complete collection fixture should represent |
| 19 | Conformance and Capability Requirements | Conformance detection patterns — documents all conformance class URIs and /conformance endpoint detection, establishing which conformance URIs test fixtures must include to trigger capability detection |
| 20 | Sub-Resource Navigation Requirements | Nested navigation patterns and link relation types — documents all @link suffix relations (subsystems@link, datastreams@link, controlstreams@link, etc.) and sub-resource endpoint structures that fixtures must model for multi-hop navigation |
| 21 | CRUD Operations Requirements | CRUD operation matrix — documents all HTTP operations for all resources including ControlStream/Command endpoints, schema validation constraints, and cascade delete semantics that CRUD test fixtures must support |
| 22 | Query Parameter Requirements | Complete query parameter catalog — integration tests exercise query parameters that depend on correct fixture collection setup |
| 23 | Usage Scenarios and Priorities | 15 core usage scenarios and 6 essential workflows — defines which resource type combinations (systems+datastreams+observations, controlstreams+commands+status) are exercised in real-world use, informing which resource links fixtures should include |
| 24 | CSAPI Part 1 Requirements | Complete Part 1 specification analysis — defines all 5 Part 1 resource types, their required associations (subsystems, samplingFeatures, datastreams, controlstreams), and endpoint patterns that determine which resource links must be modeled in fixtures |
| 25 | CSAPI Part 2 Requirements | Complete Part 2 specification analysis — defines all 6 Part 2 dynamic data resource types and their associations (DataStreams→Observations, ControlStreams→Commands→Status/Result), informing which Part 2 resource links must be modeled in fixtures |
| 26 | Gap Analysis from Previous Iteration | Lessons from failed iteration — identifies gaps in format parsing and validation coverage from v1, establishing comprehensive implementation requirements; indirectly informs fixture design by defining which resource types and format variations the implementation must handle |
| 27 | Lessons Learned from Previous Iterations | Mistakes to avoid — warns against over-engineering (unnecessary abstractions, excess code) and under-engineering (trivial tests, poor test quality); relevant to ensuring fixture extraction is right-sized rather than over-abstracted or under-tested |
Design Research (architecture context)
| # | Document | What It Provides |
|---|---|---|
| 28 | CSAPIQueryBuilder Architecture Decisions | Definitive architecture decisions for CSAPIQueryBuilder class design (single-class pattern, helper method reuse, file organization). Determines the QueryBuilder API surface that fixtures must support, but does not discuss fixture shapes directly |
| 29 | Collections Reader Research | Defines the OgcApiCollectionInfo type structure and CSAPI collection detection logic. Shows which fields the type requires (explaining why fixture factories must populate many fields), but does not specifically discuss 'padding fields' — that observation comes from comparing fixture factories against this type |
| 30 | OGCAPIEndpoint Integration Research | Factory method and conformance detection — integration tests exercise this path, fixtures must satisfy the detection logic |
| 31 | CSAPI Architecture Decisions | 9 resource type architecture with URL patterns, CRUD operations, sub-resource relationships, and format support. Explains the resource landscape that fixture factories must represent, but does not specifically discuss different link subsets for different test suites |
Testing Research (directly governs test fixture design)
| # | Document | What It Provides |
|---|---|---|
| 32 | Testing Strategy Research | High-level testing strategy research plan (outline/stubs only). Poses fixture organization as a research question among many others, but defers specifics to Section 15 (fixture sourcing) and Section 19 (test file structure). Does not directly govern fixture sharing strategy |
| 33 | Testing Research Plan 12 — QueryBuilder Testing Strategy | QueryBuilder method testing patterns — integration tests use collection fixtures to instantiate QueryBuilders |
| 34 | Testing Research Plan 13 — Resource Method Testing Patterns | Resource method test patterns — defines how fixtures should support per-resource test scenarios |
| 35 | Testing Research Plan 14 — Integration Test Workflow Design | Integration test workflow design defining 4 workflow categories (Discovery, Observation, Command, Cross-resource navigation) with scenarios and fixture requirements. Relevant as the design basis for integration tests, but describes workflow categories — the mapping to specific integration test files that share fixture factories is an implementation detail not covered here |
| 36 | Testing Research Plan 15 — Fixture Sourcing Organization | Fixture sourcing and organization for ~280 test data files (JSON/CSV), covering naming, directory placement, and reusability. Governs data fixture conventions, but focuses on spec-sourced data files — not on TypeScript fixture factory modules (_fixtures.ts) that programmatically construct test objects. The _fixtures.ts sharing pattern from Finding 016 is a code organization concern not directly addressed |
| 37 | Testing Research Plan 19 — Test Organization File Structure | Test file structure and naming conventions (flat colocated .spec.ts files, describe/it block patterns). Governs where test files go generally, and its flat colocated structure convention informs placement, but does not specifically address shared fixture factory modules like _fixtures.ts |
| 38 | Testing Strategy Review — Phase 2f: Integration Workflow Category | Quality gate review of 4 integration/workflow research documents, evaluating client-orientation, anti-pattern compliance, and scope proportionality. Validates research quality for integration test design, but does not specifically address fixture management, deduplication, or sharing strategy |
| 39 | Testing Strategy Review — Phase 0: Lessons from Failed Attempt | Lessons from failed testing approach identifying 5 anti-patterns around testing server behavior instead of client code. Discusses fixture usage philosophy (fixtures as controlled test inputs via mocked fetch), which is tangentially relevant, but does not address fixture code architecture or the specific problem of duplicated fixture factory code |
Testing Documentation
| # | Document | What It Provides |
|---|---|---|
| 40 | Test Fixtures Guide | Fixture file creation and management process — the definitive guide for how data fixture files (JSON/XML response files in fixtures/) should be organized, named, and maintained. Does not cover in-code test fixture factories (the duplication target of this finding), but establishes organizational precedent |
Planning Documents
| # | Document | What It Provides |
|---|---|---|
| 41 | CSAPI Implementation Guide | Architecture reference for the CSAPI module — provides context for integration test design and fixture requirements |
| 42 | Contribution Goal and Definition | PR acceptance criteria — high-level quality standards (>80% coverage, type safety, zero-breaking-change) that any code change must satisfy. Does not specifically address fixture architecture |
| 43 | Phase 6 Implementation Guide | Phase 6 refactoring specifications — defines Prettier/ESLint formatting standards (§4.1–4.2) that any new file including _fixtures.ts must comply with. Does not mention _fixtures.ts directly (it does not yet exist) |
| 44 | Phase 6 Contribution Goal and Definition | PR acceptance readiness definition for Phase 6 module boundary refactoring — CI compliance gates (C1–C5: Prettier, typecheck, ESLint, test suites) apply to all changes including fixture refactoring. Architectural gates (A1–A4) are specific to CSAPI decoupling, not fixture extraction |
Governance Documents
| # | Document | What It Provides |
|---|---|---|
| 45 | AI Operational Constraints | Operational boundaries — mandatory review before implementation |
| 46 | AI Collaboration Agreement | Collaboration governance framework — defines authority model, drift prevention philosophy, and review expectations for AI-assisted development. Applies indirectly to all code changes through its precedent hierarchy and scope control principles, not through direct code quality standards |
| 47 | Phase 2 Lessons Learned | Phase 2 process lessons — Lesson 7 (DRY violations compound across issues) provides general precedent for extracting shared code, but the document does not discuss fixture management or fixture extraction strategy specifically |
| 48 | Issue Creation Template — Code Review | Template used to create this issue — ensures structural consistency |
Implementation Records
| # | Document | What It Provides |
|---|---|---|
| 49 | Phase 5.5 Code Review | Phase 5.5 code review — provides context for code quality expectations but did not identify integration test fixture duplication or collection factory patterns relevant to this finding |
| 50 | Final Project Code Review | End-to-end code review prior to upstream submission — identifies format handler code duplication (F-NEW-04) but does not mention integration test fixture factory duplication. This specific duplication was not noted in this review |
| 51 | Outstanding Findings Status Report | Outstanding findings status — confirms by omission that this fixture duplication was not previously identified or deferred. None of the 5 open issues or 47 tracked findings relate to integration test fixture factories |
Upstream PR
| # | Document | What It Provides |
|---|---|---|
| 52 | Upstream PR #136 | The upstream contribution — includes the 4 integration test files; any fix here must be forward-ported to clean-pr |