Stop treating the port status snapshots as contract - #5590
Conversation
Registering one conformance test required editing eleven files nobody had
a result for. `validate()` demanded that every checked-in
`docs/website/data/port_status_reports/<port>.json` carry an entry for
every registered test, with no `not-run` and a `summary` matching it
exactly -- so a branch adding a test had to append an entry to all eleven
reports and retype their totals, plus `assertEqual(180, counts["tests"])`
in `test_port_status.py`. Two branches adding a test conflicted by
construction, on lines whose entire content was a number neither author
had a reason to think about.
The cost was worse than the friction. Those files are CI output; a branch
has no result to put in them, so the cheapest way to a green build was to
type "pass" next to a test no port had run. Twelve results reached master
that way -- LogSubclassCaptureTest, IntentsApiTest, the seven Database
tests, CalendarApiTest, the on-device AI tests, PureEditorScreenshotTest
and ClipboardRoundTripTest -- each appended to a report whose
generated_at and commit predate the test it claims to have passed. The
public table published every one of them as green.
None of it was ever needed. `publishable_report_problems` already
classifies "this report predates a test" as ordinary drift: publication
keeps the older report and waits for the port's next master run. Only the
checked-in copies were held to a stricter rule than the ones the site
actually serves.
So validate() now reads the snapshots through that same drift/malformed
split. "report predates tests: X" prints as information and never fails;
a report Hugo cannot render is still a defect. Adding a test touches
Cn1ssDeviceRunner and port_status.json, and nothing else.
"Every registered test runs on every port" moves to where nobody can type
the answer:
* `port_status.py coverage` runs in the nightly sweep against the
reports the ports actually published. `not-run` always fails. A test
missing from port P's report fails only when some earlier run on
another port already carried it -- that port dropped the test rather
than predating it. No history lookup and no grace period to tune; the
reports date themselves.
* `port_status.py provenance`, via check_port_status_provenance.sh in
port-status-contract.yml, refuses a report whose results changed while
the generated_at, commit and run_url naming the run stayed put. That
is exactly the edit the twelve fabricated passes were made by.
test_port_status.py keeps no literal totals. It asserts the relationship
the numbers stood in for -- the suite and the contract describe the same
test set, nothing is registered twice, nothing is both a conformance and
a performance test -- plus floors that catch a collapse. Nothing in it
needs retyping when a test is added.
The page separates absent from `not-run`, which the old rule had no way
to distinguish because it forbade both. Absent means the run predates the
test and renders "awaiting this port's next run"; `not-run` means the
suite ran with the test in its contract and nothing reported back, and
stays a defect.
The eleven snapshots are refreshed from port-status-data, which drops the
fabrications. The five Apple ports that have not run since
LogSubclassCaptureTest landed now say so instead of claiming a pass.
Verified: 46 normalizer tests pass; validate, coverage and provenance all
green; Hugo builds and validate_port_status.mjs passes on the rendered
page; both new gates confirmed to fire on an injected dropped test, an
injected not-run, and a stamp-preserving edit; and a simulated branch that
registers a test while touching no report passes validate and provenance.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3b71a9e03a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # writes in a pull request can reach it. A registered test runs on every port, | ||
| # and the only permitted exception is a skip the suite itself emits with an | ||
| # erratum explaining it. | ||
| python3 "${SCRIPT_DIR}/port_status.py" coverage --reports "${published_dir}" |
There was a problem hiding this comment.
When a test is newly registered and a port has not rerun yet, the earlier accept call classifies that report as contract drift (status 11), adds it to problems, and exits before this coverage command runs. The candidate-publication loop also refuses that report for the same reason. Consequently the new known_since logic that deliberately allows an older report to predate a test is unreachable in exactly that scenario, and the nightly sweep fails until every port has produced the new test instead of tolerating older snapshots as intended.
Useful? React with 👍 / 👎.
| unrun = sorted( | ||
| name | ||
| for name, result in tests.items() | ||
| if isinstance(result, dict) and result.get("status") == "not-run" | ||
| ) |
There was a problem hiding this comment.
When a published report newly marks a test as skip but no skip erratum exists, this filter treats it as a satisfactory result and the coverage command succeeds. The preceding validate() only collects skipped tests from the checked-in fallback reports, not from the reports supplied through --reports, so the nightly gate can publish and approve an undocumented skip; it is only discovered later if a website build syncs that report and reruns validation. Check the published reports' skipped tests against the supplement here so the stated skip exception is actually enforced by this gate.
Useful? React with 👍 / 👎.
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
|
Compared 151 screenshots: 151 matched. Native Android coverage
✅ Native Android screenshot tests passed. Native Android coverage
Benchmark ResultsDetailed Performance Metrics
|
…eir errata Two defects in the gates the previous commit put in place of the hand-edited snapshots. The nightly sweep never reached the coverage gate in the one case it was written for. Its closing assertion ran `accept` on each published report and recorded any nonzero status as a problem, then exited before the coverage command ran. Contract drift is status 11 -- a report built before a newly registered test, which is the state of every port between that merge and its next run -- so a test-adding merge turned the nightly red for a day, and the `known_since` comparison that deliberately tolerates an older report was unreachable in exactly the scenario it exists for. That also made the sweep contradict the candidate loop twenty lines above it, whose comment already says drift "stays quiet, because waiting for a run on the current contract is the intended behaviour there, not a defect". Drift is logged and falls through to the freshness check now; only a report the website cannot use is a defect. The failure predates this branch: ios-gl, ios-metal, mac-native, tvos and watchos are on the data branch at a commit before LogSubclassCaptureTest, so the sweep is failing on master today for that reason. The coverage gate also accepted any `skip` as a satisfactory result, so a port could stop running a test by marking it skipped and publishing -- the exception the gate documents is a skip *with an erratum*, and nothing checked. validate() could not cover this: it reads skips out of the checked-in fallbacks, not out of the reports the ports publish, so the first symptom would have been a failed website build rather than the name of the port that started skipping. Coverage now applies the page's own rule, `skip_is_documented`: the erratum has to name the test and, where it lists reason codes, account for every reason that run gave from a port that code applies to. Matching on the test name alone would let any future skip of a named test read as documented -- CameraApiTest has errata, but the missing-webcam code is written about Windows, and the same code arriving from Linux says something nobody has explained. The eleven published reports pass the stricter rule unchanged. Four tests cover it: a documented skip, an undocumented one, a reason code scoped to another port, and a skip carrying no reason at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Cloudflare Preview
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9264e7762e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…st map The check compared `tests` and `summary` and returned "untouched" for anything else, so a branch could rewrite a benchmark duration, the memory figures, the binary size or `suite_finished` in place and the edit would be published as a measurement of the original run. Those are the findings least likely to be doubted, because unlike a test result nobody can check one by reading it: the page presents `performance` as ten measured durations for a named commit, and `suite_finished` is what makes a port card say the suite completed rather than that the run stopped early. Naming a subset of the payload left exactly those as the one thing a branch could still edit without tripping the gate. Everything except `generated_at`, `commit` and `run_url` is a finding now. The message names which fields changed, so the report reads "performance changed but generated_at, commit, run_url did not" rather than making the author diff the file to find out what the gate objected to. No exemption for a schema migration, deliberately: a migration rewrites the producers and the snapshots are then refreshed from the data branch, which moves the stamps and passes on its own. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 675779ecc9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ired test Two inconsistencies where the new rules did not apply themselves evenly. The port card ignored the awaiting count it computed. With the suite finished and nothing failed, a port whose report predates a newly registered test still read "Suite completed" in green -- directly above a feature cell that renders partial for that very test, because the cell has no answer to show. The headline contradicting the column is the shape the card's own existing comment already argues against for not-run. Awaiting selects partial now, last in the chain so a stale report or an incomplete run still names itself first, and with a label that says what is pending rather than what is broken: "1 test awaiting this port's next run". The coverage gate's not-run scan read every entry in a report instead of only the tests the contract still defines, which is the filter the skip check immediately below it already uses. A report that predates a test's retirement and carries that test as not-run therefore failed the sweep over a test nobody can run any more -- while the same report is tolerated as drift everywhere else, so the nightly stayed red until that port happened to rerun for an unrelated reason. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: abc4659b9b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Compared 149 screenshots: 149 matched. Benchmark ResultsDetailed Performance Metrics
|
|
Compared 181 screenshots: 181 matched. |
|
Compared 149 screenshots: 149 matched. Benchmark ResultsDetailed Performance Metrics
|
|
Compared 149 screenshots: 149 matched. |
|
Compared 149 screenshots: 149 matched. |
|
Compared 149 screenshots: 149 matched. Benchmark ResultsDetailed Performance Metrics
|
…e contract The provenance gate accepted a change to *any* one of the three provenance fields, which left open the easier version of the forgery it was written to stop: invent the result, type today's date into `generated_at`, and leave `commit` and `run_url` still naming the run that never produced it. validate() accepts such a timestamp, so nothing downstream would have objected either. Both `run_url` and `generated_at` must now name a genuinely different run, and the new value has to be non-empty -- deleting the field is a change too, and would otherwise have let the edit through with no run named at all. `commit` stays out of that requirement on purpose: a port legitimately re-runs the same master commit, and its second run is a different run. The message names which identity field is still pointing at the old one. The port card summed the report's own summary counts, which cover every entry the report carries rather than the tests the contract defines. That is wrong at both ends of the difference, and the previous commit only fixed one end. A report predating a newly registered test has no entry for it -- handled. A report predating a test's RETIREMENT still carries the test, so a retired failure kept the card red, and a retired not-run kept it partial, over a test that appears nowhere in the matrix below it. Now that validate() tolerates retired entries as drift and the sync keeps such a report, that state is reachable rather than hypothetical. The card is the headline for a column of cells, so it is computed that way now: one pass over the contract's tests, looking each up in the report, tallying the same four states the cells show plus awaiting. Output on the current data is byte-identical -- nothing is retired today -- and a retired failure injected into the JavaScript report no longer reddens its card. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f2d972d514
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| self.assertEqual(set(registered), set(mapped) | set(performance)) | ||
| self.assertEqual(set(), set(mapped) & set(performance)) |
There was a problem hiding this comment.
These replacement checks only prove that every registered test is mapped somewhere; deleting the explicit assertions for camera-access, calendar-integration, the on-device APIs, video tests, and similar features means a future edit can move one of those tests under an unrelated feature while this suite and validate() still pass. The website would then silently publish the result under the wrong capability. Keep those semantic mapping checks—they are not the conflict-prone literal totals this change is trying to remove.
Useful? React with 👍 / 👎.
Removing the literal totals took the feature-mapping assertions with them, and those are a different thing. validate() only requires each registered test to sit under exactly one feature -- any feature satisfies it -- so with these gone a future edit could file CameraApiTest under an unrelated row and the whole suite would still pass while the page published a camera claim the test never made. The same goes for the calendar, on-device vision/language/inference, AR and VideoIO rows, each of which exists to say something specific about a capability. They are back, in their own test rather than appended to the counts one, with the reason they are kept written down: this is a spot check of the mappings where landing a result under the wrong row misstates a capability, not a registry every new feature has to join. What made the old block painful was the totals beside it, which every branch had to retype; nothing obliges a branch to touch this list. Verified by moving CameraApiTest under an unrelated feature: the assertion fails. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98b012004f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if all( | ||
| before.get(field) != after.get(field) and after.get(field) | ||
| for field in RUN_IDENTITY_FIELDS | ||
| ): | ||
| return [] |
There was a problem hiding this comment.
When a branch changes report findings and also supplies any different nonempty generated_at and run_url, this returns success without establishing that the URL identifies a real CI run or that the run produced the report; for example, a fabricated result with run_url: "made-up-new-run" passes both provenance_problems() and the publishability checks. The fresh evidence after the prior review is that the revised guard now checks both fields, but still validates only inequality and truthiness, so the anti-forgery gate remains bypassable by editing two strings rather than one.
Useful? React with 👍 / 👎.
Requiring both identity fields to change still only compared strings, so the gate was bypassable by inventing two of them instead of one -- a plausible timestamp and a plausible run URL alongside the fabricated result. Nothing so far established that the run existed, let alone that it produced this report. It can be established, because a checked-in report is not authored: it is a copy of what CI put on the port-status-data branch. So the check now asks that branch. A changed report has to be a version the branch actually holds, which a refresh satisfies by construction and an invented report cannot be made to satisfy without the write access to that branch that only the publish workflows have. The identity strings stop being two fields a branch can fill in and become properties of a report a run really produced. About a week of that branch's history is read, not just its tip: a refresh taken before the port ran again matches an ancestor, and rejecting it for not equalling today's tip would be a false accusation. The fetch is depth-limited and takes under a second; the whole check runs in about four over eleven ports. If the branch cannot be reached the corroboration is dropped with a note rather than failing the branch -- unverifiable is not forged, and a gate that fails on a flaky fetch teaches people to route around it. A run URL is also required to look like one, which costs nothing and rejects the field filled in with a placeholder. That is shape only and proves nothing on its own; the data branch is what proves something. Verified end to end against origin/master: the eleven refreshed reports corroborate (as ancestors -- the branch has moved since), a fabricated result with run_url "made-up-new-run" is refused on shape, the same fabrication with a well-formed but never-published URL is refused as never published, and copying a genuine published report in is accepted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1178c501a9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Two ways past the data-branch check, both of them the same shape: a report that never had to prove anything. A change that touches only `generated_at` alters no finding, so every rule about findings returned "untouched" and the corroboration was never reached. That is the edit with the worst consequence of any of them: the page reads generated_at to decide whether a column is stale, so retyping it is how a port that has stopped reporting altogether would go on looking like it was still running -- and validate() accepts any plausible past timestamp. Corroboration is now asked of any change at all, not only of a changed result. A report the branch ADDS had no base version, and the driver skipped it. That is the one file with nothing earlier to check against, so it was the only one nobody was checking: a pull request adding a port could give it an entirely hand-authored green snapshot, which is precisely the forgery the rest of this gate exists to stop. An added report now has to be a version the data branch published, like every other. For that to be actionable rather than a dead end, a port with no stored report had to become a supported state -- otherwise adding a port would begin by authoring the snapshot this rule refuses. validate() now reports a missing report as drift, and the page already renders it correctly without one: every cell reads "No stored report" and the card reads unknown, which is what is true of a port CI has never heard from. Verified by removing tvos.json and building the site. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5bb67b1ece
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| head_path = REPO_ROOT / report_directory / f"{port_id}.json" | ||
| if not head_path.is_file(): | ||
| # Removed, or a port that has never published. Neither is an | ||
| # edit to a result. | ||
| continue |
There was a problem hiding this comment.
When a PR deletes the checked-in report for an existing manifest port, this path silently accepts the deletion, and validate() also classifies the missing file as nonfatal drift. That breaks the fallback guaranteed by scripts/website/sync_port_status_reports.sh: if the data-branch fetch fails or its candidate is rejected for drift, Hugo receives no report and renders the established port as unknown. Allow an absent file only when the base revision did not already contain that port's report; reject deletion of an existing fallback.
Useful? React with 👍 / 👎.
|
Compared 148 screenshots: 148 matched. Benchmark Results
Detailed Performance Metrics
|
|
Compared 144 screenshots: 144 matched. |
…-are-not-contract
…blished fallback Three things, two of them master's own red CI that this branch inherits through the merge commit. iOS, all four failing jobs: IOSNative.m calls six com_codename1_impl_ios_IOSWearableCallbacks_* functions without including the generated header that declares them. clang 17 rejects an implicit declaration outright, so the phone, Metal and watch targets all failed to compile -- master has been red on scripts-ios.yml and scripts-ios-native.yml since #5583 landed. CN1SmartHome.m carries the identical include for IOSHomeCallbacks, the class this one was modelled on; this one was simply missing. The mangled names themselves are correct, checked against the six Java signatures. Android, the instrumentation NPE: dispatchDraw read renderingOperations.size() and then copied the list, both without the lock the same method takes twenty lines later to clear it -- while flushGraphics swapped the list out from under it on the EDT, also unlocked. ArrayList.addAll copies through toArray(), and a concurrent mutation there returns an array sized for the new contents and padded with NULLS. Those nulls arrived as AsyncOps and threw out of executeWithClip: a hard crash on the UI thread, which is what launchMainActivityAndWaitForDeviceRunner hit. flushGraphics has carried an "if (o != null)" guard against this same corruption since a user reported it, so the nulls were known; nothing had established where they came from. The snapshot and the swap are now both taken under RENDERING_OPERATIONS_LOCK. Only the copy -- the ops still execute outside it, because that is the frame's actual drawing and holding a lock there would park the EDT for the whole paint. The one remaining unguarded read of the field, the flushGraphics wait loop, goes through a small accessor rather than being left as the exception that teaches the next reader the field is free to touch. No null guard added in dispatchDraw: with the copy synchronized the nulls cannot occur, and a guard there would only hide it if they ever did again. Third, from review: deleting the checked-in report for a port that has one is now refused. Making a missing report a supported state was necessary so that adding a port need not begin by hand-authoring a snapshot -- but absent because it never existed and absent because someone removed it are different things, and only the first is harmless. The site serves that file precisely when the data branch is unreachable, so removing one turns an established column unknown at the moment the live data is missing. Retiring a port still works: drop it from the manifest and the check stops looking at it. Verified: the android module compiles against the freshly installed core and the new locking is in the bytecode (two monitorenters in dispatchDraw); the six iOS mangled names match their Java signatures; 66 normalizer tests pass; deleting tvos.json is refused and restoring it passes; validate, coverage, provenance, Hugo and validate_port_status.mjs are all green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both branches fixed the same missing include in IOSNative.m. Master's #5594 is kept whole: it reaches the same conclusion, records the more important half of it -- an implicit declaration that does link invents a prototype and delivers a message out of the wrong registers, which is what the phone slice had been doing -- cites the IOSIntentCallbacks precedent in CodenameOne_GLAppDelegate.m rather than the CN1SmartHome one, and comes with the -[WCSession activate] selector fix in CN1WatchConnectivity.m that this branch never found. The file is now identical to master's. The Android paint-crash fix in this branch is untouched by the merge; nothing on master went near it.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e8d9c16d7f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| any( | ||
| (not code.get("ports") or port_id in code["ports"]) | ||
| and isinstance(reason, str) | ||
| and reason.startswith(code.get("prefix", "")) |
There was a problem hiding this comment.
When a skip erratum's reason_codes entry accidentally omits prefix or sets it to an empty string, reason.startswith("") succeeds for every reason, so the nightly coverage gate approves an unrelated skip as documented. The validator currently checks only the erratum's top-level descriptive fields, so this configuration error passes validation; require every reason code to contain a nonempty string prefix before using it for skip matching.
Useful? React with 👍 / 👎.
|
Compared 143 screenshots: 143 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
|
Compared 149 screenshots: 149 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
|
Compared 217 screenshots: 217 matched. |
Every string starts with the empty one. A reason_codes entry that lost its `prefix` to a typo therefore matched every reason a run could give, so the skip it was written to explain and any unrelated future skip of the same test both read as documented -- on the nightly coverage gate and on the public page, which would draw a green tick with a footnote pointing at an erratum about something else. Nothing objected: validate() checked only the erratum's four top-level descriptive fields and never looked inside its codes. Fixed at the source and on both sides that consume it. validate() now requires every reason code to carry a non-empty string prefix, and any `ports` it names to be a non-empty list of real port ids -- an unknown port silently narrowed a code to nothing in the same quiet way. skip_is_documented and the Hugo partial each refuse an empty prefix on their own rather than trusting the validator, because they are two independent copies of this matching rule and the page is the one that draws the tick. The twenty-four codes in the supplement today all pass unchanged. While adding the validator rule I named a local `ports`, which is the manifest's port list in scope for the whole function, and the returned port count started reading whichever erratum came last. test_contract_covers_registered_tests_and_goldens caught it -- the test whose literal totals this branch replaced with relationships, doing exactly the job the numbers could not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…guard Build Android JDK 21 failed on one uncovered golden: GoogleWebMap. The JDK 8 and 17 legs of the same run produced all 151, and master is green, so nothing was broken -- the Google Maps tiles did not load over the network inside the runner's budget, which GoogleWebMapScreenshotTest already anticipates. After a full 24-second wait and a retry on a warm WebKit it prints "status=SKIPPED reason=map-tiles-never-loaded" and deliberately does not capture, and port_status_supplement.json documents that reason for android, ios-gl and ios-metal precisely so the public table renders it as a documented skip. That skip path could never actually succeed. The count guard runs first and counts a golden as covered only when it compared equal or different, so the skipped golden was uncovered and the job died on it regardless of what the test had said or what the errata allowed. Every component downstream agreed the skip was legitimate; the gate in front of them all disagreed, so a network the runner cannot reach reds a port at random. The guard now discounts a golden whose owning test reported a skip that THIS port's errata explain. That is not the weakening the comment above it warns against, and the distinction is the one the comment itself draws: the guard exists because "a dropped test leaves no per-test record behind to count", and a test that prints its skip left exactly such a record. Silence still fails. So does an unexplained skip, and so does a reason code written about another port -- map-tiles-never-loaded arriving from Linux, where the errata expect no-api-key, is a port misbehaving rather than an unreachable network, and stays a failure. A python that cannot answer discounts nothing. port_status.py grows a "documented-skips" query for it, reusing the same errata matching the coverage gate and the page already share, so there is one rule for what a documented skip is rather than a fourth copy of it. Four tests cover the discrimination: the documented skip, silence, an undocumented reason, and the right reason from the wrong port. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7c3ece43e5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if skip_is_documented(supplement, port_id, owner, reasons): | ||
| accounted.append(golden.stem) |
There was a problem hiding this comment.
When a documented-skipped test owns multiple goldens and one is nevertheless present as equal/different—for example, the test captured one frame before skipping—the covered frame is already included by cn1ss_count_covered(), but this appends every golden owned by the test. cn1ss_process_and_report() then subtracts that full count from expected_count - covered_count, double-counting the produced frame and potentially masking an uncovered golden from another test, allowing the count-regression guard to pass. Determine which skipped goldens are absent from the comparison results before discounting them.
Useful? React with 👍 / 👎.
The discount named every golden a documented-skipped test owns, whether or not the run produced it. A test that owns several screenshots and captures a few before skipping -- CenteredDialogTitleScreenshotTest owns a light and a dark frame -- would have every one of them subtracted, while cn1ss_count_covered had already counted the captured ones. Each double subtraction cancels an uncovered golden belonging to some OTHER test, so the guard could pass a run that had genuinely lost a screenshot: the exact regression it exists to catch, reopened by the fix for the neighbouring hole. The query takes the comparison results now and skips any golden that compared equal or different, so what it returns is only what is actually absent. Passing the comparison is the caller's job and it already has the file open for count_covered; with no comparison available nothing changes, because then there is nothing recorded as produced. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Compared 12 screenshots: 12 matched. |
1 participant
Registering one conformance test required editing eleven files nobody had a result for.
validate()demanded that every checked-indocs/website/data/port_status_reports/<port>.jsoncarry an entry for every registered test, with nonot-runand asummarymatching it exactly. So a branch adding a test had to append an entry to all eleven reports and retype their totals, plusassertEqual(180, counts["tests"])intest_port_status.py. Two branches adding a test conflicted by construction, on lines whose entire content was a number neither author had a reason to think about.The cost was worse than the friction. Those files are CI output; a branch has no result to put in them, so the cheapest way to a green build was to type
"pass"next to a test no port had run. Twelve results reached master that way —LogSubclassCaptureTest,IntentsApiTest, the sevenDatabase*tests,CalendarApiTest, the on-device AI tests,PureEditorScreenshotTestandClipboardRoundTripTest— each appended to a report whosegenerated_atandcommitpredate the test it claims to have passed. The public table published every one of them as green.None of it was ever needed.
publishable_report_problemsalready classifies "this report predates a test" as ordinary drift: publication keeps the older report and waits for the port's next master run. Only the checked-in copies were held to a stricter rule than the ones the site actually serves.What changes
validate()reads the snapshots through that same drift/malformed split.report predates tests: Xprints as information and never fails; a report Hugo cannot render is still a defect. Adding a test touchesCn1ssDeviceRunnerandport_status.json, and nothing else."Every registered test runs on every port" moves to where nobody can type the answer:
port_status.py coverageruns in the nightly sweep against the reports the ports actually published.not-runalways fails. A test missing from port P's report fails only when some earlier run on another port already carried it — that port dropped the test rather than predating it. No history lookup and no grace period to tune; the reports date themselves.port_status.py provenance, viacheck_port_status_provenance.shinport-status-contract.yml, refuses a report whose results changed while thegenerated_at,commitandrun_urlnaming the run stayed put. That is exactly the edit the twelve fabricated passes were made by.test_port_status.pykeeps no literal totals. It asserts the relationship the numbers stood in for — the suite and the contract describe the same test set, nothing is registered twice, nothing is both a conformance and a performance test — plus floors that catch a collapse.The page separates absent from
not-run, which the old rule had no way to distinguish because it forbade both. Absent means the run predates the test and renders "awaiting this port's next run";not-runmeans the suite ran with the test in its contract and nothing reported back, and stays a defect.The eleven snapshots are refreshed from
port-status-data, which drops the fabrications. The five Apple ports that have not run sinceLogSubclassCaptureTestlanded now say so instead of claiming a pass.Verification
validate,coverageandprovenanceall green.validate_port_status.mjspasses on the rendered page.not-run, and a stamp-preserving edit.validateandprovenance.🤖 Generated with Claude Code