Skip to content

test(analysis): compact generated reference geometry - #1361

Merged
Mavrick-F merged 1 commit into
opengeos:mainfrom
Mavrick-F:chore/compact-reference-fixtures
Nov 3, 2026
Merged

Mavrick-F merged 1 commit into
opengeos:mainfrom
Mavrick-F:chore/compact-reference-fixtures

Conversation

@Mavrick-F

@Mavrick-F Mavrick-F commented Nov 3, 2026 •

Copy link
Copy Markdown
Member

Summary

The generated analysis reference outputs, including the buffer outlines added in #1161, spend much of their size on indentation. Write each expected feature on one compact JSON line, keeping feature boundaries visible and retaining every coordinate digit.

The 21 expected-output files shrink from 1,177,604 to 660,899 bytes (43.9%), and from 69,252 to 209 lines. Parsed JSON and all non-whitespace characters match the baseline exactly. Geometry, numeric precision, test tolerances and application behavior are unchanged. This reduces fixture storage; it does not claim faster application performance.

Testing

  • Compared all changed GeoJSON files against the baseline: parsed values and every non-whitespace character match exactly.
  • uv run crates/analysis/tests/reference/generate.py --check
  • cargo xtask ci
  • cargo build --locked --target wasm32-unknown-unknown -p rustgis-web
  • pre-commit run --all-files

UI evidence

No visible UI change.

Related issue

Follow-up to #1161.

Summary by CodeRabbit

  • Tests
    • Updated expected GeoJSON outputs to compact formatting. Most fixtures retain their data; the buffer-station outlines now close back to their starting coordinates.
    • Test output generation now uses the same compact format.

@coderabbitai

coderabbitai Bot commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Walkthrough

The reference generator now writes compact GeoJSON, and expected fixtures use that format. The buffer station fixture also adds coordinates that complete both polygon rings.

Changes: GeoJSON reference output

