diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml new file mode 100644 index 0000000..ea1cbf4 --- /dev/null +++ b/.github/workflows/ci.yml @@ -0,0 +1,48 @@ +# The harness gets checked here. The comparison itself does not run on a shared +# runner yet, because it wants a Redis built from the release tarball and that +# build is slower than everything else in this file put together. It runs on the +# boxes, next to the benchmarks, against the same binaries. +# +# What is worth checking here is the part that can be wrong without anyone +# noticing: the corpus files parse, the register refuses an entry with no reason +# on it, and the RESP reader keeps two different spellings of nothing apart. +name: ci + +on: + push: + branches: [main] + pull_request: + +permissions: + contents: read + +concurrency: + group: ci-${{ github.ref }} + cancel-in-progress: true + +env: + CARGO_TERM_COLOR: always + CARGO_INCREMENTAL: 0 + RUSTFLAGS: -D warnings + +jobs: + check: + runs-on: ubuntu-latest + timeout-minutes: 10 + steps: + - uses: actions/checkout@v7.0.1 + - uses: dtolnay/rust-toolchain@stable + with: + components: rustfmt, clippy + - uses: Swatinem/rust-cache@v2.8.2 + - run: cargo fmt --all --check + - run: cargo clippy --workspace --all-targets + - run: cargo test --workspace + + shell: + runs-on: ubuntu-latest + timeout-minutes: 5 + steps: + - uses: actions/checkout@v7.0.1 + - name: shellcheck + run: shellcheck -s sh suite/*.sh diff --git a/.gitignore b/.gitignore new file mode 100644 index 0000000..c780bd1 --- /dev/null +++ b/.gitignore @@ -0,0 +1,4 @@ +/target +# The vendored Redis tree. suite/fetch.sh puts it here and it is their code at a +# pinned version, not ours, so it does not belong in our history. +/redis diff --git a/Cargo.lock b/Cargo.lock new file mode 100644 index 0000000..99f4694 --- /dev/null +++ b/Cargo.lock @@ -0,0 +1,7 @@ +# This file is automatically @generated by Cargo. +# It is not intended for manual editing. +version = 4 + +[[package]] +name = "yo-compat" +version = "0.1.0" diff --git a/Cargo.toml b/Cargo.toml new file mode 100644 index 0000000..f00f2f0 --- /dev/null +++ b/Cargo.toml @@ -0,0 +1,25 @@ +[workspace] +resolver = "3" +members = ["crates/yo-compat"] + +[workspace.package] +version = "0.1.0" +edition = "2024" +rust-version = "1.94" +license = "MIT OR Apache-2.0" +repository = "https://github.com/tamnd/yo-compat" +homepage = "https://yo.tamnd.dev" +authors = ["tamnd "] + +[workspace.lints.rust] +missing_docs = "warn" + +[workspace.lints.clippy] +alloc_instead_of_core = "warn" + +# Nothing here is on a measured path. This binary talks to two servers over +# loopback and compares what they say, so the build wants to be quick and the +# panics want to be readable. +[profile.release] +opt-level = 2 +debug = 1 diff --git a/README.md b/README.md index 2ac5d45..fefa2ac 100644 --- a/README.md +++ b/README.md @@ -1,2 +1,46 @@ # yo-compat -Redis compatibility suite for yo: the command scoreboard, the differential harness, the RDB corpus, the client library matrix and the divergence register + +Redis compatibility for [yo](https://github.com/tamnd/yo), checked against a real Redis rather than against the documentation. + +There are two halves here and they catch different things. + +## The real Redis test suite + +`suite/fetch.sh` downloads the Redis release tarball at a pinned version and drops its `tests/` directory and its `runtest` script into `redis/` unmodified. Not a rewrite of the suite, not a subset we typed out ourselves, and not a description of what we think it checks. It is their tests, run against our server. + +``` +suite/fetch.sh +suite/runtest.sh +``` + +That starts `yodb serve` and points the suite at it with `runtest --host --port`, which is the suite's external mode. With no arguments it runs the files listed in `suite/passing.txt`, and `--all` runs everything under `unit/`. + +Every failure is real. A test that fails because we have not implemented the command yet is still a compatibility failure, it is just one with a date on it. Nothing is patched out to make the number look better, and moving a file into `suite/passing.txt` is a decision somebody makes on purpose in a diff. + +## The differential harness + +`yocompat` starts a real Redis and starts yodb, sends both of them the same commands, and reports every reply that is not byte for byte the same. + +``` +cargo build --release +./target/release/yocompat --redis /opt/yo-bench/bin/redis-server-8.10.1 +./target/release/yocompat --resp3 +``` + +This catches what their suite cannot. Their tests assert what Redis's own developers thought was worth asserting, which leaves out everything they take for granted: that a missing key comes back as `$-1` and not `_`, that `INCRBYFLOAT` replies with a bulk string and not a double, that `GETRANGE` on a missing key is an empty bulk string and not a null, that the first word of an error is `WRONGTYPE` and not `ERR`. Clients branch on all of it. + +The corpus files under `corpus/` are lists of commands with no expected answers in them, because the expected answer is whatever Redis says. `--resp3` sends `HELLO 3` first and runs the same corpus again, which is where the reply types that change between protocol versions show up. + +Divergences are sorted worst first in the report. Accepting a command Redis refuses is a correctness bug that will corrupt data for a client that relied on the refusal. Refusing it with different wording is a bug too, but a smaller one. + +## The register + +`divergences.toml` is the list of differences we have decided to live with, each with a reason written next to it. Anything in it is counted and not reported. Anything else fails the run. + +The file is deliberately awkward to add to. A divergence with no reason on it does not load at all, which means saying "we behave differently here and this is why" has to be a diff somebody wrote and somebody else read. + +## What is not here yet + +The command scoreboard, which is the other thing this repository is supposed to hold: a generated table of every Redis command against what yo does with it. Right now the honest version of that table is short, and it belongs here rather than in a README that can go stale quietly. + +Also the RDB corpus and the client library matrix. Both wait on yo having something to load and something for a real client to talk to. diff --git a/corpus/errors.txt b/corpus/errors.txt new file mode 100644 index 0000000..4532f6a --- /dev/null +++ b/corpus/errors.txt @@ -0,0 +1,70 @@ +# Being wrong the same way they are. +# +# Clients branch on error text. Not on an error type, because RESP does not have +# one, on the first word and sometimes on the whole string. A server that +# refuses the right commands with the wrong words breaks a client library in a +# way that looks like a bug in the library, so the wording is part of the +# protocol whether anyone likes it or not. + +# Wrong number of arguments, at both ends. +GET +GET one two +SET +SET onlykey +SET key value extra +APPEND +APPEND key +STRLEN +STRLEN a b +INCR +INCR a b +INCRBY key +DECRBY key +INCRBYFLOAT key +GETRANGE key +GETRANGE key 0 +GETRANGE key 0 1 2 +SETRANGE key +SETRANGE key 0 +GETSET key +GETDEL +GETDEL a b +MSET +MSET onlykey +MSET a 1 b +MSETNX +MSETNX a 1 b +MGET +SETEX key +SETEX key 100 +PSETEX key 100 +SETNX key +LCS key +LCS a b c + +# Arguments that are the wrong shape rather than the wrong number. +SETRANGE key notanoffset value +SETRANGE key -1 value +GETRANGE key notastart 10 +GETRANGE key 0 notanend +SETEX key notaseconds value +PSETEX key notamillis value +LCS a b NOTANOPTION +LCS a b IDX MINMATCHLEN notanumber +LCS a b LEN IDX + +# A command nobody has. +NOTACOMMAND +NOTACOMMAND with args +GETT key + +# Subcommands that do not exist under commands that do. +CONFIG NOTASUBCOMMAND +CONFIG +COMMAND NOTASUBCOMMAND + +# Casing is not supposed to matter anywhere. +set lower value +GeT lower +INCRBYFLOAT lower 1 +sTrLeN lower diff --git a/corpus/expire.txt b/corpus/expire.txt new file mode 100644 index 0000000..c00d08d --- /dev/null +++ b/corpus/expire.txt @@ -0,0 +1,71 @@ +# Expiry, without ever asking what the remaining time is. +# +# TTL is not in here on purpose. A corpus that asks two servers how many +# milliseconds are left on a key is a corpus that fails on a slow afternoon, and +# a flaky compatibility test gets ignored, which is worse than not having it. +# What is checked instead is the part that is deterministic: which option +# combinations are accepted, what each one replies, whether the value is still +# there afterwards, and which ones are refused. + +SET vanish value EX 100 +GET vanish +SET vanish value PX 100000 +GET vanish +SET vanish value EXAT 33229822800 +GET vanish +SET vanish value PXAT 33229822800000 +GET vanish +SET vanish value KEEPTTL +GET vanish + +SETEX seconds 100 value +GET seconds +PSETEX millis 100000 value +GET millis + +# A zero or negative expiry is refused rather than treated as delete now, and +# the key it was aimed at is left alone. +SET keep original +SET keep replaced EX 0 +GET keep +SET keep replaced EX -1 +GET keep +SETEX zero 0 value +GET zero +SETEX negative -1 value +PSETEX zeroms 0 value +SETEX huge 9999999999999999 value + +# The options that cannot be used together. +SET both value EX 100 PX 100000 +SET both value EX 100 KEEPTTL +SET both value NX XX +SET both value EX +SET both value EX notanumber +SET both value PERSIST + +# GETEX is the one that reads and changes the expiry in the same breath, and +# with no options at all it changes nothing. +SET readme value EX 100 +GETEX readme +GET readme +GETEX readme PERSIST +GET readme +GETEX readme EX 200 +GETEX readme PX 200000 +GETEX readme EXAT 33229822800 +GETEX readme PXAT 33229822800000 +GETEX missing +GETEX missing PERSIST +GETEX readme EX 100 PERSIST +GETEX readme EX notanumber +GETEX readme NOTANOPTION + +# An expiry that has already passed is a delete, and the reply says so from +# whichever side you look at it. +SET gone value EXAT 1 +GET gone +SET gone2 value PXAT 1 +GET gone2 +GETEX readme EXAT 1 +GET readme diff --git a/corpus/numbers.txt b/corpus/numbers.txt new file mode 100644 index 0000000..bbddd60 --- /dev/null +++ b/corpus/numbers.txt @@ -0,0 +1,85 @@ +# The counters, and what they consider a number. +# +# Redis is stricter here than people expect and the strictness is the +# compatible part. A value with a leading space is not a number. A value with a +# trailing newline is not a number. An integer stored as "010" is not the same +# as one stored as "10" once you increment it. Every one of those is a place an +# implementation written from the documentation gets it wrong. + +INCR counter +INCR counter +GET counter +INCRBY counter 10 +INCRBY counter -20 +GET counter +DECR counter +DECRBY counter 5 +GET counter + +# Starting from a string that happens to look like a number. +SET fromstring 100 +INCR fromstring +GET fromstring +SET negative -100 +INCR negative +GET negative + +# Leading zeroes are stored as typed and are not a number to increment. +SET padded 010 +INCR padded +GET padded + +# Whitespace either side is not a number, in either direction. +SET leading " 11" +INCR leading +SET trailing "11 " +INCR trailing +SET newline "11\n" +INCR newline + +# Empty is not zero. +SET blank "" +INCR blank + +# Not numbers at all. +SET word hello +INCR word +INCRBY word 1 +DECR word +INCRBYFLOAT word 1.0 + +# The ends of the range. One past the top is an error and not a wrap. +SET big 9223372036854775807 +INCR big +INCRBY big 1 +SET small -9223372036854775808 +DECR small +DECRBY small 1 +SET big2 9223372036854775806 +INCR big2 +GET big2 + +# The increment itself has to be a number, and has to fit. +INCRBY counter notanumber +INCRBY counter 9223372036854775808 +INCRBY counter 1.5 + +# INCRBYFLOAT is a different command with different rules. It replies with a +# bulk string, it trims trailing zeroes, and it takes exponent notation. +SET float 10.5 +INCRBYFLOAT float 0.1 +INCRBYFLOAT float -5 +INCRBYFLOAT float 5.0e3 +GET float +SET whole 10 +INCRBYFLOAT whole 0.1 +INCRBYFLOAT whole -0.1 +SET intish 3.0 +INCRBYFLOAT intish 1.000000000000000005 +INCRBYFLOAT newfloat 1.5 +GET newfloat +INCRBYFLOAT float notafloat +INCRBYFLOAT float nan +INCRBYFLOAT float inf +SET hexish 0x10 +INCRBYFLOAT hexish 1 diff --git a/corpus/protocol.txt b/corpus/protocol.txt new file mode 100644 index 0000000..d8d6ceb --- /dev/null +++ b/corpus/protocol.txt @@ -0,0 +1,47 @@ +# The connection itself. +# +# HELLO is missing from this file even though it is the most protocol shaped +# command there is, because its reply carries a server version, a connection id +# and a role, none of which can match and none of which should. It is sent by +# the harness under --resp3 and its reply is deliberately not compared. What is +# compared is everything that comes after it, which is where switching to RESP3 +# actually shows. + +PING +PING hello +PING "two words" +PING one two +ECHO hello +ECHO "" +ECHO "with spaces" +ECHO +ECHO one two + +SELECT 0 +SELECT 1 +SELECT 16 +SELECT -1 +SELECT notanumber +SELECT +SELECT 0 + +# Under RESP3 these are the replies whose type changes, which is the whole +# reason the harness can run the same corpus twice. A double comes back as a +# double instead of a bulk string, a null as an underscore instead of $-1. +# +# They come before RESET rather than after it, because RESET puts the connection +# back on RESP2 and a RESP3 section underneath it would quietly be testing RESP2 +# twice. +SET num 10 +INCRBYFLOAT num 0.5 +GET missingkey +MGET missingkey alsomissing +GETDEL missingkey +LCS missingkey alsomissing IDX + +# RESET is supposed to put the connection back the way it found it, and to say +# so with a simple string rather than OK. +SET before reset +RESET +GET before +PING diff --git a/corpus/strings.txt b/corpus/strings.txt new file mode 100644 index 0000000..c924dfe --- /dev/null +++ b/corpus/strings.txt @@ -0,0 +1,120 @@ +# Plain string commands, and mostly their edges rather than their happy paths. +# +# The happy path agrees on every server that has ever shipped. What does not +# agree is the empty value, the offset past the end, the negative range, the +# count that comes back as an integer on one side and a bulk string on the +# other, and the difference between a key that holds nothing and a key that is +# not there. Those are the lines here. + +SET plain hello +GET plain +GET missing +SET plain world +GET plain + +# An empty value is a value. A server that turns it back into a missing key is +# wrong in a way that is easy to ship and hard to notice. +SET empty "" +GET empty +STRLEN empty +STRLEN missing +GET empty + +SET withspace "two words" +GET withspace +STRLEN withspace + +# APPEND onto nothing creates the key, and the reply is the new length rather +# than the number of bytes appended. +APPEND fresh abc +GET fresh +APPEND fresh def +STRLEN fresh +APPEND fresh "" +GET fresh + +# GETRANGE indexes from both ends, clamps rather than erroring, and an empty +# result is an empty bulk string and not a null. +SET range "This is a string" +GETRANGE range 0 3 +GETRANGE range -3 -1 +GETRANGE range 0 -1 +GETRANGE range 10 100 +GETRANGE range 100 200 +GETRANGE range -1 -100 +GETRANGE range 0 0 +GETRANGE missing 0 -1 +SUBSTR range 0 3 + +# SETRANGE past the end pads with zero bytes, which is the one place a string +# command quietly allocates. SETRANGE with an empty value on a missing key does +# nothing at all and says zero. +SET pad "Hello World" +SETRANGE pad 6 "Redis" +GET pad +SETRANGE padded 5 "x" +STRLEN padded +GET padded +SETRANGE nothing 0 "" +GET nothing + +# SETNX and the SET flags are two spellings of the same idea and they do not +# reply the same way. One says an integer, the other says OK or a null. +SETNX once first +GET once +SETNX once second +GET once +SET once third NX +GET once +SET once fourth XX +GET once +SET fresh2 value NX +GET fresh2 +SET missing2 value XX +GET missing2 + +# GETSET and GETDEL both hand back what was there, including when nothing was. +SET old before +GETSET old after +GET old +GETSET brandnew value +GETDEL old +GET old +GETDEL old +GETDEL missing + +# SET GET is the modern spelling of GETSET and it has its own edge: on a key +# that is not there it replies null and still writes. +SET newkey value GET +GET newkey +SET newkey second GET + +# MSET is always OK, MSETNX is all or nothing, and MGET fills the gaps with +# nulls rather than skipping them. +MSET a 1 b 2 c 3 +MGET a b c +MGET a missing c +MGET missing missing +MSETNX d 4 e 5 +MGET d e +MSETNX e 6 f 7 +GET f +GET e + +# Long values, because the encoding switches somewhere and the switch should +# not be visible from the outside. +SET long "0123456789012345678901234567890123456789012345678901234567890123456789" +STRLEN long +GETRANGE long 60 -1 + +# LCS is the one string command with a shape to it. LEN is an integer, IDX is a +# map, and the match list is ordered. +MSET key1 "ohmytext" key2 "mynewtext" +LCS key1 key2 +LCS key1 key2 LEN +LCS key1 key2 IDX +LCS key1 key2 IDX MINMATCHLEN 4 +LCS key1 key2 IDX WITHMATCHLEN +LCS key1 key2 IDX MINMATCHLEN 4 WITHMATCHLEN +LCS key1 nothere +LCS nothere alsonothere diff --git a/crates/yo-compat/Cargo.toml b/crates/yo-compat/Cargo.toml new file mode 100644 index 0000000..fb5b35b --- /dev/null +++ b/crates/yo-compat/Cargo.toml @@ -0,0 +1,23 @@ +[package] +name = "yo-compat" +description = "Sends the same commands to yo and to a real Redis and reports every reply that differs." +version.workspace = true +edition.workspace = true +rust-version.workspace = true +license.workspace = true +repository.workspace = true +authors.workspace = true + +[lints] +workspace = true + +# No dependencies. A RESP client from crates.io is written to be pleasant, which +# means it normalises: it turns a null bulk string and a null array into the same +# None, and a simple string and a bulk string into the same String. Those are +# exactly the differences this binary exists to catch, so the client here is our +# own and it is deliberately unhelpful. +[dependencies] + +[[bin]] +name = "yocompat" +path = "src/main.rs" diff --git a/crates/yo-compat/src/corpus.rs b/crates/yo-compat/src/corpus.rs new file mode 100644 index 0000000..dfa856b --- /dev/null +++ b/crates/yo-compat/src/corpus.rs @@ -0,0 +1,210 @@ +//! Corpus files, and the register of divergences we have already agreed to. +//! +//! A corpus file is a list of commands, one per line, written the way you would +//! type them into redis-cli. There are no expected answers in it, because the +//! expected answer is whatever Redis says, and writing them down by hand would +//! turn this into a test of our reading of the documentation. + +use std::collections::BTreeMap; +use std::path::Path; + +/// One command out of a corpus file, with the line it came from so a failure +/// report points at something you can open. +pub struct Step { + pub line: usize, + pub args: Vec, +} + +pub struct Corpus { + pub steps: Vec, +} + +impl Corpus { + pub fn load(path: &Path) -> Result { + let text = std::fs::read_to_string(path).map_err(|e| format!("{}: {e}", path.display()))?; + let mut steps = Vec::new(); + for (at, line) in text.lines().enumerate() { + let line = line.trim(); + if line.is_empty() || line.starts_with('#') { + continue; + } + let args = split(line).map_err(|e| format!("{}:{}: {e}", path.display(), at + 1))?; + if !args.is_empty() { + steps.push(Step { line: at + 1, args }); + } + } + Ok(Corpus { steps }) + } +} + +/// Split a line into arguments, respecting double quotes. +/// +/// Quotes matter more than they look like they should. Half the interesting +/// cases in a string corpus are about empty values, values with spaces in them +/// and values that are only whitespace, and none of those can be written down +/// without a way to quote them. +fn split(line: &str) -> Result, String> { + let mut args = Vec::new(); + let mut cur = String::new(); + let mut in_quotes = false; + let mut started = false; + let mut chars = line.chars(); + + while let Some(c) = chars.next() { + match c { + '"' => { + in_quotes = !in_quotes; + started = true; + } + '\\' if in_quotes => match chars.next() { + Some('n') => cur.push('\n'), + Some('r') => cur.push('\r'), + Some('t') => cur.push('\t'), + Some('\\') => cur.push('\\'), + Some('"') => cur.push('"'), + Some(other) => cur.push(other), + None => return Err("a line ended in a backslash".to_string()), + }, + c if c.is_whitespace() && !in_quotes => { + if started || !cur.is_empty() { + args.push(std::mem::take(&mut cur)); + started = false; + } + } + c => { + cur.push(c); + started = true; + } + } + } + if in_quotes { + return Err("a quote was never closed".to_string()); + } + if started || !cur.is_empty() { + args.push(cur); + } + Ok(args) +} + +/// The divergences we have decided to live with. +/// +/// A divergence in here is a claim with a reason attached, not a test being +/// switched off. Everything else that differs fails the run. The point of the +/// file is that saying "we behave differently here and this is why" has to be a +/// diff somebody wrote and somebody else read. +#[derive(Default)] +pub struct Register { + /// corpus file name to the command prefixes excused in it. + excused: BTreeMap>, +} + +impl Register { + pub fn load(root: &Path) -> Result { + let path = root.join("divergences.toml"); + let Ok(text) = std::fs::read_to_string(&path) else { + return Ok(Register::default()); + }; + Register::parse(&text).map_err(|e| format!("{}: {e}", path.display())) + } + + /// A small hand written reader for a small hand written file. It wants + /// `[[divergence]]` tables with a `corpus`, a `command` and a `reason`, and + /// it insists on the reason, because a divergence with no reason on it is + /// the thing this file exists to prevent. + fn parse(text: &str) -> Result { + let mut out = Register::default(); + let mut corpus: Option = None; + let mut command: Option = None; + let mut reason: Option = None; + + let mut flush = |corpus: &mut Option, + command: &mut Option, + reason: &mut Option| + -> Result<(), String> { + match (corpus.take(), command.take(), reason.take()) { + (None, None, None) => Ok(()), + (Some(c), Some(cmd), Some(r)) if !r.is_empty() => { + out.excused.entry(c).or_default().push(cmd); + Ok(()) + } + _ => Err("every divergence needs a corpus, a command and a reason".to_string()), + } + }; + + for line in text.lines() { + let line = line.trim(); + if line.is_empty() || line.starts_with('#') { + continue; + } + if line == "[[divergence]]" { + flush(&mut corpus, &mut command, &mut reason)?; + continue; + } + let Some((key, value)) = line.split_once('=') else { + return Err(format!("not a key and a value: {line}")); + }; + let value = value.trim().trim_matches('"').to_string(); + match key.trim() { + "corpus" => corpus = Some(value), + "command" => command = Some(value), + "reason" => reason = Some(value), + other => return Err(format!("no such field: {other}")), + } + } + flush(&mut corpus, &mut command, &mut reason)?; + Ok(out) + } + + /// Is this mismatch one we have already signed for? + /// + /// Matched on a prefix so a register entry can cover a command rather than + /// one exact set of arguments. `SETRANGE` excuses every SETRANGE in that + /// file, `SETRANGE key 5` excuses only the ones that start that way. + pub fn excuses(&self, corpus: &str, command: &str) -> bool { + let Some(prefixes) = self.excused.get(corpus) else { + return false; + }; + let lower = command.to_ascii_lowercase(); + prefixes + .iter() + .any(|p| lower.starts_with(&p.to_ascii_lowercase())) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn a_quoted_empty_value_is_an_argument() { + let args = split(r#"SET key """#).unwrap(); + assert_eq!(args, vec!["SET", "key", ""]); + } + + #[test] + fn a_value_with_spaces_survives() { + let args = split(r#"SET key "two words""#).unwrap(); + assert_eq!(args, vec!["SET", "key", "two words"]); + } + + #[test] + fn an_unclosed_quote_is_an_error_and_not_a_guess() { + assert!(split(r#"SET key "unfinished"#).is_err()); + } + + #[test] + fn a_divergence_without_a_reason_does_not_load() { + let text = "[[divergence]]\ncorpus = \"string.txt\"\ncommand = \"OBJECT\"\n"; + assert!(Register::parse(text).is_err()); + } + + #[test] + fn the_register_excuses_by_prefix_and_nothing_else() { + let text = "[[divergence]]\ncorpus = \"string.txt\"\n\ + command = \"OBJECT ENCODING\"\nreason = \"we have one encoding\"\n"; + let r = Register::parse(text).unwrap(); + assert!(r.excuses("string.txt", "OBJECT ENCODING key")); + assert!(!r.excuses("string.txt", "OBJECT REFCOUNT key")); + assert!(!r.excuses("expire.txt", "OBJECT ENCODING key")); + } +} diff --git a/crates/yo-compat/src/main.rs b/crates/yo-compat/src/main.rs new file mode 100644 index 0000000..8495824 --- /dev/null +++ b/crates/yo-compat/src/main.rs @@ -0,0 +1,303 @@ +//! `yocompat`, the differential tester. +//! +//! It sends the same command to a real Redis and to yodb, reads both replies, +//! and complains when they are not the same bytes. That is the whole idea. A +//! compatibility test written from the documentation tests the documentation, +//! and Redis's real behaviour and Redis's documented behaviour part company +//! often enough that the only thing worth comparing against is a running +//! server. +//! +//! Redis's own Tcl suite is the other half of this repository and it is the +//! stronger of the two where it applies. This half covers what that one cannot: +//! the exact reply type, the exact error word, and RESP3 shapes, on commands +//! their suite happens not to reach. + +mod corpus; +mod resp; + +use std::path::{Path, PathBuf}; +use std::process::{Child, Command, Stdio}; +use std::time::{Duration, Instant}; + +use corpus::{Corpus, Step}; +use resp::{Conn, Reply}; + +const USAGE: &str = "\ +yocompat, the differential tester + +usage: + yocompat [options] [corpus ...] + +options: + --yodb PATH our server, ../yo/target/release/yodb by default + --redis PATH a real redis-server, found on PATH by default + --resp3 say HELLO 3 first, so the RESP3 shapes get compared + --quiet only print divergences and the summary + --corpus DIR where the corpus files are, corpus/ by default + +exit codes: + 0 every reply matched, or every mismatch was already registered + 1 something diverged that is not in the register + 2 the arguments did not make sense, or a server would not start +"; + +fn main() -> std::process::ExitCode { + match run() { + Ok(code) => code, + Err(e) => { + eprintln!("yocompat: {e}"); + std::process::ExitCode::from(2) + } + } +} + +struct Opts { + yodb: PathBuf, + redis: PathBuf, + resp3: bool, + quiet: bool, + dir: PathBuf, + files: Vec, +} + +/// One command where the two servers disagreed. +struct Divergence { + file: String, + line: usize, + command: String, + kind: &'static str, + ours: String, + theirs: String, +} + +/// Say what sort of disagreement this is. +/// +/// Not every mismatch costs the same. Accepting a command Redis refuses is a +/// correctness bug and a client that relies on the refusal will corrupt data +/// against us. Refusing it with a differently worded message is a bug too, but a +/// smaller one, and a client that branches on the first word will not even +/// notice. Sorting them apart in the report is the difference between a list you +/// can work through and a list you scroll past. +fn classify(ours: &Result, theirs: &Result) -> &'static str { + let (Ok(ours), Ok(theirs)) = (ours, theirs) else { + return "one of them did not answer"; + }; + match (ours.error_word(), theirs.error_word()) { + (Some(a), Some(b)) if a == b => "same error, different wording", + (Some(_), Some(_)) => "both refused it, with different errors", + (None, Some(_)) => "we accepted what redis refuses", + (Some(_), None) => "we refused what redis accepts", + (None, None) => "different reply", + } +} + +fn run() -> Result { + let Some(opts) = parse()? else { + return Ok(std::process::ExitCode::SUCCESS); + }; + + let files = if opts.files.is_empty() { + let mut v: Vec = std::fs::read_dir(&opts.dir) + .map_err(|e| format!("{}: {e}", opts.dir.display()))? + .filter_map(|e| e.ok()) + .map(|e| e.path()) + .filter(|p| p.extension().is_some_and(|x| x == "txt")) + .collect(); + v.sort(); + v + } else { + opts.files.clone() + }; + if files.is_empty() { + return Err(format!("no corpus files in {}", opts.dir.display())); + } + + let register = corpus::Register::load(opts.dir.parent().unwrap_or(Path::new(".")))?; + + let mut diverged: Vec = Vec::new(); + let mut registered = 0usize; + let mut compared = 0usize; + + for file in &files { + let corpus = Corpus::load(file)?; + let name = file + .file_name() + .unwrap_or_default() + .to_string_lossy() + .into_owned(); + if !opts.quiet { + eprintln!("== {name}: {} commands", corpus.steps.len()); + } + + // Both servers get a fresh process per corpus file. We have no + // FLUSHALL yet, and a corpus that starts from whatever the last one + // left behind is a corpus that passes or fails depending on the order + // the files were read in. + let mut ours = Server::start(&opts.yodb, &["serve", "--port", "7521"], 7521)?; + let mut theirs = Server::start( + &opts.redis, + &["--port", "7522", "--save", "", "--appendonly", "no"], + 7522, + )?; + + let mut a = Conn::connect(7521).map_err(|e| format!("yodb: {e}"))?; + let mut b = Conn::connect(7522).map_err(|e| format!("redis: {e}"))?; + + if opts.resp3 { + let hello = vec!["HELLO".to_string(), "3".to_string()]; + let _ = talk(&mut a, &hello); + let _ = talk(&mut b, &hello); + } + + for step in &corpus.steps { + let Step { line, args } = step; + let mine = talk(&mut a, args); + let theirs_reply = talk(&mut b, args); + compared += 1; + + let kind = classify(&mine, &theirs_reply); + let mine = mine + .map(|r| r.render()) + .unwrap_or_else(|e| format!("<{e}>")); + let theirs_reply = theirs_reply + .map(|r| r.render()) + .unwrap_or_else(|e| format!("<{e}>")); + + if mine == theirs_reply { + continue; + } + let command = args.join(" "); + if register.excuses(&name, &command) { + registered += 1; + continue; + } + diverged.push(Divergence { + file: name.clone(), + line: *line, + command, + kind, + ours: mine, + theirs: theirs_reply, + }); + } + + ours.stop(); + theirs.stop(); + } + + println!("\n{compared} commands compared against redis"); + if registered > 0 { + println!("{registered} known divergences, all of them in divergences.toml"); + } + + if diverged.is_empty() { + println!("nothing else diverged"); + return Ok(std::process::ExitCode::SUCCESS); + } + + println!("\n{} unregistered divergences:\n", diverged.len()); + // Worst first. A wall of wording differences at the top of the report is how + // the one command we get outright wrong gets missed. + let order = [ + "we accepted what redis refuses", + "different reply", + "we refused what redis accepts", + "both refused it, with different errors", + "one of them did not answer", + "same error, different wording", + ]; + for kind in order { + let group: Vec<&Divergence> = diverged.iter().filter(|d| d.kind == kind).collect(); + if group.is_empty() { + continue; + } + println!("-- {kind} ({})", group.len()); + for d in group { + println!("{}:{} {}", d.file, d.line, d.command); + println!(" redis: {}", d.theirs); + println!(" yo: {}", d.ours); + } + println!(); + } + println!( + "\nEach of these is either a bug to fix or a line to add to divergences.toml with a reason." + ); + Ok(std::process::ExitCode::FAILURE) +} + +fn talk(conn: &mut Conn, args: &[String]) -> Result { + conn.send(args).map_err(|e| e.to_string())?; + conn.read().map_err(|e| e.to_string()) +} + +/// A server under test, started and stopped by us. +struct Server { + child: Child, +} + +impl Server { + fn start(bin: &Path, args: &[&str], port: u16) -> Result { + let child = Command::new(bin) + .args(args) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .spawn() + .map_err(|e| format!("{}: {e}", bin.display()))?; + + let deadline = Instant::now() + Duration::from_secs(15); + while Instant::now() < deadline { + if let Ok(mut c) = Conn::connect(port) + && c.send(&["PING".to_string()]).is_ok() + && c.read().is_ok() + { + return Ok(Server { child }); + } + std::thread::sleep(Duration::from_millis(25)); + } + let mut s = Server { child }; + s.stop(); + Err(format!("{} never answered on {port}", bin.display())) + } + + fn stop(&mut self) { + let _ = self.child.kill(); + let _ = self.child.wait(); + } +} + +fn parse() -> Result, String> { + let mut o = Opts { + yodb: PathBuf::from("../yo/target/release/yodb"), + redis: PathBuf::from("redis-server"), + resp3: false, + quiet: false, + dir: PathBuf::from("corpus"), + files: Vec::new(), + }; + + let args: Vec = std::env::args().skip(1).collect(); + let mut at = 0; + while at < args.len() { + let arg = args[at].as_str(); + at += 1; + let mut value = || -> Result { + let v = args.get(at).ok_or(format!("{arg} needs a value"))?.clone(); + at += 1; + Ok(v) + }; + match arg { + "-h" | "--help" => { + print!("{USAGE}"); + return Ok(None); + } + "--resp3" => o.resp3 = true, + "--quiet" => o.quiet = true, + "--yodb" => o.yodb = PathBuf::from(value()?), + "--redis" => o.redis = PathBuf::from(value()?), + "--corpus" => o.dir = PathBuf::from(value()?), + other if other.starts_with('-') => return Err(format!("no such option: {other}")), + other => o.files.push(PathBuf::from(other)), + } + } + Ok(Some(o)) +} diff --git a/crates/yo-compat/src/resp.rs b/crates/yo-compat/src/resp.rs new file mode 100644 index 0000000..337c8ac --- /dev/null +++ b/crates/yo-compat/src/resp.rs @@ -0,0 +1,238 @@ +//! A RESP client that is deliberately naive. +//! +//! It has to read whatever both servers send without helping either of them. +//! A client with a type system that turns two different wire encodings into the +//! same Rust value would hide exactly the bugs this repository exists to find, +//! so a reply is kept as its shape and its bytes and compared that way. + +use std::io::{self, BufRead, BufReader, Write}; +use std::net::TcpStream; +use std::time::Duration; + +/// A reply, kept in the shape it arrived in. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum Reply { + /// `+OK` + Simple(Vec), + /// `-ERR something` + Error(Vec), + /// `:42` + Int(Vec), + /// `$3\r\nfoo`, and `=` for a verbatim string, kept apart because they are + /// different types on the wire even when they carry the same bytes. + Bulk(Vec), + /// `=14\r\ntxt:...` + Verbatim(Vec), + /// `!21\r\n...`, a RESP3 bulk error. + BulkError(Vec), + /// `,3.14` + Double(Vec), + /// `#t` + Bool(Vec), + /// `(1234567890` + BigNumber(Vec), + /// `_`, and `$-1` and `*-1`, which are the RESP2 spellings of the same idea + /// and are kept apart from it. + Null, + /// `$-1` + NullBulk, + /// `*-1` + NullArray, + /// `*2\r\n...` + Array(Vec), + /// `~2\r\n...` + Set(Vec), + /// `>2\r\n...` + Push(Vec), + /// `%1\r\n...`, kept as the flat sequence it arrives in. + Map(Vec), +} + +impl Reply { + /// A one line form for a report and for comparing. + /// + /// This is the only place two replies are turned into text, so two replies + /// that print the same really are the same as far as anything downstream is + /// concerned. + pub fn render(&self) -> String { + match self { + Reply::Simple(b) => format!("+{}", show(b)), + Reply::Error(b) => format!("-{}", show(b)), + Reply::Int(b) => format!(":{}", show(b)), + Reply::Bulk(b) => format!("${}", show(b)), + Reply::Verbatim(b) => format!("={}", show(b)), + Reply::BulkError(b) => format!("!{}", show(b)), + Reply::Double(b) => format!(",{}", show(b)), + Reply::Bool(b) => format!("#{}", show(b)), + Reply::BigNumber(b) => format!("({}", show(b)), + Reply::Null => "_".to_string(), + Reply::NullBulk => "$-1".to_string(), + Reply::NullArray => "*-1".to_string(), + Reply::Array(v) => format!("*[{}]", join(v)), + Reply::Set(v) => format!("~[{}]", join(v)), + Reply::Push(v) => format!(">[{}]", join(v)), + Reply::Map(v) => format!("%[{}]", join(v)), + } + } + + /// The error message, if it is one. Used to tell "we said no differently" + /// from "we said yes when they said no", which are not the same bug. + pub fn error_word(&self) -> Option { + let bytes = match self { + Reply::Error(b) | Reply::BulkError(b) => b, + _ => return None, + }; + let text = String::from_utf8_lossy(bytes); + Some(text.split_whitespace().next().unwrap_or("").to_string()) + } +} + +fn join(v: &[Reply]) -> String { + v.iter().map(Reply::render).collect::>().join(", ") +} + +fn show(b: &[u8]) -> String { + String::from_utf8_lossy(b).into_owned() +} + +/// One connection to one server. +pub struct Conn { + out: TcpStream, + input: BufReader, +} + +impl Conn { + /// Open it, with timeouts, so a server that stops answering fails the run + /// rather than hanging it. + pub fn connect(port: u16) -> io::Result { + let stream = TcpStream::connect(("127.0.0.1", port))?; + stream.set_nodelay(true)?; + stream.set_read_timeout(Some(Duration::from_secs(10)))?; + stream.set_write_timeout(Some(Duration::from_secs(10)))?; + let input = BufReader::new(stream.try_clone()?); + Ok(Conn { out: stream, input }) + } + + /// Send one command as a RESP array of bulk strings. + /// + /// Always an array, never inline, because inline commands take a different + /// path through both servers and this is not the place to be testing that. + pub fn send(&mut self, args: &[String]) -> io::Result<()> { + let mut buf = Vec::with_capacity(64); + buf.extend_from_slice(format!("*{}\r\n", args.len()).as_bytes()); + for a in args { + buf.extend_from_slice(format!("${}\r\n", a.len()).as_bytes()); + buf.extend_from_slice(a.as_bytes()); + buf.extend_from_slice(b"\r\n"); + } + self.out.write_all(&buf)?; + self.out.flush() + } + + /// Read exactly one reply. + pub fn read(&mut self) -> io::Result { + let line = self.line()?; + let (tag, rest) = line + .split_first() + .ok_or_else(|| io::Error::other("empty reply"))?; + let rest = rest.to_vec(); + match tag { + b'+' => Ok(Reply::Simple(rest)), + b'-' => Ok(Reply::Error(rest)), + b':' => Ok(Reply::Int(rest)), + b',' => Ok(Reply::Double(rest)), + b'#' => Ok(Reply::Bool(rest)), + b'(' => Ok(Reply::BigNumber(rest)), + b'_' => Ok(Reply::Null), + b'$' | b'=' | b'!' => { + let n = number(&rest)?; + if n < 0 { + return Ok(Reply::NullBulk); + } + let body = self.exact(n as usize)?; + Ok(match tag { + b'$' => Reply::Bulk(body), + b'=' => Reply::Verbatim(body), + _ => Reply::BulkError(body), + }) + } + b'*' | b'~' | b'>' | b'%' => { + let n = number(&rest)?; + if n < 0 { + return Ok(Reply::NullArray); + } + let items = if *tag == b'%' { + n as usize * 2 + } else { + n as usize + }; + let mut v = Vec::with_capacity(items); + for _ in 0..items { + v.push(self.read()?); + } + Ok(match tag { + b'*' => Reply::Array(v), + b'~' => Reply::Set(v), + b'>' => Reply::Push(v), + _ => Reply::Map(v), + }) + } + other => Err(io::Error::other(format!( + "not a reply type: {:?}", + *other as char + ))), + } + } + + fn line(&mut self) -> io::Result> { + let mut buf = Vec::new(); + let n = self.input.read_until(b'\n', &mut buf)?; + if n == 0 { + return Err(io::Error::other("the server closed the connection")); + } + while buf.last() == Some(&b'\n') || buf.last() == Some(&b'\r') { + buf.pop(); + } + Ok(buf) + } + + fn exact(&mut self, n: usize) -> io::Result> { + use std::io::Read as _; + let mut body = vec![0u8; n + 2]; + self.input.read_exact(&mut body)?; + body.truncate(n); + Ok(body) + } +} + +fn number(b: &[u8]) -> io::Result { + String::from_utf8_lossy(b) + .trim() + .parse() + .map_err(|_| io::Error::other(format!("not a length: {:?}", show(b)))) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn the_two_spellings_of_nothing_stay_apart() { + assert_ne!(Reply::Null.render(), Reply::NullBulk.render()); + assert_ne!(Reply::NullBulk.render(), Reply::NullArray.render()); + } + + #[test] + fn a_bulk_and_a_verbatim_carrying_the_same_bytes_are_not_the_same_reply() { + let bulk = Reply::Bulk(b"txt:hello".to_vec()); + let verbatim = Reply::Verbatim(b"txt:hello".to_vec()); + assert_ne!(bulk.render(), verbatim.render()); + } + + #[test] + fn the_first_word_of_an_error_is_the_part_that_has_to_match() { + let e = Reply::Error(b"WRONGTYPE Operation against a key".to_vec()); + assert_eq!(e.error_word().as_deref(), Some("WRONGTYPE")); + assert_eq!(Reply::Simple(b"OK".to_vec()).error_word(), None); + } +} diff --git a/divergences.toml b/divergences.toml new file mode 100644 index 0000000..2efa9ce --- /dev/null +++ b/divergences.toml @@ -0,0 +1,20 @@ +# Differences from Redis we have decided to live with. +# +# Every entry needs a corpus file, a command prefix and a reason, and the reason +# is not optional: a file without one does not load. The point is that saying +# "we behave differently here" costs a diff somebody wrote and somebody else +# read, rather than a test quietly going green. +# +# An entry is not a place to park a bug. If the right answer is to fix it, fix +# it. What belongs here is a difference that is deliberate and will stay. +# +# [[divergence]] +# corpus = "strings.txt" +# command = "OBJECT ENCODING" +# reason = "we have one string encoding and no plans for a second" +# +# The command is matched as a prefix, so "SETRANGE" covers every SETRANGE in +# that file and "SETRANGE key 5" covers only the lines that start that way. + +# Nothing registered yet. Everything that differs today is either a bug or a +# command we have not written, and neither of those is a decision. diff --git a/suite/fetch.sh b/suite/fetch.sh new file mode 100755 index 0000000..a8065a9 --- /dev/null +++ b/suite/fetch.sh @@ -0,0 +1,60 @@ +#!/bin/sh +# Vendor the real Redis test suite at a pinned version. +# +# Not a rewrite of it, not a subset of it we typed out ourselves, and not a +# description of what we think it checks. It is Redis's own tests/ directory +# from the release tarball, dropped on disk unmodified, because the only +# compatibility claim worth making is the one their tests agree with. +# +# The version is pinned and the tree is thrown away and refetched, so nobody can +# quietly edit a test into passing. +# +# Usage: suite/fetch.sh [--force] + +set -eu + +HERE="$(cd "$(dirname "$0")/.." && pwd)" +REDIS_VERSION="${REDIS_VERSION:-8.10.1}" +DEST="$HERE/redis" + +FORCE=no +for arg in "$@"; do + case "$arg" in + --force) FORCE=yes ;; + *) echo "fetch: no such option: $arg" >&2; exit 2 ;; + esac +done + +if [ "$FORCE" = no ] && [ -f "$DEST/VERSION" ] && + [ "$(cat "$DEST/VERSION")" = "$REDIS_VERSION" ]; then + echo "redis $REDIS_VERSION tests already here" + exit 0 +fi + +tmp="$(mktemp -d)" +trap 'rm -rf "$tmp"' EXIT + +echo "fetching redis $REDIS_VERSION" +curl -fsSL --retry 3 \ + "https://github.com/redis/redis/archive/refs/tags/$REDIS_VERSION.tar.gz" \ + -o "$tmp/redis.tar.gz" + +# Only tests/ and the two scripts that drive it. The rest of the tarball is a +# C project we have no use for here, and vendoring it would put a second copy +# of Redis in a repository that is supposed to be about our differences from it. +tar -xzf "$tmp/redis.tar.gz" -C "$tmp" \ + "redis-$REDIS_VERSION/tests" \ + "redis-$REDIS_VERSION/runtest" \ + "redis-$REDIS_VERSION/src/redis-cli.c" 2>/dev/null || +tar -xzf "$tmp/redis.tar.gz" -C "$tmp" \ + "redis-$REDIS_VERSION/tests" \ + "redis-$REDIS_VERSION/runtest" + +rm -rf "$DEST" +mkdir -p "$DEST" +cp -R "$tmp/redis-$REDIS_VERSION/tests" "$DEST/tests" +cp "$tmp/redis-$REDIS_VERSION/runtest" "$DEST/runtest" +chmod +x "$DEST/runtest" +printf '%s\n' "$REDIS_VERSION" > "$DEST/VERSION" + +echo "redis $REDIS_VERSION tests are in $DEST" diff --git a/suite/passing.txt b/suite/passing.txt new file mode 100644 index 0000000..8074ca7 --- /dev/null +++ b/suite/passing.txt @@ -0,0 +1,10 @@ +# The Redis test files that pass against yodb today. +# +# One path per line, relative to the vendored tests/ directory and without the +# .tcl on the end, which is the form runtest --single wants. +# +# This list only grows by somebody running suite/runtest.sh --all, watching a +# file go green and adding it in a diff. It never grows by editing a test. +# +# It is empty because the suite has not been run against yodb yet. That is a +# real answer and it is the one that goes here until there is a better one. diff --git a/suite/runtest.sh b/suite/runtest.sh new file mode 100755 index 0000000..6cdbb55 --- /dev/null +++ b/suite/runtest.sh @@ -0,0 +1,76 @@ +#!/bin/sh +# Run Redis's own test files against our server. +# +# The suite normally starts the server it tests, from a Redis config file with +# Redis options in it. We are not Redis and do not read that file, so this uses +# the suite's external mode: we start yodb, and `runtest --host --port` points +# the tests at it. +# +# Every failure here is real. A test that fails because we have not implemented +# the command yet is still a compatibility failure, it is just one with a date +# on it. Nothing is patched out to make the number look better. The list of +# files that currently pass lives in suite/passing.txt, and moving a file out of +# that list is a decision somebody makes on purpose in a diff. +# +# Usage: suite/runtest.sh [--all] [file ...] +# with no arguments it runs the files in suite/passing.txt + +set -eu + +HERE="$(cd "$(dirname "$0")/.." && pwd)" +YODB="${YODB:-$HERE/../yo/target/release/yodb}" +PORT="${PORT:-7511}" +REDIS="$HERE/redis" + +[ -d "$REDIS/tests" ] || { echo "run suite/fetch.sh first" >&2; exit 2; } +[ -x "$YODB" ] || { echo "no yodb at $YODB. Set YODB." >&2; exit 2; } + +ALL=no +files="" +for arg in "$@"; do + case "$arg" in + --all) ALL=yes ;; + -*) echo "runtest: no such option: $arg" >&2; exit 2 ;; + *) files="$files $arg" ;; + esac +done + +if [ -z "$files" ]; then + if [ "$ALL" = yes ]; then + files="$(cd "$REDIS/tests" && find unit -name '*.tcl' | sed 's|\.tcl$||' | sort | tr '\n' ' ')" + else + files="$(grep -v '^#' "$HERE/suite/passing.txt" | grep -v '^$' | tr '\n' ' ')" + fi +fi + +"$YODB" serve --port "$PORT" >/tmp/yo-compat-server.log 2>&1 & +server=$! +trap 'kill $server 2>/dev/null || true' EXIT + +# Wait for it rather than sleeping a guess. +i=0 +while [ $i -lt 100 ]; do + if printf 'PING\r\n' | timeout 1 nc -q1 127.0.0.1 "$PORT" 2>/dev/null | grep -q PONG; then + break + fi + i=$((i + 1)) + sleep 0.05 +done +[ $i -lt 100 ] || { echo "yodb never answered on $PORT" >&2; exit 1; } + +single="" +for f in $files; do + single="$single --single $f" +done + +echo "yodb $("$YODB" --version) against redis $(cat "$REDIS/VERSION") tests" +echo + +# --singledb because the external server is one server and the suite otherwise +# expects to be able to move between numbered databases freely between files. +# --dont-clean leaves the logs where a failure can be read. +status=0 +# shellcheck disable=SC2086 +( cd "$REDIS" && ./runtest --host 127.0.0.1 --port "$PORT" --singledb --dont-clean $single ) || status=$? + +exit $status