test(valkey): fix build_replicaof_config spec assertions and verify_pod_role mocking#2616
Merged
Merged
Conversation
…od_role mocking Three valkey_start_spec.sh tests for build_replicaof_config have been failing on PR #2592 / #2615 since the script evolved its info-logging convention: 1. line 212 this pod is the primary (lexicographic heuristic) 2. line 240 this pod is a replica 3. line 272 Sentinel reports the current master Two underlying drifts: (a) build_replicaof_config logs decisions to stderr (`echo ... >&2`), but the tests asserted on stdout. The tests were written against an earlier revision that used stdout. Switch the assertions to stderr — the existing expected-text fragments match what the script writes; only the stream is different. (b) Test 3 mocked valkey-cli to return the sentinel response and relied on the same mock to satisfy verify_pod_role. The current production code wraps the role probe in `timeout 3 valkey-cli ... info replication`, and `timeout` shell-execs the binary path — it bypasses test-scope shell functions entirely, so the mock never serves the role probe. The fix mocks verify_pod_role directly (the wrapping helper) and dispatches the valkey-cli mock by argv (SENTINEL get-master-addr-by-name vs other). Also updates the expected stderr fragment from the obsolete "Sentinel reports current master" string to "sentinel quorum + role verified", which is what the script emits today on the success path. No production code changes; tests-only fix. Validation: - `shellspec --load-path shellspec addons/valkey/scripts-ut-spec/valkey_start_spec.sh` — 13/0 - `shellspec --load-path shellspec addons/valkey/scripts-ut-spec/` (full valkey suite) — 93/0
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat/valkey-addon #2616 +/- ##
===================================================
Coverage ? 0.00%
===================================================
Files ? 79
Lines ? 10623
Branches ? 0
===================================================
Hits ? 0
Misses ? 10623
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fix the 3
build_replicaof_configtest failures on the Valkey staging PR (#2592) which has been showingshellspec-test FAILUREfor a while:valkey_start_spec.sh:212—writes no replicaof directive (this pod is primary)valkey_start_spec.sh:240—writes replicaof directive pointing to primaryvalkey_start_spec.sh:272—uses Sentinel-reported master as replicaof targetThese are tests-only fixes; no product code changes.
Root causes
Two independent drifts between
valkey-start.shand the tests:Drift A — info-log channel
build_replicaof_configlogs decisions viaecho ... >&2(info-level on stderr), but the tests assert viaThe stdout should include. The tests were written against an earlier revision that emitted to stdout. The expected-text fragments are still correct — only the stream is wrong.Fix: switch the 3 assertions from
stdouttostderr.Drift B — verify_pod_role wrapping
timeoutTest 3 mocked
valkey-clito return Sentinel's response and relied on the same mock to satisfyverify_pod_role. The production code now wraps the role probe in:role=$(timeout 3 ${cli_base} -h "${fqdn}" info replication 2>/dev/null | ...)timeoutshell-execs the binary path, bypassing test-scope shell functions, so the mock never serves the role probe. The script logsINFO: quorum elected ... but role='<unreachable>' — retrying in 5s.12 times then falls through to pod scan / lexicographic bootstrap, which is why the test was looking for an obsolete"Sentinel reports current master"fragment that no longer exists in the success path.Fix:
verify_pod_roledirectly (the wrapping helper) — robust to thetimeoutshell-exec barriervalkey-climock by argv:SENTINEL get-master-addr-by-namereturns master FQDN + port, other paths no-op"sentinel quorum + role verified"(what the script emits today on the success path)Validation
shellspec --load-path shellspec addons/valkey/scripts-ut-spec/valkey_start_spec.sh→ 13 examples, 0 failuresshellspec --load-path shellspec addons/valkey/scripts-ut-spec/(full valkey suite) → 93 examples, 0 failuresCI on this PR (and the rebased PR #2592) should drop from
743 examples, 3 failuresto743 examples, 0 failures.Why this is tests-only
addons/valkey/scripts/valkey-start.shis unchanged. Onlyaddons/valkey/scripts-ut-spec/valkey_start_spec.shis touched, and only the 3 brokenItblocks are touched. Other 10 examples in the file are untouched and still pass.