geolocation: add cross-language borsh fixture tests#3682
Conversation
ben-dz
left a comment
There was a problem hiding this comment.
The fixture-test design is sound, but the branch does not build or pass CI.
C1 (Critical): controlplane/telemetry/cmd/geoprobe-agent/main.go:871 is missing the opening { on if len(ml.targets) > 0, so go build fails. Accidental, out-of-scope edit to a production binary; breaks make build/make ci. Does not mask a behavioral change (base is byte-identical except the brace). Restore { or drop the file.
C2 (Critical): the generated .bin/.json fixtures are not committed (only the generate-fixtures crate is), yet the PR wires go test ./sdk/geolocation/go/... into make sdk-test, and loadFixture t.Fatalf's on missing files. This is the established repo pattern left incomplete: geolocation's fixture_test.go is a near-verbatim copy of sdk/revdist/go/fixture_test.go, which runs unconditionally and consumes the committed fixtures in sdk/revdist/testdata/fixtures/ (serviceability and telemetry commit theirs too). The CI sdk-test job has no Rust toolchain and runs no make generate-fixtures, so all three fixture tests fail and take down the SDK test gate. Run the generator and commit the six files. Committing (vs build-in-CI) is correct: golden bytes are frozen and reviewable, so an unintended Rust wire change shows up as a fixture diff — regenerating every CI run would move producer and expected bytes together and mask it.
Medium/Low: no cross-language coverage of the incremental/old-account trailing-field fallback (M1); assertFields doesn't verify total field coverage or full buffer drain (M2); no CI drift-check keeps committed fixtures fresh — a repo-wide gap that is this PR's staleness risk, best fixed with make generate-fixtures + git diff --exit-code in a Rust-enabled job (M3); discarded strconv.ParseUint errors (L1); no generate-fixtures ordering dep (L2); missing generator .gitignore (L3). Both Critical items block merge.
- M1 (Medium) — sdk/geolocation/go/state.go:150-153 and 454-457: GeoProbe.TargetUpdateCount and GeolocationUser.ResultDestination are #[incremental] appended fields with Go fallback logic (decode error -> default), but the fixtures only cover the all-fields-present shape, so the fallback paths have zero cross-language coverage. The serviceability generator has direct precedent (device_legacy / device_future_version). Add truncated geo_probe_legacy / geolocation_user_legacy fixtures (fast-follow acceptable).
- M3 (Medium, repo-wide follow-up) — Nothing keeps the committed fixtures in sync with the Rust structs:
make generate-fixturesruns in no workflow and there is no drift guard. If someone edits a Rust struct and forgets to regenerate, the committed fixture goes stale and the Go/py/ts tests pass against old bytes — the staleness risk raised in review. This already affects revdist/serviceability/telemetry, so it is a repo-wide follow-up rather than a geolocation blocker, but this PR is a natural moment to add a Rust-enabled CI job runningmake generate-fixturesthengit diff --exit-code sdk/*/testdata/fixtures. That yields freshness AND reviewable goldens — better than regenerating every run. - L2 (Low) — Makefile:138-139:
make sdk-testhas no ordering dependency ongenerate-fixtures, so a Rust struct change silently tests against stale fixtures until someone regenerates by hand. Matches sibling-SDK convention, so acceptable — noted to confirm intent. - L3 (Low) — sdk/geolocation/testdata/fixtures/generate-fixtures/ is missing a
.gitignore(withtarget/) that sibling fixture generators commit; a straycargo runbuild dir could be accidentally staged. - No CHANGELOG entry required — this is test-only tooling with no runtime/SDK-consumer behavior change, consistent with prior fixture-test PRs.
|
@ben-dz I’ll follow up with truncated geo_probe_legacy / geolocation_user_legacy fixtures (same pattern as serviceability’s device_legacy) if that's okay |
|
/run-e2e |
|
This looks good to me, but the 2577cb0 commit needs to be signed for branch protection to let me merge it. will couple a rebase onto the latest |
|
@ben-dz done |
|
/run-e2e |
nikw9944
left a comment
There was a problem hiding this comment.
All blocking items from the previous review round are resolved at head 37cdac5: the stray geoprobe-agent edit is gone, the six golden fixtures are committed (verified byte-identical to fresh generator output), ParseUint errors are fatal, a .gitignore was added, a field-count check was added, and a new sdk-fixture-drift CI job regenerates and diffs the goldens (verified green today for all four SDKs, so it won't trip on pre-existing sibling drift). go test / go vet pass, and every field of the three Rust structs is covered by the Go assertions. Remaining findings are medium/low: the borsh-incremental fallback paths (GeoProbe.TargetUpdateCount, GeolocationUser.ResultDestination) still have no legacy/truncated fixture coverage (agreed fast-follow), the tests are decode-only (no re-serialize round-trip / buffer-drain check), the drift job's generator builds are uncached against a 15-minute timeout, and check-fixtures won't catch new untracked fixture files. Diff size is dominated by the generated Cargo.lock and test tooling; no production code change.
- sdk.yml has no
permissions:block, so the new job inherits the default GITHUB_TOKEN scope. Exposure is theoretical (checkout only; pull_request trigger gives fork code a read-only token; PR caches can't overwrite base-branch caches) and the omission is pre-existing across the workflow. Defense-in-depth follow-up: addpermissions: contents: readat the workflow top level. (low) - Pre-existing, no action for this PR: GeoProbe.Deserialize validates Code/ParentDevices lengths only after decoding (sdk/geolocation/go/state.go:138-148), relying on the decoder's remaining-bytes checks, whereas GeolocationUser pre-validates the target count against the buffer size. Optionally align GeoProbe with the GeolocationUser pattern in a follow-up. (low)
- Verified as part of review: regenerating all four SDKs' fixtures produces byte-identical output and a clean tree, so the new sdk-fixture-drift job merges green; go test / go vet pass; every Rust struct field is covered by the Go assertions; struct_size omission and JSON conventions match the serviceability/telemetry precedent. The field-count equality check is stronger than revdist's — worth backporting to sdk/revdist/go/fixture_test.go in a follow-up.
|
We seem to have a problem with CI - |
Summary of Changes
sdk/geolocation/testdata/fixtures/generate-fixtures) serializes real onchain Rust structs via Borsh and writes paired.bin+.jsonfixtures for:GeolocationProgramConfigGeoProbeGeolocationUser(including targets andresult_destination)sdk/geolocation/go/fixture_test.go) that load those fixtures, deserialize using the Go SDK, and validate field-by-field values against the JSON sidecars.make generate-fixturesnow generates geolocation fixturesmake sdk-testnow runsgo test ./sdk/geolocation/go/...Testing Verification
make generate-fixturesgo test ./sdk/geolocation/go/...make sdk-test