fix(tests): Fix return values for stdlib benchmarks#1814
Conversation
pront
added
the
no-changelog
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a561e735b5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| // Checks if function returns correct result before starting benchmarking | ||
| let got = expression.resolve(&mut ctx).map_err(|e| e.to_string()); | ||
| assert_eq!(got, want); |
There was a problem hiding this comment.
With this unconditional assert_eq! in the benchmark harness, release cargo bench --features="default test" --bench stdlib -- --test now aborts before measuring later functions when a benchmark returns a floating value that differs by one f64 ulp from the literal expectation; I ran that command and it panicked at vrl_stdlib/functions/shannon_entropy/default with 3.7369879306358205 vs 3.736987930635821. The previous check was only a debug_assert_eq!, so exact equality here makes the stdlib benchmark suite fail in normal bench runs unless floating results are compared with tolerance or handled specially.
Useful? React with 👍 / 👎.
| " 0.042 upstream_addr=10.0.1.55:8080 upstream_status=200 request_id=req-abc123def456", | ||
| ); | ||
| const PARSE_REGEX_SINGLE_MATCH_PATTERN: &str = "(?P<number>.*?) group"; | ||
| const PARSE_REGEX_SINGLE_MATCH_PATTERN: &str = r"(?P<number>[^\s-]*?) group"; |
There was a problem hiding this comment.
This shared constant also feeds the parse_regex/single_match benchmark, while the documented example and unit test still use (?P<number>.*?) group in src/stdlib/parse_regex.rs. That old pattern already returns {"number":"first"} for the single-match case, so changing it here only alters the workload being measured (lazy wildcard vs. negated character class) and makes future parse_regex timings incomparable with previous runs; use a separate pattern for the parse_regex_all case if that benchmark needs different captures.
Useful? React with 👍 / 👎.
|
Please see https://github.com/vectordotdev/vrl/blob/main/AI_POLICY.md#ai-review-comments for un-resolved comments. |
pront
added
the
meta: awaiting author
2 participants
Summary
Many benchmarks for stdlib functions was wrong, because return value is checked just when doing benchmarking in debug mode.
Change Type
Is this a breaking change?
How did you test this PR?
Does this PR include user facing changes?
our guidelines.
Checklist
run
dd-rust-license-tool writeand commit the changes. More details here.