flake8
-
rbtools/commands/tests/test_main.py (Diff revision 1) Show all issues -
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
Review Request #10856 — Created Jan. 24, 2020 and submitted
Information | |
---|---|
kpatenio | |
RBTools | |
release-2.0.x | |
392c050... | |
Reviewers | |
rbtools | |
On python 2, running
rbt --help <command>
andrbt <command> --help
are equivalent. However, runningrbt <command> --help
does not work
on python 3. This happens because byte strings are utilized in the
code to see if--help
or-h
are passed in as arguments. They are
never found because the code is trying to findb'--help
' orb'-h'
.
In python 2,b'--help'
is equivalent to'--help'
, but in python 3,
this is not the case.To fix this issue, the byte strings
b'--help'
andb’-h’
have been
changed to python 3 compatible strings (i.e.--help
and-h
). Unit
tests have also been added to ensure that all possible positions and
cases of the 'help' argument work for rbtools.(ex.rbt —help <command>
,
rbt <command> -h
,rbt help <command>
, etc.).All tests were run using both python 2.7.17 and python 3.7.6.
Test cases run on python 2.7.17 and 3.7.6:
Test case whererbt help <command>
is run
Test case whererbt <command> --help
orrbt <command> -h
is run
Test case whererbt --help <command>
orrbt -h <command>
is run
Test case where an invalid rbt command is run withhelp
Test case where multiple arguments are entered
Description | From | Last Updated |
---|---|---|
Can you also detail which versions of Python all your tests have been run under? We need to ensure full … |
|
|
Can you wrap the change description at 79 columns? It would also be nice to use backticks whenever you're showing … |
|
|
E501 line too long (91 > 79 characters) |
![]() |
|
E501 line too long (91 > 79 characters) |
![]() |
|
E501 line too long (91 > 79 characters) |
![]() |
|
E501 line too long (94 > 79 characters) |
![]() |
|
E501 line too long (80 > 79 characters) |
![]() |
|
E501 line too long (92 > 79 characters) |
![]() |
|
E501 line too long (92 > 79 characters) |
![]() |
|
E501 line too long (92 > 79 characters) |
![]() |
|
E501 line too long (82 > 79 characters) |
![]() |
|
E501 line too long (88 > 79 characters) |
![]() |
|
E501 line too long (80 > 79 characters) |
![]() |
|
E501 line too long (92 > 79 characters) |
![]() |
|
E501 line too long (92 > 79 characters) |
![]() |
|
E501 line too long (92 > 79 characters) |
![]() |
|
E501 line too long (82 > 79 characters) |
![]() |
|
E501 line too long (88 > 79 characters) |
![]() |
|
E501 line too long (80 > 79 characters) |
![]() |
|
E501 line too long (82 > 79 characters) |
![]() |
|
I think this docstring and file could be named in a more descriptive way eg. Tests for RBTools help command … |
|
|
Similar issue about naming here - I think something like HelpCommandTests would be better |
|
|
I think this could be better named as test_help_command. Ordering it that way would make it more consistent with the … |
|
|
Maybe this could be pulled out into a function since you use it a few times |
|
|
This snippet feels like it could also be a function since you use it a few times. Also, maybe command_string … |
|
|
Same as above |
|
|
Minor but maybe naming test_help to be test_command would be better here to be more consistent with your first test … |
|
|
Same as above |
|
|
Same as above |
|
|
Same as above |
|
|
Since you're testing help already, it might be a good idea to add tests for when there are more than … |
|
|
W293 blank line contains whitespace |
![]() |
|
W293 blank line contains whitespace |
![]() |
|
W293 blank line contains whitespace |
![]() |
|
W293 blank line contains whitespace |
![]() |
|
W391 blank line at end of file |
![]() |
|
This is wrapping way too early. Can you make sure it wraps at 79 characters? Same with docstrings below. |
|
|
I think you can go ahead and just use these strings inline. It results in less code, and is easier … |
|
|
Blank line between blocks/statements. Same below. |
|
|
Docstring summaries are limited to one line. Same below. |
|
|
The string type is going to be required. Should be unicode. Same below. |
|
|
"Returns" blocks are in the form of: Returns: type: description So probably unicode for the type. |
|
|
We use %-formatting, instead of .format(). It's technically faster. |
|
|
Blank line between each argument. |
|
|
bool |
|
|
bool |
|
|
W291 trailing whitespace |
![]() |
|
W291 trailing whitespace |
![]() |
|
W291 trailing whitespace |
![]() |
|
W291 trailing whitespace |
![]() |
|
I've been thinking about these code blocks, which are almost universal in your code. I think what would be better … |
|
|
How about making this one statement? Same below. |
|
|
This could be one line now. Same with ones below. |
|
|
Sorry, I wasn't clear in my description above. We'd want a blank line when separating blocks on the same indentation … |
|
|
This can be one statement. |
|
|
Here, and below, it'll be a lot easier to debug if we're passing in the command directly to _check_help_output. The … |
|
|
This should be "Check". We use a form of "Do this, do that" rather than "Does this, does that." in … |
|
|
The type should be (bool, optional), since it's not required. |
|
|
True should be shown as a literal, using double-backticks. |
|
|
Functions that don't return a value shouldn't have a Returns section. |
|
|
This seems fragile. I'm not wild about it. The idea is to have a version of command that didn't have … |
|
|
Rather than defining the variable, you can move the assertIn into those if statements. |
|
|
This is only here for the execute command, right? In this case, let's only wrap as much as we need … |
|
rbtools/commands/tests/test_main.py (Diff revision 1) |
---|
Fixed linting issues
Testing Done: |
|
||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
||||||||||||||||||||||||||||||
Diff: |
Revision 2 (+107 -1) |
rbtools/commands/tests/test_main.py (Diff revision 2) |
---|
I think this docstring and file could be named in a more descriptive way eg.
Tests for RBTools help command
andtest_help.py
. That way it's also consistent with the other test file I see in that directory (test_post.py
).
rbtools/commands/tests/test_main.py (Diff revision 2) |
---|
Similar issue about naming here - I think something like
HelpCommandTests
would be better
rbtools/commands/tests/test_main.py (Diff revision 2) |
---|
I think this could be better named as
test_help_command
. Ordering it that way would make it more consistent with the couple of other tests with a similar pattern in the name intest_post.py
rbtools/commands/tests/test_main.py (Diff revision 2) |
---|
Maybe this could be pulled out into a function since you use it a few times
rbtools/commands/tests/test_main.py (Diff revision 2) |
---|
This snippet feels like it could also be a function since you use it a few times.
Also, maybe
command_string
andtest
could be renamed to something more descriptive, such asexpected_command_string
andtest_result
- it wasn't very clear to me thattest
contained the output of executing the commands.
rbtools/commands/tests/test_main.py (Diff revision 2) |
---|
Minor but maybe naming
test_help
to betest_command
would be better here to be more consistent with your first test where you referred to a single command astest_command
or changing the first one totest_help
(but imotest_command
is easier to understand)
rbtools/commands/tests/test_main.py (Diff revision 2) |
---|
Since you're testing
help
already, it might be a good idea to add tests for when there are more than than three arguments eg.rbt help alias abcd
and also less than eg.rbt help
Addressed review changes and added a new test case
Testing Done: |
|
|||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
|||||||||||||||||||||
Diff: |
Revision 3 (+194 -1) |
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 4 (+189 -1) |
First pass, addressing some of the syntax and standards stuff.
Can you also detail which versions of Python all your tests have been run under? We need to ensure full coverage.
Can you wrap the change description at 79 columns?
It would also be nice to use backticks whenever you're showing commands, arguments, or Python code, to help readability.
rbtools/commands/tests/test_main.py (Diff revision 4) |
---|
This is wrapping way too early. Can you make sure it wraps at 79 characters?
Same with docstrings below.
rbtools/commands/tests/test_main.py (Diff revision 4) |
---|
I think you can go ahead and just use these strings inline. It results in less code, and is easier to read through and understand.
Same with tests below.
rbtools/commands/tests/test_main.py (Diff revision 4) |
---|
Blank line between blocks/statements.
Same below.
rbtools/commands/tests/test_main.py (Diff revision 4) |
---|
Docstring summaries are limited to one line.
Same below.
rbtools/commands/tests/test_main.py (Diff revision 4) |
---|
The string type is going to be required. Should be
unicode
.Same below.
rbtools/commands/tests/test_main.py (Diff revision 4) |
---|
"Returns" blocks are in the form of:
Returns: type: description
So probably
unicode
for the type.
rbtools/commands/tests/test_main.py (Diff revision 4) |
---|
We use
%
-formatting, instead of.format()
. It's technically faster.
Addressed syntax and styling reviews
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Commit: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 5 (+185 -1) |
Removed trailing whitespaces
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 6 (+185 -1) |
Replaced variables 'invalid_rbt_command' with strings 'invalid'
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 7 (+184 -1) |
rbtools/commands/tests/test_main.py (Diff revision 7) |
---|
I've been thinking about these code blocks, which are almost universal in your code. I think what would be better would be to have a single command that takes care of executing these tests. Something like a
_check_help_output()
method, which takes the command to run and theinvalid
parameter.That would take care of the
try/execute/except
block, would take care of creating the fail message (you could just roll that logic up into this method), and the assertion.The assertion itself can then become an
assertIn
, rather than anassertTrue(... in ...)
, which is what it basically is today. That allows us to leverage the standard behavior in unit tests for checking for a needle in a haystack, taking advantage of the enhanced failure information that provides.This would simplify the tests greatly. Each test for a command would basically be a call. You could remove the
for
loops and just call this with each variant (which really helps when debugging test failures -- it's harder when loops are involved).
rbtools/commands/tests/test_main.py (Diff revision 7) |
---|
How about making this one statement?
Same below.
rbtools/commands/tests/test_main.py (Diff revision 7) |
---|
This could be one line now.
Same with ones below.
rbtools/commands/tests/test_main.py (Diff revision 7) |
---|
Sorry, I wasn't clear in my description above. We'd want a blank line when separating blocks on the same indentation level, but you wouldn't have one between the start of a block and that block's first statement.
This is fine:
test_rbt_commands = [ ... ] for test_command in test_rbt_commands: try: ...
Refactored code to be simplified
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 8 (+106 -1) |
rbtools/commands/tests/test_main.py (Diff revision 8) |
---|
Here, and below, it'll be a lot easier to debug if we're passing in the command directly to
_check_help_output
. The list doesn't benefit us in any way, since we unrolled the loops, and can actually lead to future copy/paste bugs (if we skip an index or miss an item in the list).
rbtools/commands/tests/test_main.py (Diff revision 8) |
---|
This should be "Check". We use a form of "Do this, do that" rather than "Does this, does that." in the summary.
rbtools/commands/tests/test_main.py (Diff revision 8) |
---|
The type should be
(bool, optional)
, since it's not required.
rbtools/commands/tests/test_main.py (Diff revision 8) |
---|
True
should be shown as a literal, using double-backticks.
rbtools/commands/tests/test_main.py (Diff revision 8) |
---|
Functions that don't return a value shouldn't have a
Returns
section.
rbtools/commands/tests/test_main.py (Diff revision 8) |
---|
This seems fragile. I'm not wild about it. The idea is to have a version of
command
that didn't have a--help
variant in it, right?It's best when making a unit test helper function to not have to guess too much about the caller's intention. I think this should simply take another parameter specifying the RBTools subcommand name.
subcommand_name
should be fine. You can then just pass that into the output strings, and avoid the guessing here.
rbtools/commands/tests/test_main.py (Diff revision 8) |
---|
Rather than defining the variable, you can move the
assertIn
into thoseif
statements.
rbtools/commands/tests/test_main.py (Diff revision 8) |
---|
This is only here for the
execute
command, right? In this case, let's only wrap as much as we need to in thetry/except
. We should only require theexecute
to be wrapped.
More refactoring
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 9 (+80 -1) |