• 
      

    Add unit tests for integration config form

    Review Request #10830 — Created Jan. 17, 2020 and submitted

    Information

    Djblets
    release-1.0.x
    6096d6a...

    Reviewers

    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 …

    chipx86chipx86

    Please add the bug number to the "Bugs" field on the review request.

    daviddavid

    Please move the last part of your description (about tests passing) to the "Testing Done" field. That also needs a …

    daviddavid

    The lines in the description should wrap at 79 columns. Currently the first is 87 columns. As a tip, you'll …

    chipx86chipx86

    There's a particular order that imports need to be grouped in (blank lines separating them): Python-provided modules Third-party modules (from …

    chipx86chipx86

    Mind naming this request_factory? We try to avoid abbreviations, since it's harder to search for them, and isn't self-descriptive.

    chipx86chipx86

    Let's rename this to just form. class_under_test isn't actually very self-describing.

    chipx86chipx86

    This will be a lot easier to read and maintain (and fit under 79 characters) as: self.form = IntegrationConfigForm( integration=self.integration, …

    chipx86chipx86

    E501 line too long (100 > 79 characters)

    reviewbotreviewbot

    E501 line too long (106 > 79 characters)

    reviewbotreviewbot

    These docstrings are shown when running the test suite, and since there's usually so many (thousands in Review Board!), it's …

    chipx86chipx86

    Rather than testing whether this returns a value, how about testing that it's the exact value we're expecting?

    chipx86chipx86

    E501 line too long (97 > 79 characters)

    reviewbotreviewbot

    It's confusing that there's two name for this... We try to stick with the singular form, assertEqual. (We don't always …

    chipx86chipx86

    E501 line too long (109 > 79 characters)

    reviewbotreviewbot

    E501 line too long (97 > 79 characters)

    reviewbotreviewbot

    E501 line too long (90 > 79 characters)

    reviewbotreviewbot

    E501 line too long (112 > 79 characters)

    reviewbotreviewbot

    We never want to use the u prefix on any strings. We get those automatically from the from __future__ import …

    chipx86chipx86

    Please add a class docstring for this (can be the same as your module docstring).

    daviddavid

    This line should have a comma at the end.

    daviddavid

    Missing a period at the end of this.

    chipx86chipx86

    This is missing a period at the end.

    chipx86chipx86

    Super minor nit, but it's common to have a blank line after the super() call. Helps to think of code …

    chipx86chipx86

    I think the docstrings here aren't really descriptive enough. I'm having trouble understanding from them what we're testing. The first …

    chipx86chipx86

    Oh, just realized. You'll want assertIsInstance here. It's a nice alternative, because if the test fails, it'll provide more useful …

    chipx86chipx86

    Looks like this is wrapping too early. It should be closer to 79 characters. Same with the ones below.

    chipx86chipx86

    E501 line too long (89 > 79 characters)

    reviewbotreviewbot

    E501 line too long (86 > 79 characters)

    reviewbotreviewbot
    Checks run (1 failed, 1 succeeded)
    flake8 failed.
    JSHint passed.

    flake8

    chipx86
    1. 
        
    2. Show all issues

      Can you add a docstring at the top of the file? Just a one-line summary. Unit tests for <full class path>. is good.

    3. djblets/integrations/tests/test_forms.py (Diff revision 1)
       
       
       
       
       
       
       
      Show all issues

      There's a particular order that imports need to be grouped in (blank lines separating them):

      1. Python-provided modules
      2. Third-party modules (from the perspective of the project you're working with)
      3. 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 the djblets ones. Alphabetical.

    4. Show all issues

      Mind naming this request_factory? We try to avoid abbreviations, since it's harder to search for them, and isn't self-descriptive.

    5. Show all issues

      Let's rename this to just form. class_under_test isn't actually very self-describing.

    6. djblets/integrations/tests/test_forms.py (Diff revision 1)
       
       
       
       
      Show all issues

      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.

    7. Show all issues

      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.

    8. djblets/integrations/tests/test_forms.py (Diff revision 1)
       
       
       
      Show all issues

      Rather than testing whether this returns a value, how about testing that it's the exact value we're expecting?

    9. Show all issues

      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.)

    10. Show all issues

      We never want to use the u prefix on any strings. We get those automatically from the from __future__ import unicode_literals at the top on Python 2.x, and it's the default for 3.x.

    11. 
        
    bui1
    david
    1. Just a few style-related comments:

    2. Show all issues

      Please add the bug number to the "Bugs" field on the review request.

    3. Show all issues

      Please move the last part of your description (about tests passing) to the "Testing Done" field. That also needs a period.

    4. Show all issues

      Please add a class docstring for this (can be the same as your module docstring).

    5. Show all issues

      This line should have a comma at the end.

    6. 
        
    bui1
    bui1
    chipx86
    1. This is coming along nicely! I have a few suggestions on docstrings, to help make them more self-descriptive.

    2. Show all issues

      Missing a period at the end of this.

    3. Show all issues

      This is missing a period at the end.

    4. djblets/integrations/tests/test_forms.py (Diff revision 3)
       
       
       
      Show all issues

      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."

    5. djblets/integrations/tests/test_forms.py (Diff revision 3)
       
       
       
       
       
       
       
       
       
       
       
       
       
       
      Show all issues

      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"?

    6. 
        
    bui1
    mike_conley
    1. This looks good to me. Thanks, Monica!

    2. 
        
    chipx86
    1. Juuuust about there!

    2. Show all issues

      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 your rbt post -u.

    3. Show all issues

      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.

    4. djblets/integrations/tests/test_forms.py (Diff revision 4)
       
       
       
      Show all issues

      Looks like this is wrapping too early. It should be closer to 79 characters.

      Same with the ones below.

      1. I've made the changes but flake8 suggests that having the entire docstring on one line is too long so i'll make the change to wrap it later

    5. 
        
    bui1
    Review request changed
    Description:
    ~  

    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 with the form.

      ~

    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.

    Commit:
    e5e2792b3dcb7e4f27853327128335b7c51a9fd5
    ab3079815c30d07e4c2da234e95fcb6d60e26bf2

    Checks run (1 failed, 1 succeeded)

    flake8 failed.
    JSHint passed.

    flake8

    bui1
    david
    1. Ship It!
    2. 
        
    bui1
    Review request changed
    Status:
    Completed
    Change Summary:
    Pushed to release-2.0.x (ebb4fe6)