Layer / File(s) Summary
Compact expected serialization
crates/analysis/tests/reference/generate.py
write_expected writes compact JSON for each feature and separates features with newlines.
Updated reference fixtures
crates/analysis/tests/reference/expected/*.geojson
Expected fixtures use compact formatting. The buffer station fixture also adds coordinates that complete both polygon rings.

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Other

Possibly related PRs

Suggested reviewers: giswqs

Pre-merge checks: Passed 5
Check name Status Explanation
Title check Passed The title clearly and concisely describes the main change: compacting generated analysis reference geometry.
Description check Passed The description includes all required sections. It explains the change, reports testing, states that there is no visible UI change, and identifies the related issue.
Docstring Coverage Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 1 functions across 1 files. (16 skipped: 16 unsupported.)

Finishing Touches
Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Warning

Some tools did not complete. Review the errors below.

ast-grep (0.51.1)

ast-grep could not parse rule config: /ast-grep-rules/ast-grep-essentials/rules/swift/security/insecure-biometrics-swift.yml

Review coverage is incomplete: 5 files could not be fully reviewed. Findings from completed review steps are included; see review info for details.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

A rabbit checks each ring with care,
Then packs the features neat and spare.
Newlines mark where features end,
While closed loops meet their start again.
The GeoJSON is ready to share.

CodeRabbit Verify

  • Run CodeRabbit Verify to test the changes in this pull request.
Recent review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: ac710276-1b70-4b05-9db0-05c56be70214
Commits

Reviewing files that changed from the base of the PR and between 53e06c6 and 8d291a6.

Files selected for processing: 22
  • crates/analysis/tests/reference/expected/buffer_long_5km.geojson
  • crates/analysis/tests/reference/expected/buffer_parcels_-2km.geojson
  • crates/analysis/tests/reference/expected/buffer_parcels_1km.geojson
  • crates/analysis/tests/reference/expected/buffer_points_1km.geojson
  • crates/analysis/tests/reference/expected/buffer_roads_2km.geojson
  • crates/analysis/tests/reference/expected/buffer_stations_120km.geojson
  • crates/analysis/tests/reference/expected/clip_parcels.geojson
  • crates/analysis/tests/reference/expected/clip_roads.geojson
  • crates/analysis/tests/reference/expected/dissolve_all.geojson
  • crates/analysis/tests/reference/expected/dissolve_code.geojson
  • crates/analysis/tests/reference/expected/erase_parcels.geojson
  • crates/analysis/tests/reference/expected/erase_roads.geojson
  • crates/analysis/tests/reference/expected/hull_all.geojson
  • crates/analysis/tests/reference/expected/hull_each.geojson
  • crates/analysis/tests/reference/expected/identity_parcels.geojson
  • crates/analysis/tests/reference/expected/identity_roads.geojson
  • crates/analysis/tests/reference/expected/intersect_parcels.geojson
  • crates/analysis/tests/reference/expected/intersect_roads.geojson
  • crates/analysis/tests/reference/expected/symdiff_parcels.geojson
  • crates/analysis/tests/reference/expected/union_parcels.geojson
  • crates/analysis/tests/reference/expected/update_parcels.geojson
  • crates/analysis/tests/reference/generate.py
Files not fully reviewed because their changes were unavailable: 5
  • crates/analysis/tests/reference/expected/buffer_parcels_-2km.geojson
  • crates/analysis/tests/reference/expected/buffer_points_1km.geojson
  • crates/analysis/tests/reference/expected/buffer_parcels_1km.geojson
  • crates/analysis/tests/reference/expected/buffer_long_5km.geojson
  • crates/analysis/tests/reference/expected/buffer_roads_2km.geojson

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 5 remain after this review.

Comment on lines +284 to +286
text = '{"type":"FeatureCollection","features":[\n' + ",\n".join(
json.dumps(f, separators=(",", ":")) for f in feats
) + "\n]}"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit (low severity, latent): with no features, ",\n".join(...) is empty, so this writes a blank line inside the array: {"type":"FeatureCollection","features":[ / empty line / ]}. It is still valid JSON, and no current case produces an empty file, so this is not blocking. A small fix that keeps output byte-identical for non-empty layers (so --check is unaffected):

Suggested change
text = '{"type":"FeatureCollection","features":[\n' + ",\n".join(
json.dumps(f, separators=(",", ":")) for f in feats
) + "\n]}"
body = ",\n".join(json.dumps(f, separators=(",", ":")) for f in feats)
text = '{"type":"FeatureCollection","features":[' + ("\n" + body if feats else "") + "\n]}"

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed with an empty feature list: the output remains valid JSON and parses to an empty FeatureCollection. No current fixture is empty. Leaving this formatting nit for the release quality pass.

@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

Code review

Scope note: pr-context/diff.patch is truncated at 1.5 MB. Only the first five expected files (buffer_long_5km, buffer_parcels_-2km, buffer_parcels_1km, buffer_points_1km, buffer_roads_2km) are visible in it. I read generate.py at the PR head instead, and checked the other expected files on disk.

Bugs: None found.

  • generate.py writes the compact format with separators=(",", ":"). Python's json uses shortest round-trip float repr, so no digits are lost. reference.rs parses these files with serde_json, so the new layout is safe on the Rust side.
  • No expected file still has the old indented layout. A search for indented lines or "type": returned zero matches across expected/.
  • Spot check: buffer_long_5km.geojson still contains the first baseline coordinate 4.988800519641332,45.955698960411645.
  • I could not verify the "parsed JSON matches baseline" claim for the 16 files whose diffs are truncated, and I did not run generate.py --check (no shell).

Security: None. The change is test fixtures plus a Python writer with no external input.

Performance: None. The format change reduces fixture size by about 43%, as the PR states.

Quality:

  • Low, latent (inline comment posted on generate.py:284-286): with an empty feature list, write_expected writes a blank line inside the array. It is still valid JSON, and no current case is empty. The suggested fix keeps non-empty output byte-identical, so --check is unaffected.
  • Maintainability trade-off (not posted): one feature per line keeps the file size down, but a change to a single coordinate now rewrites that feature's whole line. The old indented layout gave line-granular diffs. This is a reasonable trade for generated fixtures, but reviewers should expect noisier diffs on regeneration.

AGENTS.md: No violations found.

  • This is a fixture and writer change only, so the no-panic, never-silent, wasm, i18n, and dependency rules do not apply.
  • The PR's testing section lists generate.py --check and cargo xtask ci. I did not re-run them.

Confidence: High that the format change is correct and the Rust side is unaffected. Medium-high on the byte-level equivalence claim, since I could only spot-check it.

Prior reviews: The thread list is empty, so there were no earlier concerns to skip.

Change size

Compared with the base branch: the whole PR, merge base 8ebc4ce to PR head 8d291a6.

Measure Value
Files changed 22 (0 added, 22 modified, 0 removed, 0 renamed)
Size before 1.15 MB (1,204,958 B)
Size after 656.6 KB (688,405 B)
Total size change -502.1 KB (-516,553 B)
Path Before After Change
buffer_parcels_1km.geojson 292.0 KB 163.0 KB -129.0 KB
buffer_points_1km.geojson 244.6 KB 138.2 KB -106.4 KB
buffer_roads_2km.geojson 226.3 KB 127.9 KB -98.5 KB
buffer_long_5km.geojson 181.4 KB 102.1 KB -79.2 KB
buffer_parcels_-2km.geojson 91.2 KB 51.4 KB -39.8 KB
buffer_stations_120km.geojson 48.8 KB 27.6 KB -21.3 KB
union_parcels.geojson 11.9 KB 6.4 KB -5.5 KB
identity_parcels.geojson 8.3 KB 4.5 KB -3.8 KB
symdiff_parcels.geojson 8.2 KB 4.4 KB -3.8 KB
update_parcels.geojson 6.8 KB 3.4 KB -3.3 KB
erase_parcels.geojson 4.6 KB 2.5 KB -2.1 KB
dissolve_code.geojson 3.7 KB 1.8 KB -1.8 KB
dissolve_all.geojson 3.3 KB 1.5 KB -1.8 KB
intersect_parcels.geojson 3.8 KB 2.0 KB -1.7 KB
hull_each.geojson 3.6 KB 2.0 KB -1.7 KB
clip_parcels.geojson 3.1 KB 1.7 KB -1.4 KB
identity_roads.geojson 3.2 KB 1.9 KB -1.3 KB
intersect_roads.geojson 1.8 KB 1.1 KB -712 B
erase_roads.geojson 1.5 KB 893 B -602 B
clip_roads.geojson 1.4 KB 840 B -537 B

The table lists the 20 largest changes. The remaining two files, generate.py and one more, are not in largest_changes.

@Mavrick-F

Copy link
Copy Markdown
Member Author

The local comparison covered all 21 changed expected files against baseline 8ebc4ce, without a diff-size limit: both parsed JSON and every non-whitespace character matched exactly. The generator regeneration/check, full CI (3,390 tests), wasm build and hooks passed before publication.

@Mavrick-F

Copy link
Copy Markdown
Member Author

The walkthrough says the station buffer adds coordinates to complete rings. That fixture is unchanged after parsing against baseline 8ebc4ce, as are all 21 expected files; no coordinates or ring closures were added. The local check also compared every non-whitespace character, without GitHub diff-size limits. The five unavailable diffs are covered by that check and the passing generator consistency workflow.

@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

📖 Docs preview

Item Value
Preview https://preview.rustgis.app/pr-1361/
Web app https://preview.rustgis.app/pr-1361/app/
Commit 8d291a6

GitHub Pages takes a minute or two to publish a new commit. Newer commits from the fork aren't published until a maintainer adds the preview label (once per commit).

@Mavrick-F

Copy link
Copy Markdown
Member Author

@coderabbitai approve

@coderabbitai

coderabbitai Bot commented Nov 3, 2026

Copy link
Copy Markdown
Contributor
Action performed

Comments resolved and changes approved.

@Mavrick-F
Mavrick-F merged commit 9794edd into opengeos:main Nov 3, 2026
19 checks passed
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