Add testing framework for ReviewBot tools

Review Request #10288 — Created Oct. 27, 2018 and discarded

Information

ReviewBot
master

Reviewers

Previously, we did not have tests for tools in Review Bot.

This change adds a framework for testing the output parsing logic of
the Review Bot tool classes. Additionally, some sample unit tests have
been added to demonstrate how I envision this framework being used.

Successfuly ran the unit tests (./tests/runtests.py).

Description From Last Updated

Description should be wrapped at 72 characters?

ilawilaw

Double quotes are used in some places where it should be single quotes

ilawilaw

"Review Bot" is the modern titling for the product, so the description should be reflective of that. (The repository has …

chipx86chipx86

E302 expected 2 blank lines, found 1

reviewbotreviewbot

E225 missing whitespace around operator

reviewbotreviewbot

E501 line too long (84 > 79 characters)

reviewbotreviewbot

E501 line too long (84 > 79 characters)

reviewbotreviewbot

Single quotes?

ilawilaw

Review Bot instead of reviewbot

ilawilaw

E501 line too long (84 > 79 characters)

reviewbotreviewbot

E501 line too long (84 > 79 characters)

reviewbotreviewbot

This doesn't appear related to the change.

chipx86chipx86

nose is a third-party module, so it should be part of its own import group.

chipx86chipx86

First line of the file should be a docstring summary of the file.

chipx86chipx86

unittest is part of Python, so it should be in its own import group (which would come before mock's).

chipx86chipx86

/ isn't a safe assumption for path separators. Instead, whenever we need a path, we should build it with os.path.join

chipx86chipx86

There's too much happening in this file and class. It'll get unwieldy as we go. What we should do is …

chipx86chipx86

"Review Bot" Missing a period.

chipx86chipx86

The space at the beginning of the summary must be removed.

chipx86chipx86

These must be valid Python types, so: list of dict

chipx86chipx86

This doesn't quite match the way we do argument summaries. Something more like: Each dictionary represents a file in the …

chipx86chipx86

It would be really nice not to use Mock. While mocks can be useful, they can also make it increasingly …

chipx86chipx86

Blank line between statements and blocks.

chipx86chipx86

Blank line between statements and blocks.

chipx86chipx86

Blank line between blocks.

chipx86chipx86

Blank line between statements and blocks.

chipx86chipx86

Blank line between statements and blocks.

chipx86chipx86

Docstring summaries must be confined to one line. This can be: Assert that comments in a review match expected comments.

chipx86chipx86

dict

chipx86chipx86

For the tuples, turn them into literals, like: ... and value is a tuple in the form of ``(comment, line, …

chipx86chipx86

Needs to be bool, optional, since the argument doesn't have to be provided. The content is also indented too far.

chipx86chipx86

When we get rid of mocks, and use kgb, we'll be able to get rid of the if and just …

chipx86chipx86

Must be one line.

chipx86chipx86

Make sure blank lines, indentation, types, and formatting are correct. Also, don't forget the , optional optional arguments. Types that …

chipx86chipx86

unicode, not string.

chipx86chipx86

Please use kgb instead of mocks here. Also, these are very specific to flake8, which means this function is useless …

chipx86chipx86

This must exist in a test case (and file) specific to this tool.

chipx86chipx86

The format for testing docstrings should be in the form of: """Testing <toolname> with <condition>""" This helps keep the output …

chipx86chipx86

This must be in a Flake8-specific test case and file.

chipx86chipx86

E501 line too long (107 > 79 characters)

reviewbotreviewbot

E501 line too long (107 > 79 characters)

reviewbotreviewbot

E501 line too long (106 > 79 characters)

reviewbotreviewbot

E501 line too long (93 > 79 characters)

reviewbotreviewbot

E501 line too long (93 > 79 characters)

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

flake8

ammar
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

ilaw
  1. 
      
  2. Description should be wrapped at 72 characters?

  3. Double quotes are used in some places where it should be single quotes

  4. bot/reviewbot/tools/tests/tests.py (Diff revision 2)
     
     

    Single quotes?

  5. bot/reviewbot/tools/tests/tests.py (Diff revision 2)
     
     

    Review Bot instead of reviewbot

  6. bot/reviewbot/tools/tests/tests.py (Diff revision 2)
     
     
     
     
     
     
     
     

    Just being picky but is it not in the same order as the credentials check test (i.e. review and expected_comments)

  7. 
      
ammar
Review request changed

Description:

   

Previously, we did not have tests for tools in ReviewBot.

   
~  

This change adds a framework for testing the output parsing logic of the ReviewBot tool classes. Additionally, two sample unit tests have been added to demonstrate how I envision this framework being used.

  ~

This change adds a framework for testing the output parsing logic of

  + the ReviewBot tool classes. Additionally, two sample unit tests have
  + been added to demonstrate how I envision this framework being used.

Commit:

-81e278667372b8a2a6641a639e1a99893d803dbb
+f18b5549e0a5555b0f668facfe83dad25206af92

Diff:

Revision 3 (+128)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

ammar
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

ammar
chipx86
  1. 
      
  2. "Review Bot" is the modern titling for the product, so the description should be reflective of that. (The repository has the old name for legacy purposes.)

  3. bot/reviewbot/repositories.py (Diff revision 5)
     
     

    This doesn't appear related to the change.

  4. bot/reviewbot/tests/runtests.py (Diff revision 5)
     
     
     
     

    nose is a third-party module, so it should be part of its own import group.

  5. bot/reviewbot/tools/tests/tests.py (Diff revision 5)
     
     

    First line of the file should be a docstring summary of the file.

  6. bot/reviewbot/tools/tests/tests.py (Diff revision 5)
     
     
     

    unittest is part of Python, so it should be in its own import group (which would come before mock's).

  7. bot/reviewbot/tools/tests/tests.py (Diff revision 5)
     
     

    / isn't a safe assumption for path separators. Instead, whenever we need a path, we should build it with os.path.join

  8. bot/reviewbot/tools/tests/tests.py (Diff revision 5)
     
     

    There's too much happening in this file and class. It'll get unwieldy as we go.

    What we should do is have:

    1. A testcase.py file in bot/reviewbot/tools/tests/ that contains a base BaseToolTestCase class that tool-specific tests can inherit from. This should only contain generic methods that are useful for tool-related test suites.
    2. A file per tool, named after the tool, that contains a test case class subclassing BaseToolTestCase that only contains functionality specific to that test.

    We use this pattern in most of our code elsewhere.

  9. bot/reviewbot/tools/tests/tests.py (Diff revision 5)
     
     

    "Review Bot"

    Missing a period.

  10. bot/reviewbot/tools/tests/tests.py (Diff revision 5)
     
     

    The space at the beginning of the summary must be removed.

  11. bot/reviewbot/tools/tests/tests.py (Diff revision 5)
     
     

    These must be valid Python types, so: list of dict

  12. bot/reviewbot/tools/tests/tests.py (Diff revision 5)
     
     

    This doesn't quite match the way we do argument summaries. Something more like:

    Each dictionary represents a file in the review. It must have the following keys:
    
    ``dest_file`` (:py:class:`unicode`):
        The filename of the diff being reviewed.
    
     ``patched_file_contents`` (:py:class:`bytes`):
         The contents of the diff for the file.
    

    Something like that.

  13. bot/reviewbot/tools/tests/tests.py (Diff revision 5)
     
     
     
     
     

    It would be really nice not to use Mock. While mocks can be useful, they can also make it increasingly harder to test real-world stuff. Instead, since we control the Review and File classes, we should be using those instead. We can always use KGB to spy on any API requests to avoid those requests being made.

    By having the proper objects, we get increased testing of those objects, and we don't have to start going through and mocking more and more and more behavior as we add tests (diverging even further from core functionality and behavior and more toward mock-specific behavior).

  14. bot/reviewbot/tools/tests/tests.py (Diff revision 5)
     
     
     

    Blank line between statements and blocks.

  15. bot/reviewbot/tools/tests/tests.py (Diff revision 5)
     
     
     

    Blank line between statements and blocks.

  16. bot/reviewbot/tools/tests/tests.py (Diff revision 5)
     
     
     

    Blank line between blocks.

  17. bot/reviewbot/tools/tests/tests.py (Diff revision 5)
     
     
     

    Blank line between statements and blocks.

  18. bot/reviewbot/tools/tests/tests.py (Diff revision 5)
     
     
     

    Blank line between statements and blocks.

  19. bot/reviewbot/tools/tests/tests.py (Diff revision 5)
     
     
     

    Docstring summaries must be confined to one line. This can be:

    Assert that comments in a review match expected comments.
    
  20. bot/reviewbot/tools/tests/tests.py (Diff revision 5)
     
     
     
     
     

    For the tuples, turn them into literals, like:

    ... and value is a tuple in the form of
    ``(comment, line, issue)`` or ``(comment, line)``
    

    so it's more clear.

  21. bot/reviewbot/tools/tests/tests.py (Diff revision 5)
     
     
     
     

    Needs to be bool, optional, since the argument doesn't have to be provided.

    The content is also indented too far.

  22. bot/reviewbot/tools/tests/tests.py (Diff revision 5)
     
     
     
     
     
     

    When we get rid of mocks, and use kgb, we'll be able to get rid of the if and just have the one loop and assertion, since kgb does the right thing with these calls and argument defaults.

  23. bot/reviewbot/tools/tests/tests.py (Diff revision 5)
     
     
     

    Must be one line.

  24. bot/reviewbot/tools/tests/tests.py (Diff revision 5)
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    Make sure blank lines, indentation, types, and formatting are correct. Also, don't forget the , optional optional arguments.

    Types that are classes must be the absolute module path to the class.

  25. bot/reviewbot/tools/tests/tests.py (Diff revision 5)
     
     

    unicode, not string.

  26. bot/reviewbot/tools/tests/tests.py (Diff revision 5)
     
     
     
     
     

    Please use kgb instead of mocks here.

    Also, these are very specific to flake8, which means this function is useless for every other type of tool. Either this should exist in a Flake8-specific test case, or should be made generic.

  27. bot/reviewbot/tools/tests/tests.py (Diff revision 5)
     
     

    This must exist in a test case (and file) specific to this tool.

  28. bot/reviewbot/tools/tests/tests.py (Diff revision 5)
     
     

    The format for testing docstrings should be in the form of:

    """Testing <toolname> with <condition>"""
    

    This helps keep the output consistent and readable.

    Also, no period at the end.

  29. bot/reviewbot/tools/tests/tests.py (Diff revision 5)
     
     

    This must be in a Flake8-specific test case and file.

  30. 
      
ammar
Review request changed

Description:

~  

Previously, we did not have tests for tools in ReviewBot.

  ~

Previously, we did not have tests for tools in Review Bot.

   
   

This change adds a framework for testing the output parsing logic of

~   the ReviewBot tool classes. Additionally, two sample unit tests have
  ~ the Review Bot tool classes. Additionally, two sample unit tests have
    been added to demonstrate how I envision this framework being used.

Commit:

-97f739c0faced78b8e09789511d08dadbdddfc3d
+439194991888e023987be7b5ce2d51f3a4e4e0b6

Diff:

Revision 6 (+184)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

ammar
david
Review request changed

Status: Discarded

Loading...