Skip to content

fix(failed_tests_file): correctly iterate testsFailed dict#251

Merged
kei-nan merged 2 commits into
RedisLabsModules:masterfrom
kei-nan:fix-failed-tests-file-iter
May 29, 2026
Merged

fix(failed_tests_file): correctly iterate testsFailed dict#251
kei-nan merged 2 commits into
RedisLabsModules:masterfrom
kei-nan:fix-failed-tests-file-iter

Conversation

@kei-nan
Copy link
Copy Markdown
Contributor

@kei-nan kei-nan commented May 28, 2026

Bug

self.testsFailed is a dict of {test_name: [failures]} (built via setdefault(name, []).extend(...) in addFailureToTest). The --failed-tests-file writer in RLTest/__main__.py iterates it as:

for test, _ in self.testsFailed:
    file.write(test.split(' ')[0] + "\n")

Iterating a dict yields keys (strings). Python then tries to unpack each key into (test, _) — which fails for any test name longer than two characters:

File "…/RLTest/__main__.py", line 1070, in execute
    for test, _ in self.testsFailed:
        ^^^^^^^
ValueError: too many values to unpack (expected 2)

This silently breaks the --failed-tests-file (-F) feature whenever a run actually has failures: the writer aborts before producing the file, RLTest exits with the unhandled exception, and any downstream tooling that depends on the file sees no output.

Repro

python -m RLTest --module path/to/module.so --failed-tests-file /tmp/failed.txt --test some_failing_test.py

Crashes with the traceback above; /tmp/failed.txt is empty or missing.

Fix

Iterate keys directly. The value side wasn't used (just _), so this is the smallest correct change:

for test in self.testsFailed:
    file.write(test.split(' ')[0] + "\n")

Why this hasn't been caught

