Skip to content

Migrate legacy EWKT tests, document decoding, and support curved geometries - #46

Merged
Alexandre-T merged 2 commits into
mainfrom
14-story-migrate-legacy-ewkt-parser-tests-and-document-ewkt-decoding
Oct 5, 2026
Merged

Alexandre-T merged 2 commits into
mainfrom
14-story-migrate-legacy-ewkt-parser-tests-and-document-ewkt-decoding

Conversation

@Alexandre-T

Copy link
Copy Markdown
Contributor

This migrates legacy EWKT parser scenarios to the public decoder API and documents decoding capabilities. The branch also adds CircularString and CompoundCurve decoding for WKT/EWKT, corrects WKT collection dimension inheritance, and updates spatial-types to 1.0.0-beta.

Closes #14.

Changes

  • Add identifiable legacy scenarios under tests/Legacy/, included by the standard PHPUnit suite.
  • Cover curved geometry decoding, collection dimensions, and invalid structures with current unit tests.
  • Document EWKT usage, supported geometry types, dimensions, EMPTY values, and implementation limitations in README.

Validation

  • composer test: passed, 652 tests and 4,389 assertions (PHP 8.5.11).
  • git diff --check: passed.
  • composer quality: failed. PHP CS Fixer identified formatting changes in eight legacy files; PHPStan then reported six errors in tests/Legacy/CircularStringTest.php and tests/Legacy/Utils/SpecificTestCase.php. Later quality steps were not reached. Automatic formatting edits from this verification were reverted so the published branch remains unchanged.

Items for independent review

The PR is published for independent assessment; it is not a claim that the pre-merge Definition of Done is satisfied.

@Alexandre-T Alexandre-T linked an issue Oct 5, 2026 that may be closed by this pull request
11 tasks
@codecov-commenter

codecov-commenter commented Oct 5, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.43%. Comparing base (7716725) to head (95102b9).

Additional details and impacted files
@@            Coverage Diff            @@
##               main      #46   +/-   ##
=========================================
  Coverage     99.43%   99.43%           
  Complexity      252      252           
=========================================
  Files            31       31           
  Lines           534      534           
=========================================
  Hits            531      531           
  Misses            3        3           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Alexandre-T

Copy link
Copy Markdown
Contributor Author

Independent review — CHANGES REQUIRED

Reviewed revision: 4c0a3159871c0a74d3dabebea4b1f56edb03a7be (remote HEAD rechecked). The PR is not ready for merge.

BLOCKING findings

  1. Required quality and CI checks fail — tests/Legacy/CircularStringTest.php:47, tests/Legacy/Utils/SpecificTestCase.php:112,133, and eight legacy files. Independently running PHPStan level 9 reproduces six errors: float fixture types, incorrectly typed collection entries/recursive arguments, and arithmetic on mixed. The PHP 8.4 CI fails PHP CS Fixer on eight files (run). The committed diff also has trailing whitespace at ExceptionTest.php:48. This violates the final acceptance criterion and governance's pre-merge quality requirements. Correct the types and formatting without suppressions, then run the complete quality suite and obtain green CI.

  2. Required legacy directory is absent — tests/Legacy/. Story Story: Migrate legacy EWKT parser tests and document EWKT decoding #14 explicitly requires tests/Legacy/EwktParser/. Move the migrated suite and update its namespaces/references while retaining discovery by the standard test suite.

  3. Legacy lexer coverage bypasses the requested public decoder API — tests/Legacy/LexerTest.php:134–173. These cases instantiate Strategy\Common\Lexer and assert token implementation details rather than exercising Decoder. This does not meet “Migrated tests use the public spatial-decoder API.” Adapt the compatible scenarios into public decode assertions; document any token-only scenarios that cannot be migrated. Internal lexer unit tests can remain separately if useful.

  4. Intentional legacy adaptations are incompletely documented — tests/Legacy/ParserTest.php, TriangleTest.php, Utils/SpecificTestCase.php, and README. Comparison with the local wkt-parser suite establishes preserved scenario names, but also changed exception classes/messages, replacement of the no-argument parser scenario by decode(''), and normalization of absent SRID from null to 0. Numeric conversion, triangle nesting and two excluded unsupported-type assertions have some inline explanation; the other changes have no migration explanation or source revision mapping. Add a concise migration inventory recording source revision, retained scenarios, changed expectations and reasons, and exclusions. This is required by “Any intentionally modified or excluded legacy behavior is documented”; merely changing expected values is not documentation of that decision.

  5. Production WKT changes exceed the approved story scope — WktParserFactory.php:74–92, GeometryCollectionWktParser.php:72, curve parsers/factories, composer.json:17, README. Story Story: Migrate legacy EWKT parser tests and document EWKT decoding #14 explicitly excludes WKT support, while this PR adds WKT curve support and changes accepted WKT collection dimension inheritance. It also upgrades the exact spatial-types dependency and changes previously unsupported curve behavior. Split these production changes into approved work items, or obtain an explicit scope/contract decision from Alexandre and update the work item before re-review. Migration scope alone does not authorize changes to supported representations or compatibility policy. API/Breaking Gates apply if the expanded behavior and changed WKT acceptance are retained; no such approval is recorded in Story: Migrate legacy EWKT parser tests and document EWKT decoding #14.

