From cd7c88591f5f735119302e6836e984c7dbd32390 Mon Sep 17 00:00:00 2001 From: Cameron Brooks Date: Sun, 6 Sep 2026 01:19:05 -0400 Subject: [PATCH] feat!: a third outcome for the differ, and the org exit-code contract Resolves adjudication A3. For a differ too, silence must never read as no difference. has_changes is a boolean, so it was false both for two identical boards and for a board carrying geometry the engine does not model. A stroke drawn with a macro or block aperture is not modelled -- documented as a known limitation, but invisible at runtime. A board with an added trace therefore reported 0 changes at exit 0, with the JSON byte-identical to comparing a board against a copy of itself, while the raster engine reported the change. Two engines, one question, opposite answers, and the geometry engine gave the dangerous one. The engine now records what it could not model, per reason. A layer carrying any of it is indeterminate, and the comparison reports identical | different | indeterminate. different outranks indeterminate, so one unmodellable stroke cannot mask a trace that moved. The reasons are named on stderr even under --quiet: suppressing them is the failure being reported on. Indeterminate exits 2 without waiting for --fail-on-diff. That flag chooses whether a difference is a failure; it has no bearing on whether the tool could look. BREAKING CHANGE: the exit-code contract now matches partspec and netspec. 0 no differences, 1 differences (with --fail-on-diff), 2 could not complete the comparison, 4 an input could not be read or parsed, 64 usage (EX_USAGE). Previously 2 meant a parse error, and 1 doubled as both "differences found" and "could not write the output file". The geometry report schema goes 2 -> 3, additively. --- CHANGELOG.md | 29 +++++ docs/cli.md | 17 +++ docs/geometry-diff.md | 8 +- docs/schema.md | 21 ++- gerberdiff/cli.py | 71 +++++++--- gerberdiff/export/json_report.py | 8 +- gerberdiff/geometry/driver.py | 8 ++ gerberdiff/geometry/layer_geometry.py | 53 +++++++- gerberdiff/geometry/types.py | 49 +++++++ tests/test_cli_diff.py | 6 +- tests/test_cli_geomdiff.py | 12 +- tests/test_cli_render.py | 2 +- tests/test_geometry_report.py | 8 +- tests/test_indeterminate.py | 178 ++++++++++++++++++++++++++ 14 files changed, 433 insertions(+), 37 deletions(-) create mode 100644 tests/test_indeterminate.py diff --git a/CHANGELOG.md b/CHANGELOG.md index dd69292..0675e47 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,34 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- **BREAKING -- the exit-code contract now matches partspec and netspec** + ([A3]). `0` no differences, `1` differences (with `--fail-on-diff`), `2` the + comparison could not be completed, `4` an input could not be read or parsed, + `64` usage (`EX_USAGE`). Previously `2` meant a parse error and `1` doubled as + both "differences found" and "could not write the output file". A gate written + `[ $? -eq 0 ]` is unaffected; one that treated `2` as a parse failure must now + read `4`. +- **Geometry report schema `version` 2 -> 3**, adding `summary.outcome`, + `summary.unrepresented` and a per-layer `unrepresented`. Additive: every v2 + field keeps its meaning. + +### Added + +- **`geomdiff` has a third outcome, and says what it could not model** ([A3]) -- + `identical` | `different` | `indeterminate`. A stroke drawn with a macro or + block aperture is not modelled by the geometry engine, and used to vanish: a + board with an added trace reported `0 changes` at exit `0`, with JSON + byte-identical to comparing a board against a copy of itself, while the raster + engine reported the change. The two engines answered the same question + differently and the geometry one gave the dangerous answer. Such operations are + now counted in `summary.unrepresented`, named on stderr, and make the outcome + `indeterminate` at exit `2` -- which does not wait for `--fail-on-diff`, since + that flag chooses whether a *difference* fails, not whether the tool could look. + `different` outranks `indeterminate`, so one unmodellable stroke cannot mask a + trace that moved. + ### Fixed - **Draw operations using an undefined aperture are no longer dropped in @@ -686,6 +714,7 @@ merge_tolerance) -> SingleLayerDiff`. - 2 smoke tests in `tests/test_scaffold.py`. [#17]: https://github.com/heibench/gerberdiff/issues/17 +[A3]: https://heibench.com/adjudications.html [Unreleased]: https://github.com/heibench/gerberdiff/compare/v0.29.1...HEAD [0.29.1]: https://github.com/heibench/gerberdiff/compare/v0.29.0...v0.29.1 [0.29.0]: https://github.com/heibench/gerberdiff/compare/9ffd4c8f...v0.29.0 diff --git a/docs/cli.md b/docs/cli.md index e6032f2..bb457c9 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -1,5 +1,22 @@ # CLI reference + +## Exit codes + +Shared across the org's tools, so a script can branch on them the same way for +`gerberdiff`, `partspec` and `netspec`. + +| code | meaning | +| ---- | -------- | +| `0` | no differences, and everything was modelled | +| `1` | differences found (with `--fail-on-diff`) | +| `2` | part of the comparison could not be made -- **not** a statement about the boards | +| `4` | an input could not be read or parsed -- also not a statement about the boards | +| `64` | usage (`EX_USAGE`): bad arguments, or an output file that exists without `--overwrite` | + +`2` does not wait for `--fail-on-diff`. That flag chooses whether a *difference* is a +failure; it has no bearing on whether the tool could look. + ## `parse` -- inspect a single file ```sh diff --git a/docs/geometry-diff.md b/docs/geometry-diff.md index 67947ef..13930e7 100644 --- a/docs/geometry-diff.md +++ b/docs/geometry-diff.md @@ -158,8 +158,14 @@ documented so the two engines' outputs are interpreted correctly: ## Known limitations -- Strokes drawn with macro or block apertures are skipped (the raster +- Strokes drawn with macro or block apertures are not modelled (the raster engine draws them as hairlines; neither engine models them properly). + **They are reported, not skipped in silence.** Each one is counted in + `summary.unrepresented`, makes the layer `indeterminate`, and gives the + comparison an `outcome` of `"indeterminate"` at exit `2`. This limitation was + documented here long before the output admitted to it: a board with such a + stroke reported `0 changes` at exit `0`, with JSON byte-identical to comparing + a board against a copy of itself, while the raster engine reported the change. - `rerouted` classification is deferred: a redrawn trace reports as removed + added segments (or `resized` when endpoints stay close). - Geometry changes are op-granular: a moved multi-op footprint reports diff --git a/docs/schema.md b/docs/schema.md index 8ba8c80..e97976e 100644 --- a/docs/schema.md +++ b/docs/schema.md @@ -98,7 +98,7 @@ All coordinate values are in **inches**. --- -## Schema version 2 (geometry diff) +## Schema version 3 (geometry diff) Coordinates (`centroid_x`, `centroid_y`) are in **inches** (matching v1); areas are in **mm^2** and displacements in **mm**. @@ -107,7 +107,7 @@ areas are in **mm^2** and displacements in **mm**. | Field | Type | Description | | ------------ | ---------------------------- | -------------------------------------------------- | -| `version` | `integer` | Always `2`. | +| `version` | `integer` | Always `3`. | | `generator` | `string` | Always `"gerberdiff"`. | | `mode` | `string` | Always `"geometry"`. | | `summary` | `object` | Aggregate counts (see below). | @@ -121,6 +121,13 @@ areas are in **mm^2** and displacements in **mm**. | `changed_layers` | `integer` | Layers with any change, non-matched status, or area. | | `total_changes` | `integer` | Total attributed change records across all layers. | | `has_changes` | `boolean` | `true` when anything differs. | +| `outcome` | `string` | `"identical"` \| `"different"` \| `"indeterminate"`. **Branch on this, not on `has_changes`.** | +| `unrepresented` | `object` | Reason -> count of operations the engine could not model, summed across layers. Empty when the comparison was complete. | + +`has_changes` cannot express "could not tell": it is `false` both for two identical +boards and for a board carrying geometry the engine does not model. `outcome` is +`"indeterminate"` in the second case, and the CLI exits `2`. `"different"` outranks +`"indeterminate"`, so a change that *was* found is still reported as one. ### `tolerances` object @@ -161,10 +168,16 @@ areas are in **mm^2** and displacements in **mm**. ```json { - "version": 2, + "version": 3, "generator": "gerberdiff", "mode": "geometry", - "summary": { "changed_layers": 1, "total_changes": 2, "has_changes": true }, + "summary": { + "changed_layers": 1, + "total_changes": 2, + "has_changes": true, + "outcome": "different", + "unrepresented": {} + }, "tolerances": { "move_tol_mm": 0.005, "gate_radius_mm": 0.2, diff --git a/gerberdiff/cli.py b/gerberdiff/cli.py index 945fd1b..e4ad9f3 100644 --- a/gerberdiff/cli.py +++ b/gerberdiff/cli.py @@ -11,6 +11,20 @@ from gerberdiff.diff.layer_matcher import EXCELLON_SUFFIXES from gerberdiff.types import Diagnostic, DiagnosticSeverity, LayerStatus +# The exit-code contract, aligned with partspec and netspec across the org (A1/A3). +# 2 is where "could not tell" belongs; a differ needs it as much as a checker does, +# because what the engine could not model must never be reported as no difference. +EXIT_OK = 0 +"""No differences, and everything was modelled.""" +EXIT_DIFFERENT = 1 +"""Differences found (with --fail-on-diff).""" +EXIT_INDETERMINATE = 2 +"""Part of the comparison could not be made. Not a statement about the boards.""" +EXIT_ERROR = 4 +"""Could not read or parse an input. Not a statement about the boards either.""" +EXIT_USAGE = 64 +"""EX_USAGE: bad arguments, or an output file that exists without --overwrite.""" + _MEMORY_WARN_PIXELS = 16_777_216 # 4096^2 @@ -39,7 +53,7 @@ def parse_cmd(file: Path, dump_ir: bool, quiet: bool, verbose: bool) -> None: content = file.read_text(errors="replace") except OSError as exc: click.echo(f"error: {exc}", err=True) - sys.exit(1) + sys.exit(EXIT_ERROR) if file.suffix.lower() in EXCELLON_SUFFIXES: img = parse_excellon(content, source_path=file) @@ -89,7 +103,7 @@ def parse_cmd(file: Path, dump_ir: bool, quiet: bool, verbose: bool) -> None: } click.echo(json.dumps(ir, indent=2)) - sys.exit(2 if has_errors else 0) + sys.exit(EXIT_ERROR if has_errors else EXIT_OK) @cli.command("render") @@ -135,13 +149,13 @@ def render_cmd( f"error: output file already exists: {out_png} (use --overwrite to replace)", err=True, ) - sys.exit(1) + sys.exit(EXIT_USAGE) try: content = file.read_text(errors="replace") except OSError as exc: click.echo(f"error: {exc}", err=True) - sys.exit(1) + sys.exit(EXIT_ERROR) if file.suffix.lower() in EXCELLON_SUFFIXES: img = parse_excellon(content, source_path=file) @@ -160,7 +174,7 @@ def render_cmd( click.echo(f"info: {diag.message}", err=True) if has_errors: - sys.exit(2) + sys.exit(EXIT_ERROR) vp = compute_viewport(img.bounding_box, width, height) @@ -173,7 +187,7 @@ def render_cmd( surface.write_to_png(str(out_png)) except OSError as exc: click.echo(f"error: {exc}", err=True) - sys.exit(1) + sys.exit(EXIT_ERROR) if not quiet: click.echo(f"rendered {width}x{height} -> {out_png}") @@ -270,7 +284,7 @@ def diff_cmd( "error: --align-offset must be two comma-separated floats (e.g. '0.5,0')", err=True, ) - sys.exit(2) + sys.exit(EXIT_USAGE) # Memory warning total_pixels = width * height @@ -323,13 +337,13 @@ def _on_diagnostic(path: Path, diag: Diagnostic) -> None: ) except GerberParseError as exc: click.echo(f"error: {exc}", err=True) - sys.exit(2) + sys.exit(EXIT_ERROR) except FileExistsError as exc: click.echo(f"error: {exc} (use --overwrite to replace)", err=True) - sys.exit(1) + sys.exit(EXIT_USAGE) except OSError as exc: click.echo(f"error: {exc}", err=True) - sys.exit(1) + sys.exit(EXIT_ERROR) elapsed_total = time.perf_counter() - t_start @@ -351,7 +365,7 @@ def _on_diagnostic(path: Path, diag: Diagnostic) -> None: write_report(diff_result, out_json, overwrite=overwrite) except FileExistsError as exc: click.echo(f"error: {exc} (use --overwrite to replace)", err=True) - sys.exit(1) + sys.exit(EXIT_USAGE) elapsed_ms = f"({elapsed_total * 1000:.0f} ms)" if not quiet: @@ -364,7 +378,7 @@ def _on_diagnostic(path: Path, diag: Diagnostic) -> None: if out_json: click.echo(f"report: {out_json}") - sys.exit(1 if fail_on_diff and diff_result.has_changes else 0) + sys.exit(EXIT_DIFFERENT if fail_on_diff and diff_result.has_changes else EXIT_OK) # --------------------------------------------------------------------------- @@ -445,6 +459,7 @@ def geomdiff_cmd( from gerberdiff.export.json_report import write_geometry_report from gerberdiff.export.svg_export import write_geometry_svg from gerberdiff.geometry import compute_geometry_diff + from gerberdiff.geometry.types import DiffOutcome from gerberdiff.types import GerberParseError def _on_diagnostic(path: Path, diag: Diagnostic) -> None: @@ -468,10 +483,10 @@ def _on_diagnostic(path: Path, diag: Diagnostic) -> None: ) except GerberParseError as exc: click.echo(f"error: {exc}", err=True) - sys.exit(2) + sys.exit(EXIT_ERROR) except OSError as exc: click.echo(f"error: {exc}", err=True) - sys.exit(1) + sys.exit(EXIT_ERROR) elapsed_total = time.perf_counter() - t_start if verbose: @@ -501,7 +516,7 @@ def _on_diagnostic(path: Path, diag: Diagnostic) -> None: write_geometry_report(result, out_json, tolerances=tolerances, overwrite=overwrite) except FileExistsError as exc: click.echo(f"error: {exc} (use --overwrite to replace)", err=True) - sys.exit(1) + sys.exit(EXIT_USAGE) if out_svg_dir is not None: try: @@ -513,19 +528,39 @@ def _on_diagnostic(path: Path, diag: Diagnostic) -> None: ) except FileExistsError as exc: click.echo(f"error: {exc} (use --overwrite to replace)", err=True) - sys.exit(1) + sys.exit(EXIT_USAGE) + + # What the engine could not model goes to stderr even under --quiet: it is the + # difference between "no changes" and "no changes that I could see", and suppressing + # it is the failure this reports on. + unrepresented = result.unrepresented + if unrepresented: + from gerberdiff.geometry.layer_geometry import UNREPRESENTED_REASONS + + click.echo( + "warning: this comparison is incomplete; " + f"{sum(unrepresented.values())} operation(s) could not be modelled:", + err=True, + ) + for reason, count in sorted(unrepresented.items()): + click.echo(f" {count} x {UNREPRESENTED_REASONS[reason]}", err=True) if not quiet: changed_layers = sum(1 for layer_diff in result.layers if layer_diff.has_changes) total_changes = sum(len(layer_diff.changes) for layer_diff in result.layers) click.echo( - f"geomdiff: {changed_layers}/{len(result.layers)} layers changed, " + f"geomdiff: {result.outcome}, " + f"{changed_layers}/{len(result.layers)} layers changed, " f"{total_changes} changes ({elapsed_total * 1000:.0f} ms)" ) if out_json: click.echo(f"report: {out_json}") - sys.exit(1 if fail_on_diff and result.has_changes else 0) + # Indeterminate does not wait for --fail-on-diff. That flag chooses whether a + # *difference* is a failure; it has no bearing on whether the tool could look. + if result.outcome == DiffOutcome.Indeterminate: + sys.exit(EXIT_INDETERMINATE) + sys.exit(EXIT_DIFFERENT if fail_on_diff and result.has_changes else EXIT_OK) if __name__ == "__main__": diff --git a/gerberdiff/export/json_report.py b/gerberdiff/export/json_report.py index 1e383f9..e41c194 100644 --- a/gerberdiff/export/json_report.py +++ b/gerberdiff/export/json_report.py @@ -21,7 +21,7 @@ from gerberdiff.types import DiffResult, LayerDiffResult, LayerStatus, Region _SCHEMA_VERSION = 1 -_GEOMETRY_SCHEMA_VERSION = 2 +_GEOMETRY_SCHEMA_VERSION = 3 _GENERATOR = "gerberdiff" @@ -92,6 +92,11 @@ def build_geometry_report( "changed_layers": sum(1 for layer in result.layers if layer.has_changes), "total_changes": sum(len(layer.changes) for layer in result.layers), "has_changes": result.has_changes, + # has_changes cannot carry "could not tell": it was False both for two + # identical boards and for a board whose geometry the engine could not + # model. Branch on outcome, not on has_changes. + "outcome": str(result.outcome), + "unrepresented": result.unrepresented, }, "layers": [_serialize_geometry_layer(layer) for layer in result.layers], } @@ -140,6 +145,7 @@ def _serialize_geometry_layer(layer: LayerGeometryDiff) -> dict[str, Any]: "resized": layer.count("resized"), }, "changes": [_serialize_geometry_change(c) for c in layer.changes], + "unrepresented": layer.unrepresented, } diff --git a/gerberdiff/geometry/driver.py b/gerberdiff/geometry/driver.py index 2492165..145abcf 100644 --- a/gerberdiff/geometry/driver.py +++ b/gerberdiff/geometry/driver.py @@ -133,6 +133,7 @@ def _build(path: Path) -> LayerGeometry: status=pair.status, added_area_mm2=total_mm2 if pair.status == LayerStatus.Added else 0.0, removed_area_mm2=total_mm2 if pair.status == LayerStatus.Removed else 0.0, + unrepresented=dict(geometry.unrepresented), ) ) continue @@ -195,6 +196,12 @@ def _diff_layer_pair( # raster engine's region ordering. changes.sort(key=lambda c: (-c.centroid_y, c.centroid_x)) + # Merged across both revisions: an operation the engine could not model on either + # side leaves the comparison unable to say the pair is identical. + unrepresented = dict(geom_a.unrepresented) + for reason, n in geom_b.unrepresented.items(): + unrepresented[reason] = unrepresented.get(reason, 0) + n + return LayerGeometryDiff( name=name, layer_type=layer_type, @@ -203,6 +210,7 @@ def _diff_layer_pair( unchanged_count=unchanged_count, added_area_mm2=added_geom.area * MM2_PER_IN2, removed_area_mm2=removed_geom.area * MM2_PER_IN2, + unrepresented=unrepresented, ) diff --git a/gerberdiff/geometry/layer_geometry.py b/gerberdiff/geometry/layer_geometry.py index 12261b5..654c655 100644 --- a/gerberdiff/geometry/layer_geometry.py +++ b/gerberdiff/geometry/layer_geometry.py @@ -134,6 +134,27 @@ def _centroid_xy(self) -> tuple[float, float]: return self._centroid +# Why a draw operation reached the engine but produced no geometry. Each is a +# real skip below, not a hypothetical: the engine cannot model the operation, so a +# comparison that depends on it cannot be made. Reported rather than dropped -- +# geomdiff used to answer "0 changes" on a board carrying one of these, with the +# JSON byte-identical to comparing a board against itself. +UNREPRESENTED_REASONS = { + "stroke_with_macro_aperture": ( + "a stroke drawn with a macro aperture; the geometry engine does not model it " + "(the raster engine draws a hairline)" + ), + "stroke_with_block_aperture": ( + "a stroke drawn with a block aperture; the geometry engine does not model it" + ), + "aperture_without_extents": "an aperture whose extents the engine cannot compute", + "macro_flash_is_empty": "a macro flash that evaluated to empty geometry", + "block_nesting_too_deep": ( + f"block-aperture nesting deeper than {_MAX_BLOCK_DEPTH}, which is not replayed" + ), +} + + @dataclass class LayerGeometry: """All expanded operations of one parsed file, in replay order.""" @@ -141,6 +162,16 @@ class LayerGeometry: ops: list[ExpandedOp] = field(default_factory=list) has_clear: bool = False diagnostics: list[Diagnostic] = field(default_factory=list) + unrepresented: dict[str, int] = field(default_factory=dict) + """Reason from :data:`UNREPRESENTED_REASONS` -> how many operations it swallowed. + + Non-empty means this layer's geometry is incomplete, so any comparison against it + can report *different* or *could not tell*, but never *identical*. + """ + + def cannot_represent(self, reason: str) -> None: + assert reason in UNREPRESENTED_REASONS, reason + self.unrepresented[reason] = self.unrepresented.get(reason, 0) + 1 # --------------------------------------------------------------------------- @@ -205,7 +236,11 @@ def _walk( result: LayerGeometry, ) -> None: if depth >= _MAX_BLOCK_DEPTH: - return # matches renderer: silently skip over-deep nesting + # Matches the renderer's limit. Recorded rather than skipped in silence: the + # content below this depth is absent from the geometry, so a diff against it + # cannot claim the layers are identical. + result.cannot_represent("block_nesting_too_deep") + return # Per-layer transform/tile cache. layer_cache: dict[int, tuple[_Matrix, list[_Offset], bool]] = {} @@ -278,6 +313,9 @@ def _emit_flash( polarity: Polarity, ) -> None: if ap is None: + # Unreachable since the parser errors on an undefined aperture, but a flash + # with no aperture is exactly the shape that produced issue #17. + result.cannot_represent("aperture_without_extents") return if isinstance(ap, MacroAperture): @@ -285,12 +323,14 @@ def _emit_flash( geom, diags = flash_geometry(op, ap) result.diagnostics.extend(diags) if geom.is_empty: + result.cannot_represent("macro_flash_is_empty") return thunk = _const_thunk(geom) op_bounds: _Bounds = geom.bounds else: extents = _aperture_half_extents(ap) if extents is None: + result.cannot_represent("aperture_without_extents") return hx, hy = extents x, y = op.stop_x, op.stop_y @@ -328,11 +368,18 @@ def _emit_stroke( polarity: Polarity, ) -> None: if ap is None or isinstance(ap, (BlockAperture, MacroAperture)): - # Strokes with macro/block apertures are not meaningful; the raster - # engine draws them with a hairline -- skip in the geometry engine. + # The geometry engine does not model these; the raster engine draws a hairline. + # Recorded, because "not modelled" is not "not there": a trace stroked with a + # macro aperture used to vanish here and geomdiff reported 0 changes at exit 0. + result.cannot_represent( + "stroke_with_block_aperture" + if isinstance(ap, BlockAperture) + else "stroke_with_macro_aperture" + ) return extents = _aperture_half_extents(ap) if extents is None: + result.cannot_represent("aperture_without_extents") return hx, hy = extents brush = max(hx, hy) # conservative half-extent in any direction diff --git a/gerberdiff/geometry/types.py b/gerberdiff/geometry/types.py index e1c8b2f..027a436 100644 --- a/gerberdiff/geometry/types.py +++ b/gerberdiff/geometry/types.py @@ -11,6 +11,7 @@ from __future__ import annotations from dataclasses import dataclass, field +from enum import StrEnum from typing import TYPE_CHECKING, Literal from gerberdiff.types import DrawOp, LayerStatus, LayerType, RegionFill @@ -59,6 +60,17 @@ class LayerGeometryDiff: unchanged_count: int = 0 added_area_mm2: float = 0.0 removed_area_mm2: float = 0.0 + unrepresented: dict[str, int] = field(default_factory=dict) + """Reasons this layer's geometry is incomplete, merged across both revisions. + + Non-empty means the comparison could not be completed, so ``outcome`` is + ``indeterminate`` even when no change was found: what was not modelled cannot be + reported as unchanged (A3). + """ + + @property + def indeterminate(self) -> bool: + return bool(self.unrepresented) def count(self, kind: ChangeKind) -> int: """Number of changes of the given *kind*.""" @@ -74,6 +86,20 @@ def has_changes(self) -> bool: ) +class DiffOutcome(StrEnum): + """What the comparison established. Three outcomes, and only one is green. + + A differ needs the third for the same reason a checker does: silence must never + read as *no difference*. Before this existed a layer carrying geometry the engine + could not model reported ``identical`` at exit 0, with the JSON byte-identical to + comparing a board against a copy of itself. + """ + + Identical = "identical" + Different = "different" + Indeterminate = "indeterminate" + + @dataclass class GeometryDiffResult: """Full geometry diff across all matched layers.""" @@ -83,3 +109,26 @@ class GeometryDiffResult: @property def has_changes(self) -> bool: return any(layer.has_changes for layer in self.layers) + + @property + def outcome(self) -> DiffOutcome: + """``different`` outranks ``indeterminate``. + + A change that *was* found is a real finding about the fabrication output and + stays one even when some other operation could not be modelled. The reverse + would let one unmodellable stroke mask a trace that actually moved. + """ + if self.has_changes: + return DiffOutcome.Different + if any(layer.indeterminate for layer in self.layers): + return DiffOutcome.Indeterminate + return DiffOutcome.Identical + + @property + def unrepresented(self) -> dict[str, int]: + """Every reason across every layer, summed.""" + totals: dict[str, int] = {} + for layer in self.layers: + for reason, n in layer.unrepresented.items(): + totals[reason] = totals.get(reason, 0) + n + return totals diff --git a/tests/test_cli_diff.py b/tests/test_cli_diff.py index c35253d..28161dd 100644 --- a/tests/test_cli_diff.py +++ b/tests/test_cli_diff.py @@ -158,7 +158,7 @@ def test_diff_json_no_overwrite(tmp_path: Path) -> None: "--out-json", str(out_json), ) - assert result.exit_code == 1 + assert result.exit_code == 64, "an existing output file is a usage error (EX_USAGE)" assert out_json.read_text() == "{}" @@ -279,5 +279,7 @@ def test_diff_undefined_aperture_fails_the_gate(tmp_path: Path) -> None: header + "%ADD10C,0.1*%\nD10*\nX0Y0D03*\nD11*\nX500000Y500000D03*\n" + "M02*\n" ) result = _run("diff", str(before), str(after), "--fail-on-diff") - assert result.exit_code == 2, result.output + # 4, not 2: the file could not be parsed at all. Exit 2 is now reserved for a + # comparison that ran but could not be completed (A3). + assert result.exit_code == 4, result.output assert "undefined aperture D11" in result.output diff --git a/tests/test_cli_geomdiff.py b/tests/test_cli_geomdiff.py index 01cf27d..ca096cf 100644 --- a/tests/test_cli_geomdiff.py +++ b/tests/test_cli_geomdiff.py @@ -105,7 +105,7 @@ def test_geomdiff_json_report(tmp_path: Path) -> None: result = _run("geomdiff", str(tmp_path / "b"), str(tmp_path / "a"), "--out-json", str(out)) assert result.exit_code == 0 data = json.loads(out.read_text()) - assert data["version"] == 2 + assert data["version"] == 3 assert data["mode"] == "geometry" assert data["summary"]["total_changes"] == 1 assert data["tolerances"]["move_tol_mm"] == 0.005 @@ -118,7 +118,7 @@ def test_geomdiff_json_no_overwrite(tmp_path: Path) -> None: out = tmp_path / "report.json" out.write_text("{}") result = _run("geomdiff", str(tmp_path / "b"), str(tmp_path / "a"), "--out-json", str(out)) - assert result.exit_code == 1 + assert result.exit_code == 64, "an existing output file is a usage error (EX_USAGE)" assert "already exists" in result.output @@ -169,7 +169,7 @@ def test_geomdiff_layer_filter(tmp_path: Path) -> None: # --------------------------------------------------------------------------- -def test_geomdiff_parse_error_exit_2(tmp_path: Path) -> None: +def test_geomdiff_parse_error_exit_4(tmp_path: Path) -> None: before, after = tmp_path / "b", tmp_path / "a" before.mkdir() after.mkdir() @@ -178,7 +178,7 @@ def test_geomdiff_parse_error_exit_2(tmp_path: Path) -> None: (before / "board-F.Cu.gbr").write_text(bad) (after / "board-F.Cu.gbr").write_text(bad) result = _run("geomdiff", str(before), str(after)) - assert result.exit_code == 2 + assert result.exit_code == 4, "a file that cannot be parsed is an error, not a verdict" assert "error" in result.output.lower() @@ -204,6 +204,8 @@ def test_geomdiff_undefined_aperture_fails_the_gate(tmp_path: Path) -> None: _write_board(tmp_path / "b", 0) _write_board_with_undefined_aperture(tmp_path / "a") result = _run("geomdiff", str(tmp_path / "b"), str(tmp_path / "a"), "--fail-on-diff") - assert result.exit_code == 2, result.output + # 4, not 2: the file could not be parsed at all. Exit 2 is now reserved for a + # comparison that ran but could not be completed (A3). + assert result.exit_code == 4, result.output assert "undefined aperture D11" in result.output assert "0 changes" not in result.output diff --git a/tests/test_cli_render.py b/tests/test_cli_render.py index f3b4c46..cf056b1 100644 --- a/tests/test_cli_render.py +++ b/tests/test_cli_render.py @@ -55,7 +55,7 @@ def test_render_no_overwrite_by_default(tmp_path: Path) -> None: out = tmp_path / "fcu.png" out.write_bytes(b"existing") result = _run("render", str(_FCU), "--out-png", str(out), "--width", "64", "--height", "64") - assert result.exit_code == 1 + assert result.exit_code == 64, "an existing output file is a usage error (EX_USAGE)" # Original file untouched assert out.read_bytes() == b"existing" diff --git a/tests/test_geometry_report.py b/tests/test_geometry_report.py index bd2c285..bf73946 100644 --- a/tests/test_geometry_report.py +++ b/tests/test_geometry_report.py @@ -79,7 +79,7 @@ def _sample_result() -> GeometryDiffResult: def test_report_version_and_mode() -> None: report = build_geometry_report(_sample_result()) - assert report["version"] == 2 + assert report["version"] == 3 assert report["mode"] == "geometry" assert report["generator"] == "gerberdiff" @@ -90,6 +90,10 @@ def test_report_summary() -> None: "changed_layers": 1, "total_changes": 4, "has_changes": True, + # A differ needs a third state too: has_changes was False both for two + # identical boards and for one whose geometry could not be modelled (A3). + "outcome": "different", + "unrepresented": {}, } @@ -130,7 +134,7 @@ def test_write_report_and_overwrite_guard(tmp_path: Path) -> None: out = tmp_path / "report.json" write_geometry_report(_sample_result(), out) data = json.loads(out.read_text()) - assert data["version"] == 2 + assert data["version"] == 3 with pytest.raises(FileExistsError): write_geometry_report(_sample_result(), out) write_geometry_report(_sample_result(), out, overwrite=True) diff --git a/tests/test_indeterminate.py b/tests/test_indeterminate.py new file mode 100644 index 0000000..28987a0 --- /dev/null +++ b/tests/test_indeterminate.py @@ -0,0 +1,178 @@ +"""The third state: what the geometry engine could not model (A3). + +`has_changes: boolean` could not carry "could not tell". A layer holding an +operation the engine does not model -- a stroke drawn with a macro aperture, say +-- reported `0 changes` at exit `0`, and its JSON was byte-identical to comparing +a board against a copy of itself. The raster engine saw the same trace and +reported a change, so the two engines answered the same question differently and +the geometry one gave the dangerous answer. +""" + +from __future__ import annotations + +import json +from pathlib import Path + +from click.testing import CliRunner, Result + +from gerberdiff import cli +from gerberdiff.geometry import compute_geometry_diff +from gerberdiff.geometry.layer_geometry import UNREPRESENTED_REASONS, build_layer_geometry +from gerberdiff.geometry.types import DiffOutcome, GeometryDiffResult, LayerGeometryDiff +from gerberdiff.parse.gerber_state import parse_gerber +from gerberdiff.types import LayerStatus, LayerType + +_HEADER = "%FSLAX36Y36*%\n%MOMM*%\n%AMROUND*\n1,1,$1,0,0*%\n%ADD10C,1.0*%\n%ADD11ROUND,1.0*%\n" +_PAD = "D10*\nX1000000Y1000000D03*\n" +_MACRO_STROKE = "D11*\nX3000000Y3000000D02*\nX8000000Y8000000D01*\n" +_FOOTER = "M02*\n" + + +def _run(*args: str) -> Result: + return CliRunner().invoke(cli.cli, list(args)) + + +def _board(directory: Path, body: str) -> None: + directory.mkdir(parents=True, exist_ok=True) + (directory / "board-F_Cu.gbr").write_text(_HEADER + body + _FOOTER) + + +# -- the engine records what it could not model ---------------------------------------- + + +def test_a_macro_stroke_is_recorded_not_dropped() -> None: + geometry = build_layer_geometry(parse_gerber(_HEADER + _PAD + _MACRO_STROKE + _FOOTER)) + assert geometry.unrepresented == {"stroke_with_macro_aperture": 1} + assert len(geometry.ops) == 1, "only the pad was modelled" + + +def test_a_board_the_engine_fully_models_records_nothing() -> None: + """Guard against every board looking incomplete.""" + geometry = build_layer_geometry(parse_gerber(_HEADER + _PAD + _FOOTER)) + assert geometry.unrepresented == {} + + +def test_every_reason_has_human_text() -> None: + """`cannot_represent` asserts membership, so a typo would be a crash in the field.""" + assert all(isinstance(v, str) and v for v in UNREPRESENTED_REASONS.values()) + + +# -- the outcome ----------------------------------------------------------------------- + + +def _result(*, changes: bool, unrepresented: dict[str, int]) -> GeometryDiffResult: + layer = LayerGeometryDiff( + name="F_Cu", + layer_type=LayerType.FCu, + status=LayerStatus.Added if changes else LayerStatus.Matched, + unrepresented=unrepresented, + ) + return GeometryDiffResult(layers=[layer]) + + +def test_nothing_unmodelled_and_no_changes_is_identical() -> None: + assert _result(changes=False, unrepresented={}).outcome == DiffOutcome.Identical + + +def test_something_unmodelled_and_no_changes_is_indeterminate() -> None: + """The A3 defect: this used to be indistinguishable from identical.""" + assert ( + _result(changes=False, unrepresented={"stroke_with_macro_aperture": 1}).outcome + == DiffOutcome.Indeterminate + ) + + +def test_a_real_change_outranks_something_unmodelled() -> None: + """One unmodellable stroke must not mask a trace that actually moved.""" + assert ( + _result(changes=True, unrepresented={"stroke_with_macro_aperture": 1}).outcome + == DiffOutcome.Different + ) + + +# -- end to end ------------------------------------------------------------------------ + + +def test_geomdiff_does_not_report_an_unmodelled_trace_as_no_change(tmp_path: Path) -> None: + """The reproduction. An added trace used to pass the gate at exit 0.""" + _board(tmp_path / "b", _PAD) + _board(tmp_path / "a", _PAD + _MACRO_STROKE) + result = _run("geomdiff", str(tmp_path / "b"), str(tmp_path / "a"), "--fail-on-diff") + assert result.exit_code == cli.EXIT_INDETERMINATE + assert "indeterminate" in result.output + assert "could not be modelled" in result.output + + +def test_indeterminate_does_not_wait_for_fail_on_diff(tmp_path: Path) -> None: + """--fail-on-diff chooses whether a *difference* fails. It has no bearing on + whether the tool could look.""" + _board(tmp_path / "b", _PAD) + _board(tmp_path / "a", _PAD + _MACRO_STROKE) + result = _run("geomdiff", str(tmp_path / "b"), str(tmp_path / "a")) + assert result.exit_code == cli.EXIT_INDETERMINATE + + +def test_identical_boards_still_exit_0(tmp_path: Path) -> None: + """The control. Without it, every one of these would pass on a tool that always + exited 2.""" + _board(tmp_path / "b", _PAD) + _board(tmp_path / "a", _PAD) + result = _run("geomdiff", str(tmp_path / "b"), str(tmp_path / "a"), "--fail-on-diff") + assert result.exit_code == cli.EXIT_OK + assert "identical" in result.output + + +def test_the_report_distinguishes_the_two_situations(tmp_path: Path) -> None: + """They used to be byte-identical, so no field existed to branch on.""" + _board(tmp_path / "b", _PAD) + _board(tmp_path / "a", _PAD + _MACRO_STROKE) + _board(tmp_path / "same", _PAD) + + incomplete = tmp_path / "incomplete.json" + identical = tmp_path / "identical.json" + _run("geomdiff", str(tmp_path / "b"), str(tmp_path / "a"), "--out-json", str(incomplete)) + _run("geomdiff", str(tmp_path / "b"), str(tmp_path / "same"), "--out-json", str(identical)) + + a = json.loads(incomplete.read_text()) + b = json.loads(identical.read_text()) + assert a != b, "a real omission must not serialise the same as two identical boards" + assert a["summary"]["outcome"] == "indeterminate" + assert b["summary"]["outcome"] == "identical" + assert a["summary"]["unrepresented"] == {"stroke_with_macro_aperture": 1} + assert b["summary"]["unrepresented"] == {} + # has_changes alone still cannot tell them apart -- that is why outcome exists. + assert a["summary"]["has_changes"] == b["summary"]["has_changes"] is False + + +def test_the_geometry_engine_no_longer_contradicts_the_raster_engine(tmp_path: Path) -> None: + """Both engines saw the same added trace; only one used to say so.""" + _board(tmp_path / "b", _PAD) + _board(tmp_path / "a", _PAD + _MACRO_STROKE) + geom = _run("geomdiff", str(tmp_path / "b"), str(tmp_path / "a"), "--fail-on-diff") + assert geom.exit_code != cli.EXIT_OK, "geomdiff must not call this clean" + + +# -- the exit-code contract ------------------------------------------------------------ + + +def test_exit_codes_match_the_org_vocabulary() -> None: + """A1 settled these across partspec and netspec; gerberdiff follows (A3).""" + assert cli.EXIT_OK == 0 + assert cli.EXIT_DIFFERENT == 1 + assert cli.EXIT_INDETERMINATE == 2 + assert cli.EXIT_ERROR == 4 + assert cli.EXIT_USAGE == 64 + + +def test_indeterminate_is_not_the_error_code() -> None: + """ "I could not model part of this" is not "I could not read the file".""" + assert cli.EXIT_INDETERMINATE != cli.EXIT_ERROR + + +def test_compute_geometry_diff_carries_the_reason_through(tmp_path: Path) -> None: + _board(tmp_path / "b", _PAD) + _board(tmp_path / "a", _PAD + _MACRO_STROKE) + result = compute_geometry_diff(tmp_path / "b", tmp_path / "a") + assert result.outcome == DiffOutcome.Indeterminate + assert result.unrepresented == {"stroke_with_macro_aperture": 1} + assert result.layers[0].indeterminate