The buggy code only runs when self.testsFailed is non-empty AND --failed-tests-file is set. Most CI pipelines that use -F only see green runs, so the path stays dormant. Surfaced from the RediSearch flaky-test DB work (RediSearch/RediSearch#9869) when failures finally hit the writer.

Test plan

  • Run a known-failing test with --failed-tests-file=/tmp/f.txt → confirm /tmp/f.txt lists the failed test names instead of crashing
  • Run a passing test with the same flag → confirm an empty file is created (matches existing behavior of the else branch)

`self.testsFailed` is a dict of `{test_name: [failures]}` (built via
`setdefault(name, []).extend(...)`). The `--failed-tests-file` writer
was iterating it as `for test, _ in self.testsFailed:`, which yields
dict keys (strings) and tries to unpack each key into two elements —
crashing with `ValueError: too many values to unpack (expected 2)` on
any test name longer than two characters.

This silently broke the failed-tests-file feature whenever a real run
had failures: the writer aborted before producing the file, so any
downstream tooling that depends on it saw no output.

Fix by iterating keys directly, since the value side wasn't used.
@kei-nan kei-nan self-assigned this May 28, 2026
@kei-nan kei-nan requested review from GuyAv46 and JoanFM May 28, 2026 20:02
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 28, 2026

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 34.83%. Comparing base (02cfbb9) to head (5806e03).
⚠️ Report is 12 commits behind head on master.

Files with missing lines Patch % Lines
RLTest/__main__.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #251      +/-   ##
==========================================
+ Coverage   32.46%   34.83%   +2.37%     
==========================================
  Files          17       18       +1     
  Lines        2597     2733     +136     
==========================================
+ Hits          843      952     +109     
- Misses       1754     1781      +27     
Flag Coverage Δ
unittests 34.83% <0.00%> (+2.37%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

JoanFM
JoanFM previously requested changes May 28, 2026
Comment thread RLTest/__main__.py Outdated
Per @JoanFM, iterating the dict directly works but is implicit. Using
.keys() makes it obvious to the reader that only the keys are needed.
@kei-nan kei-nan requested a review from JoanFM May 28, 2026 20:40
@kei-nan kei-nan dismissed JoanFM’s stale review May 29, 2026 12:38

Addressed comment - got approval from Guy
Would like to merge this as soon as possible.

@kei-nan kei-nan merged commit cd8c81e into RedisLabsModules:master May 29, 2026
9 checks passed
kei-nan added a commit to RediSearch/RediSearch that referenced this pull request May 29, 2026
RLTest 0.7.27 includes the fix for the FAILEDFILE writer crashing on
runs with actual failures (RedisLabsModules/RLTest#251). Without this,
the flaky-DB record step sees no .failed_*.txt and reports
'No results to record' on any failing CI run.

Switched the exact pin to a >=0.7.27,<0.8.0 range so future RLTest
patch fixes are picked up via 'uv lock --upgrade-package rltest'
without another pyproject.toml change.
kei-nan added a commit to RediSearch/RediSearch that referenced this pull request May 29, 2026
RLTest 0.7.27 includes the fix for the FAILEDFILE writer crashing on
runs with actual failures (RedisLabsModules/RLTest#251). Without this,
the flaky-DB record step sees no .failed_*.txt and reports
'No results to record' on any failing CI run.

Also bumps the lower bound from 0.7.18 to 0.7.27 so the explicit fix
requirement is captured in pyproject.toml, not just uv.lock.

psutil 5.9.8 -> 7.2.2 is a transitive dep bump that came with the
RLTest 0.7.27 resolve.
kei-nan added a commit to RediSearch/RediSearch that referenced this pull request May 29, 2026
RLTest 0.7.27 includes the fix for the FAILEDFILE writer crashing on
runs with actual failures (RedisLabsModules/RLTest#251). Without this,
the flaky-DB record step sees no .failed_*.txt and reports
'No results to record' on any failing CI run.

Also bumps the lower bound from 0.7.18 to 0.7.27 so the explicit fix
requirement is captured in pyproject.toml, not just uv.lock.
kei-nan added a commit to RediSearch/RediSearch that referenced this pull request May 29, 2026
RLTest 0.7.27 includes the fix for the FAILEDFILE writer crashing on
runs with actual failures (RedisLabsModules/RLTest#251). Without this,
the flaky-DB record step sees no .failed_*.txt and reports
'No results to record' on any failing CI run.

Also bumps the lower bound from 0.7.18 to 0.7.27 so the explicit fix
requirement is captured in pyproject.toml, not just uv.lock.
kei-nan added a commit to RediSearch/RediSearch that referenced this pull request May 29, 2026
RLTest 0.7.27 includes the fix for the FAILEDFILE writer crashing on
runs with actual failures (RedisLabsModules/RLTest#251). Without this,
the flaky-DB record step sees no .failed_*.txt and reports
'No results to record' on any failing CI run.

Replaces the previous 'RLTest<=0.7.16' upper-bound pin with a
>=0.7.27,<0.8.0 range so future RLTest patch fixes are picked up via
'uv lock --upgrade-package rltest' without another pyproject.toml change.
kei-nan added a commit to RediSearch/RediSearch that referenced this pull request May 29, 2026
RLTest 0.7.27 includes the fix for the FAILEDFILE writer crashing on
runs with actual failures (RedisLabsModules/RLTest#251). Without this,
the flaky-DB record step sees no .failed_*.txt and reports
'No results to record' on any failing CI run.

Also bumps the lower bound from 0.7.18 to 0.7.27 so the explicit fix
requirement is captured in pyproject.toml, not just uv.lock.
kei-nan added a commit to RediSearch/RediSearch that referenced this pull request May 29, 2026
RLTest 0.7.27 includes the fix for the FAILEDFILE writer crashing on
runs with actual failures (RedisLabsModules/RLTest#251). Without this,
the flaky-DB record step sees no .failed_*.txt and reports
'No results to record' on any failing CI run.

Also bumps the lower bound from 0.7.18 to 0.7.27 so the explicit fix
requirement is captured in pyproject.toml, not just uv.lock.
github-merge-queue Bot pushed a commit to RediSearch/RediSearch that referenced this pull request May 31, 2026
RLTest 0.7.27 includes the fix for the FAILEDFILE writer crashing on
runs with actual failures (RedisLabsModules/RLTest#251). Without this,
the flaky-DB record step added in #9869 sees no .failed_*.txt and
reports 'No results to record' on any failing CI run.

The bug has been dormant on master because recent CI runs have been
green; this lands the fix before the next failure hits the writer.

Switched the exact pin to a >=0.7.27,<0.8.0 range so future RLTest
patch fixes are picked up via 'uv lock --upgrade-package rltest'
without another pyproject.toml change.
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.

4 participants