Verification and pre-merge Definition of Done

  • Independently ran composer test: 652 tests, 4,389 assertions, PASS (PHP 8.5.11). PHPUnit discovers tests/, including legacy cases.
  • Independently reproduced six PHPStan errors. CI independently confirms the committed formatting failures. The shared working tree acquired eight formatting-only modifications during review; they were neither accepted as the reviewed revision nor reverted by this reviewer. Local dry-run formatting results after that point cannot establish the published HEAD's formatting status.
  • CI on the reviewed SHA: PHP 8.4 quality workflow FAIL; PHP 8.5 workflow PASS; GitGuardian PASS. Full quality success is therefore not established.
  • Inspected production diff, current tests, legacy source comparisons, README, and governance/architecture/serialization/standards skills. No applicable repository/ancestor AGENTS.md was found. SQL/MM curve grammar was checked against the available ISO/IEC CD 13249-3:201x(E), 5.1.57, including <single curve text>; EWKT remains explicitly classified as an extension. This review does not certify exhaustive normative conformance of the additional out-of-scope feature.
  • README covers EWKT types, XY/XYZ/XYM/XYZM, EMPTY, limitations and public Decoder examples. New unit tests cover curves, invalid structures, dimensions and recursive SRID behavior. Representation logic remains in the decoder and construction delegates domain invariants to spatial-types; no cross-repository code relocation is introduced.
  • Functional/traceability acceptance, complete quality, green required CI and approved scope/compatibility are still outstanding. Merge and post-merge Done verification have not occurred and are outside this review.

NON-BLOCKING observation

The PR description says story #14 is Blocked, but GitHub Projects currently shows In review. Update that statement for accuracy.

Final outcome: CHANGES REQUIRED. This is a review outcome, not an Issue status or declaration of Done.

@Alexandre-T
Alexandre-T force-pushed the 14-story-migrate-legacy-ewkt-parser-tests-and-document-ewkt-decoding branch 3 times, most recently from 929dceb to ab77f0a Compare October 5, 2026 16:33
@Alexandre-T

Copy link
Copy Markdown
Contributor Author

Independent review — READY FOR MERGE

Reviewed and rechecked remote HEAD: b65a8644b1211863f55468bc60e66492ca4ef998.

No blocking finding or unresolved gate remains for the current PR diff and the Product Owner's approved decisions. This review supersedes my earlier CHANGES REQUIRED assessment of older revisions.

