flake8
-
djblets/integrations/tests/test_forms.py (Diff revision 1) Show all issues -
-
-
-
-
-
Review Request #10830 — Created Jan. 17, 2020 and submitted
Information | |
---|---|
bui1 | |
Djblets | |
release-1.0.x | |
4589 | |
6096d6a... | |
Reviewers | |
djblets | |
Add test scenarios that include checking if
the integration config form saves properly,
and gets/sets key values for the enabled and
name form fields included in the form.All unit tests pass including with the new ones
All unit tests pass including with the new ones.
Description | From | Last Updated |
---|---|---|
Can you add a docstring at the top of the file? Just a one-line summary. Unit tests for <full class … |
|
|
Please add the bug number to the "Bugs" field on the review request. |
|
|
Please move the last part of your description (about tests passing) to the "Testing Done" field. That also needs a … |
|
|
The lines in the description should wrap at 79 columns. Currently the first is 87 columns. As a tip, you'll … |
|
|
There's a particular order that imports need to be grouped in (blank lines separating them): Python-provided modules Third-party modules (from … |
|
|
Mind naming this request_factory? We try to avoid abbreviations, since it's harder to search for them, and isn't self-descriptive. |
|
|
Let's rename this to just form. class_under_test isn't actually very self-describing. |
|
|
This will be a lot easier to read and maintain (and fit under 79 characters) as: self.form = IntegrationConfigForm( integration=self.integration, … |
|
|
E501 line too long (100 > 79 characters) |
![]() |
|
E501 line too long (106 > 79 characters) |
![]() |
|
These docstrings are shown when running the test suite, and since there's usually so many (thousands in Review Board!), it's … |
|
|
Rather than testing whether this returns a value, how about testing that it's the exact value we're expecting? |
|
|
E501 line too long (97 > 79 characters) |
![]() |
|
It's confusing that there's two name for this... We try to stick with the singular form, assertEqual. (We don't always … |
|
|
E501 line too long (109 > 79 characters) |
![]() |
|
E501 line too long (97 > 79 characters) |
![]() |
|
E501 line too long (90 > 79 characters) |
![]() |
|
E501 line too long (112 > 79 characters) |
![]() |
|
We never want to use the u prefix on any strings. We get those automatically from the from __future__ import … |
|
|
Please add a class docstring for this (can be the same as your module docstring). |
|
|
This line should have a comma at the end. |
|
|
Missing a period at the end of this. |
|
|
This is missing a period at the end. |
|
|
Super minor nit, but it's common to have a blank line after the super() call. Helps to think of code … |
|
|
I think the docstrings here aren't really descriptive enough. I'm having trouble understanding from them what we're testing. The first … |
|
|
Oh, just realized. You'll want assertIsInstance here. It's a nice alternative, because if the test fails, it'll provide more useful … |
|
|
Looks like this is wrapping too early. It should be closer to 79 characters. Same with the ones below. |
|
|
E501 line too long (89 > 79 characters) |
![]() |
|
E501 line too long (86 > 79 characters) |
![]() |
djblets/integrations/tests/test_forms.py (Diff revision 1) |
---|
Can you add a docstring at the top of the file? Just a one-line summary.
Unit tests for <full class path>.
is good.
djblets/integrations/tests/test_forms.py (Diff revision 1) |
---|
There's a particular order that imports need to be grouped in (blank lines separating them):
- Python-provided modules
- Third-party modules (from the perspective of the project you're working with)
- Project's own modules
And these must all be in alphabetical order. Skip any groups that don't have modules.
So in your case, the
django
ones would be in its own group at the top, then a blank line, then thedjblets
ones. Alphabetical.
djblets/integrations/tests/test_forms.py (Diff revision 1) |
---|
Mind naming this
request_factory
? We try to avoid abbreviations, since it's harder to search for them, and isn't self-descriptive.
djblets/integrations/tests/test_forms.py (Diff revision 1) |
---|
Let's rename this to just
form
.class_under_test
isn't actually very self-describing.
djblets/integrations/tests/test_forms.py (Diff revision 1) |
---|
This will be a lot easier to read and maintain (and fit under 79 characters) as:
self.form = IntegrationConfigForm( integration=self.integration, request=self.request, instance=None, data={ 'enabled': True, 'name': self.mock_name, })
Note also the keyword arguments. Being explicit here is really good when there's a lot of arguments going on.
Also also, note the
self.mock_name
. No need to reference the class name directly.I think we're only using that name once, so we can probably even nuke the class attribute.
djblets/integrations/tests/test_forms.py (Diff revision 1) |
---|
These docstrings are shown when running the test suite, and since there's usually so many (thousands in Review Board!), it's very important that they're explicit. They should always include the thing being tested. So you'd probably want something like:
"""Testing IntegrationConfigForm.config returns the configuration instance"""
Or
IntegrationConfigForm.get_key_value()
below, etc.
djblets/integrations/tests/test_forms.py (Diff revision 1) |
---|
Rather than testing whether this returns a value, how about testing that it's the exact value we're expecting?
djblets/integrations/tests/test_forms.py (Diff revision 1) |
---|
It's confusing that there's two name for this... We try to stick with the singular form,
assertEqual
. (We don't always get it right -- there's a few stragglers in the codebase.)
djblets/integrations/tests/test_forms.py (Diff revision 1) |
---|
We never want to use the
u
prefix on any strings. We get those automatically from thefrom __future__ import unicode_literals
at the top on Python 2.x, and it's the default for 3.x.
Summary: |
|
||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
||||||||||||||||||
Commit: |
|
||||||||||||||||||
Diff: |
Revision 2 (+65) |
Just a few style-related comments:
Please move the last part of your description (about tests passing) to the "Testing Done" field. That also needs a period.
djblets/integrations/tests/test_forms.py (Diff revision 2) |
---|
Please add a class docstring for this (can be the same as your module docstring).
djblets/integrations/tests/test_forms.py (Diff revision 2) |
---|
This line should have a comma at the end.
Description: |
|
||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
||||||||||||
Bugs: |
|
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 3 (+67) |
This is coming along nicely! I have a few suggestions on docstrings, to help make them more self-descriptive.
djblets/integrations/tests/test_forms.py (Diff revision 3) |
---|
Super minor nit, but it's common to have a blank line after the
super()
call. Helps to think of code like paragraphs. One paragraph is "We're calling the parent method and setting up all of its state." Next paragraph is "We're setting up the basic objects needed to work with integrations." Paragraph after is "We're setting up state for the form."
djblets/integrations/tests/test_forms.py (Diff revision 3) |
---|
I think the docstrings here aren't really descriptive enough. I'm having trouble understanding from them what we're testing.
The first test says it's testing a default field, but the first thing we check is actually a value provided to the form (form data filled in by a user and POSTed to the page). I think this is really testing that "get_key_value() returns values from form data"
The second, I don't think "default field" makes any sense in the context. I think it's more that "set_key_value() overrides form data"?
The third is more correct but maybe vague? How about "set_key_value() sets custom non-field data"?
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 4 (+74) |
Juuuust about there!
The lines in the description should wrap at 79 columns. Currently the first is 87 columns.
As a tip, you'll usually get a column limit when composing a Git commit in your editor, helping to stay under this limit. If you modify the commit after the review request is posted, you can update the information on the review request from the commit by passing
-g yes
when running yourrbt post -u
.
djblets/integrations/tests/test_forms.py (Diff revision 4) |
---|
Oh, just realized. You'll want
assertIsInstance
here. It's a nice alternative, because if the test fails, it'll provide more useful information about the types.
djblets/integrations/tests/test_forms.py (Diff revision 4) |
---|
Looks like this is wrapping too early. It should be closer to 79 characters.
Same with the ones below.
Description: |
|
||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
||||||||||||||||||
Diff: |
Revision 5 (+68) |
Description: |
|
|||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
|||||||||||||||||||||||||||
Diff: |
Revision 6 (+71) |