Skip to content

test(string): cover GETDEL nil, empty value and wrong type cases - #3782

Merged
aleksraiden merged 2 commits into
apache:unstablefrom
zbyyehyehyeh:test/getdel_edge_cases
Nov 13, 2026
Merged

aleksraiden merged 2 commits into
apache:unstablefrom
zbyyehyehyeh:test/getdel_edge_cases

Conversation

@zbyyehyehyeh

@zbyyehyehyeh zbyyehyehyeh commented Nov 12, 2026 •

Copy link
Copy Markdown
Contributor

GETDEL is currently covered by a single case that uses .Val(), and
go-redis returns an empty string for both a missing key and an empty value,
so the two behaviours are not distinguished.

This adds coverage for:

  • a missing key returning nil instead of an empty string;
  • an empty string value, which is still distinguishable from a missing key
    and must delete the key;
  • a key that carries a TTL;
  • a non-string value being rejected with WRONGTYPE.

All assertions pass against unstable locally:

./x.py test go build -- -run 'TestString' ./unit/type/strings

AI assistance

Produced with qwen3.7max. I reviewed every line and ran the tests locally.

Why this matters

This is a test-only change with no behaviour change in src/. The existing
case cannot tell a missing key apart from an empty value, because go-redis
collapses both into "", so a regression in GETDEL handling of either path
would go unnoticed. The new assertions use Result()/Exists() so the two
outcomes are actually distinguishable, and cover the TTL and wrong-type paths
that were previously untested.

@github-actions

Copy link
Copy Markdown

Hi @zbyyehyehyeh,

Thank you for your pull request. Please review our Contributing Guide.

Please make sure you understand your changes and explain your reasoning in this pull request. Low-quality pull requests may be closed.

@zbyyehyehyeh

Copy link
Copy Markdown
Contributor Author

Responding to the checklist above.

  • What the change does: adds a test-only subtest for GETDEL covering four
    cases — a missing key (must reply nil, not an empty string), an empty string
    value (must still delete the key), a key carrying a TTL, and a non-string
    value (must fail with WRONGTYPE). No production code is touched.
  • Why: the existing case uses .Val(), and go-redis maps both a nil reply
    and an empty bulk string to "", so the current test cannot tell the two
    apart — a regression on either path would go unnoticed.
  • How I verified it: ./x.py test go build -- -run 'TestString' ./unit/type/strings
    against unstable locally; the new subtests pass under both server
    configurations used by the suite.
  • AI usage: qwen3.7max helped draft the wording and the subtest skeleton.
    I chose the four assertions, checked each against Redis semantics, and ran
    the tests myself.

@aleksraiden
aleksraiden enabled auto-merge (squash) November 13, 2026 07:31
@aleksraiden
aleksraiden merged commit f0356fa into apache:unstable Nov 13, 2026
71 of 73 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants