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