fix(analysis): raise ValueError instead of NameError when FIO command…#87
Open
andrewwhitecdw wants to merge 2 commits into
Open
fix(analysis): raise ValueError instead of NameError when FIO command…#87andrewwhitecdw wants to merge 2 commits into
andrewwhitecdw wants to merge 2 commits into
Conversation
… missing
## Summary
When `bobber parse-results` processes a FIO log that does not contain at
least two `/usr/bin/fio --rw...` command lines, the tool is supposed to
raise a clear `ValueError`. Instead it crashes with
`NameError: name 'log' is not defined`, hiding the real problem from the
user.
## Root cause
In `fio_command_details()` (`bobber/lib/analysis/common.py`), the error
message references a variable `log` that does not exist in the function
scope (the function only receives `log_contents`, `old_reads`, and
`old_writes`):
```python
commands = re.findall(r'/usr/bin/fio --rw.*', log_contents)
if len(commands) < 2:
raise ValueError(f'FIO command not found in {log} file!')
```
Evaluating the f-string raises `NameError` before the `ValueError` can
be constructed, so callers see an unhelpful `NameError` traceback.
## Fix
Drop the undefined filename reference from the message (the function has
no access to the log filename):
```python
raise ValueError('FIO command not found in the log file!')
```
## Testing
- Reproduced with `uv run --with pyyaml python`:
- Before: `fio_command_details('no fio commands here', {}, {})` raised
`NameError: name 'log' is not defined`.
- After: raises `ValueError: FIO command not found in the log file!`
as intended.
- Sanity-checked the happy path: parsing a log containing read/write fio
commands still returns the expected parameter dicts.
- `pycodestyle bobber/lib/analysis/common.py` passes (matches the lint
step in `.github/workflows/python-package.yml`).
- The full CI suite (wheel build + `bobber build` Docker image build)
was not run locally as it requires Docker and GPU test infrastructure.
## Why existing tests missed it
The repository has no unit tests for the parsing library; CI only lints
and builds the package/Docker image, so this error path was never
exercised.
Flags the case where `fio_command_details()` receives log contents with fewer than two `/usr/bin/fio --rw...` command lines and crashed with `NameError: name 'log' is not defined` instead of the intended `ValueError`. Also sanity-checks that a valid read/write FIO log still parses. Red-green verification: - With fix (HEAD): `uv run --with pytest pytest tests/test_fio_command_nameerror.py -q` -> 2 passed. - Without fix (`git show HEAD~1:bobber/lib/analysis/common.py > bobber/lib/analysis/common.py`): same command -> 1 failed (NameError: name 'log' is not defined), 1 passed. - Restored with `git checkout -- .`.
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
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.
… missing
Summary
When
bobber parse-resultsprocesses a FIO log that does not contain at least two/usr/bin/fio --rw...command lines, the tool is supposed to raise a clearValueError. Instead it crashes withNameError: name 'log' is not defined, hiding the real problem from the user.Root cause
In
fio_command_details()(bobber/lib/analysis/common.py), the error message references a variablelogthat does not exist in the function scope (the function only receiveslog_contents,old_reads, andold_writes):Evaluating the f-string raises
NameErrorbefore theValueErrorcan be constructed, so callers see an unhelpfulNameErrortraceback.Fix
Drop the undefined filename reference from the message (the function has no access to the log filename):
Testing
uv run --with pyyaml python:fio_command_details('no fio commands here', {}, {})raisedNameError: name 'log' is not defined.ValueError: FIO command not found in the log file!as intended.pycodestyle bobber/lib/analysis/common.pypasses (matches the lint step in.github/workflows/python-package.yml).bobber buildDocker image build) was not run locally as it requires Docker and GPU test infrastructure.Why existing tests missed it
The repository has no unit tests for the parsing library; CI only lints and builds the package/Docker image, so this error path was never exercised.