-
Notifications
You must be signed in to change notification settings - Fork 110
revamp testing infra #511
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
revamp testing infra #511
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
O wow, nice! Thanks!
This mainly gets us away from hard coding line numbers right?
yes! You instead add a magic comment to the line you expect to have an error. I should probably add some instructions for it in |
Please! Then I'll merge. This is awesome .... |
done! Also moved all the test files into a subdirectory. Idk if fully necessary, but makes |
DEVELOPMENT.md
Outdated
except* (ValueError,): # B013: 0, "ValueError", "*" | ||
... | ||
``` | ||
The error code should be in the `error_codes` dict, and the other values are passed to `eval` so should be valid python objects. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what the var1, var2, ... do, could you clarify?
the other values are passed to
eval
so should be valid python objects
The example uses "*"
which fails eval.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried to clarify it, is it better now?
The example uses "*" which fails eval.
well, eval('"*"')
is fully valid. But I tried making that more explicit as well.
Feel free to suggest any changes if it still doesn't make sense, the relevant logic is
flake8-bugbear/tests/test_bugbear.py
Lines 83 to 92 in f7d5123
# get text between `B\d\d\d:` and (end of line or another comment) | |
k = re.findall(r"(B\d\d\d):([^#]*)(?=#|$)", line) | |
for err_code, err_args in k: | |
# evaluate the arguments as if in a tuple | |
args = eval(f"({err_args},)") | |
assert args, "you must specify at least column" | |
col, *vars = args | |
assert isinstance(col, int), "column must be an int" | |
error_class = error_codes[err_code] | |
expected.append(error_class(lineno, col, vars=vars)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this is much better!
… not allow # error: ...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All LGTM - Docs make sense ... Let' start here.
Many thanks for this!
fixes #419 🚀
The general approach is copied from the test runner I wrote for https://github.com/python-trio/flake8-async, minus a bunch of bells and whistles that aren't necessary for an initial implementation.
I had to move the error classes to be defined inside a dict in order for
isort
to be happy, as otherwise we'd be importing a ton of objects that were never used other than indirectly througheval
. I also created anadd_error
helper method to simplify the code.Aside from those the changes to the code are minimal, the vast majority of the changes in the admittedly huge diff is simply moving the expected result to be inline in the test files.
The B950 tests were parametrized in a way that wasn't trivial to translate to this system, the column changes are due to line lengths being different with inlined error data.
Credits to Claude Code that parsed the test data and crafted sed commands to update the test files, but I kept a close eyes on all the changes so there should be zero impact on actual functionality.