Acceptance criteria and approved decisions

  • Historical scenarios were compared with wkt-parser revision 7d3c8fd5fd061c44349acc6bc8de8e995371b168; scenario/provider names remain identifiable under the required tests/Legacy/EwktParser/ directory.
  • Migrated decoding scenarios exercise the public Decoder API. The Product Owner explicitly approved retaining the two internal Lexer tests and their historical provider. The new legacy README records this exception and explains why token types, values, positions and lexer reuse cannot be represented by public geometry results. This is an explicit approved exception, not an assertion that Lexer is public API.
  • The README documents changed exception classes/messages, replacement of no-argument parsing with empty-string rejection, and absent SRID adaptation from null to zero. Existing inline notes explain numeric conversion, triangle nesting and exclusions for now-implemented types. These adaptations distinguish historical coverage from current authoritative unit tests.
  • The global PHPMD ExcessiveMethodLength exclusion has been removed. The approved suppression applies only to the historical tokenData() provider, as explicitly requested by the Product Owner. The shared rule remains enabled for other methods.
  • Standard PHPUnit discovery includes the migrated suite. Existing public documentation covers EWKT geometry types, dimensions, EMPTY handling, limitations and Decoder usage examples.
  • WKT/curve production changes and dependency changes are already part of main and no longer belong to this PR's diff. No new production API, compatibility, architectural or normative change requires a gate in the remaining migration/documentation work.

Independent validation

  • PHPUnit: 652 tests, 4,389 assertions — PASS on PHP 8.5.11.
  • PHPStan level 9: PASS.
  • PHP CS Fixer, dry-run with cache disabled: PASS.
  • PHPMD production and tests using the committed rules: PASS.
  • PHP_CodeSniffer: PASS.
  • git diff --check: PASS.
  • Remote CI on the reviewed SHA: PHP 8.4 workflow — PASS, PHP 8.5 workflow — PASS, GitGuardian — PASS.

The working tree was clean before and after verification, and local HEAD matched remote HEAD. Checks were run without applying code fixes. Local PHPMD dependencies emit PHP 8.5 deprecation notices but return success.

The applicable pre-merge Definition of Done has been considered: acceptance criteria with the explicit approved lexical exception, compatibility scope, automated coverage, required quality checks, CI, documentation, independent review and final revision correspondence are satisfied. Governance, architecture, serialization and spatial standards guidance were applied; this remaining diff introduces no new spatial semantics.

Non-blocking observation

The PR description still describes older failures, the old legacy directory, production changes now in main, and a Blocked project status. It should be refreshed for accuracy; current story status is In review. This does not change the verified implementation result.

Final outcome: READY FOR MERGE. This is not a declaration that story #14 is Done. Alexandre's merge decision and post-merge Definition of Done verification remain outstanding. No merge was performed.

…and improve exception message assertions

tests: update exception messages for WKT parsing and geometry validation

tests: update exception messages for unimplemented spatial types in legacy tests

feat: decode CircularString in WKT and EWKT

tests: update legacy tests to maintain float values and improve exception messages

feat: decode CircularString in WKT and EWKT

tests: update legacy exception tests to reflect implemented geometry types

refactor: update type hints for coordinates to include float in legacy tests

refactor: update exception handling in SpecificTestCase to use custom InvalidCoordinateException

refactor: enhance test cases with detailed descriptions and setup/teardown methods

docs: explain historical EWKT test adaptations

docs: update legacy EWKT test expectations and exception handling descriptions

refactor: suppress PHPMD warning for excessive method length in tokenData method
@Alexandre-T
Alexandre-T force-pushed the 14-story-migrate-legacy-ewkt-parser-tests-and-document-ewkt-decoding branch from b65a864 to 95102b9 Compare October 5, 2026 18:20
@Alexandre-T
Alexandre-T merged commit 6c2a03b into main Oct 5, 2026
3 checks passed
@Alexandre-T
Alexandre-T deleted the 14-story-migrate-legacy-ewkt-parser-tests-and-document-ewkt-decoding branch October 5, 2026 18:21
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.

Story: Migrate legacy EWKT parser tests and document EWKT decoding

2 participants