Patch/testing - #420
Conversation
Scitz0
left a comment
There was a problem hiding this comment.
Review: changes requested
Reviewed PR #420, branch tip dcc4ed1, against main at 6e54441.
Blocking findings
[P1] DRep voting history always errors
drep_voting_power_history.sql:14 changes e to grest.epoch_info_cache, which exposes epoch_no, but lines 14 and 57 still reference e.no. Calls fail with column e.no does not exist; the drep_history wrapper is affected as well.
Both references should use e.epoch_no.
[P1] Current-epoch totals become stale
totals.sql:23-44 bounds donations and withdrawals by epoch_info_cache.i_last_tx_id. However, epoch_info_cache.sql:177-192 deliberately does not refresh that cursor during the current epoch.
After the current epoch's cache row is inserted, later treasury donations, reserve withdrawals, and treasury withdrawals are omitted until the epoch changes. The current epoch needs a live transaction upper bound.
[P1] Current-epoch DRep activity is misclassified
proposal_voting_summary.sql:99-121 uses the same stale i_last_tx_id upper bound.
If an inactive DRep votes or submits an update later in the current epoch, both NOT EXISTS checks can miss that activity. Its voting power is then still counted as inactive, distorting abstain power and voting percentages. The existing last_tx_id_of_interest CTE already provides a live current-epoch bound and should be reused here.
[P1] totals() reports false zeroes for historical epochs
With _epoch_no IS NULL, totals.sql:19-45 still emits one CTE row. Its null transaction bounds cause all three aggregates to coalesce to "0", and that row is joined to every ada_pots epoch.
The documented no-argument form therefore claims zero donations and withdrawals for historical epochs that contain them. Neither test profile detects this because every /totals case supplies _epoch_no=675; an explicit no-parameter case should be added.
[P1] Malformed deployment events can pass with zero tests
preview-api-tests.yml:31 skips the workflow's only job when client_payload.environment is missing or misspelled. GitHub reports conditionally skipped jobs as successful, so a malformed preview-deployed event can produce a green deployment check without executing any tests. See GitHub's job-condition documentation.
The payload should be validated inside a running job and fail explicitly when invalid.
[P1] Concurrent deployments can test the wrong revision
preview-api-tests.yml:34-36 keys the concurrency group by commit even though every run targets the same mutable Preview URL.
Deployments A and B can therefore run concurrently. Once B replaces Preview, A can test B—or a mixture of A and B—while reporting reference A. Use a Preview-wide concurrency group and serialize or cancel stale runs. Ideally, readiness should also attest the revision being served rather than only checking /tip availability.
[P1] non_empty accepts meaningless rows
quality.py:52-59 checks only the truthiness of the top-level container. Responses such as [{}] and [null] therefore count as non-empty.
I reproduced [{}] passing every configured check and OpenAPI response validation for multiple endpoints, including blocks, address_info, pool_info, and drep_info. The quality check should require recursively meaningful content or operation-specific required paths.
Additional findings
[P2] /ogmios JSON-RPC errors appear successful
quality.py:77-95 recognizes only top-level PostgREST-style error envelopes.
I reproduced the actual /ogmios case accepting the following response through status, schema, native-error, null, and quality validation:
{
"jsonrpc": "2.0",
"error": {
"code": -32601,
"message": "Method not found"
}
}The detector should recognize a top-level JSON-RPC error member.
[P2] Disabling epoch sync zeroes the cached current epoch
The aggregate arm at epoch_info_cache.sql:203-224 emits one zero/null aggregate row even when epoch_sync_enabled=false. Because the current epoch is absent from epoch_finalized, that row overwrites valid cached sums, counts, and the last-block timestamp.
Previously, the empty epoch view caused the update to affect no rows. The aggregate branch should emit no row when epoch sync is disabled.
[P2] Persistent transport failures can prevent all reporting
main.py:297-303 writes reports only after every case has run. With a 60-second timeout and two retries per case, a persistently hung deep run can take more than ten hours. If the job is cancelled first, the always-run artifact step has no report to upload.
Reports should be checkpointed incrementally, and the workflow should have an explicit bounded timeout or circuit breaker for widespread transport failure.
Validation performed
- Installed the exact pinned dependencies under Python 3.12.
- Ran the offline unit suite: 8 passed.
- Verified generated specifications match their fragments.
- Compiled the baseline and deep plans: 102 and 204 cases respectively.
- Ran the current Preview API against all 102 baseline and 102 additional deep cases; all completed successfully.
- Confirmed the PR's existing GitHub
syncchecks pass.
The live runs validate the runner's healthy-path behavior against the revision currently served by Preview. They do not establish that the PR's SQL changes were deployed, so they do not invalidate the SQL findings above.
Description