Add testing framework for ReviewBot tools

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

ammar
ReviewBot
master
97f739c...
reviewbot, students

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.

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

  • 28
  • 0
  • 7
  • 4
  • 39
Description From Last Updated
"Review Bot" is the modern titling for the product, so the description should be reflective of that. (The repository has ... chipx86 chipx86
nose is a third-party module, so it should be part of its own import group. chipx86 chipx86
First line of the file should be a docstring summary of the file. chipx86 chipx86
unittest is part of Python, so it should be in its own import group (which would come before mock's). chipx86 chipx86
/ isn't a safe assumption for path separators. Instead, whenever we need a path, we should build it with os.path.join chipx86 chipx86
There's too much happening in this file and class. It'll get unwieldy as we go. What we should do is ... chipx86 chipx86
"Review Bot" Missing a period. chipx86 chipx86
The space at the beginning of the summary must be removed. chipx86 chipx86
These must be valid Python types, so: list of dict chipx86 chipx86
This doesn't quite match the way we do argument summaries. Something more like: Each dictionary represents a file in the ... chipx86 chipx86
It would be really nice not to use Mock. While mocks can be useful, they can also make it increasingly ... chipx86 chipx86
Blank line between statements and blocks. chipx86 chipx86
Blank line between statements and blocks. chipx86 chipx86
Blank line between blocks. chipx86 chipx86
Blank line between statements and blocks. chipx86 chipx86
Blank line between statements and blocks. chipx86 chipx86
Docstring summaries must be confined to one line. This can be: Assert that comments in a review match expected comments. chipx86 chipx86
dict chipx86 chipx86
For the tuples, turn them into literals, like: ... and value is a tuple in the form of ``(comment, line, ... chipx86 chipx86
Needs to be bool, optional, since the argument doesn't have to be provided. The content is also indented too far. chipx86 chipx86
When we get rid of mocks, and use kgb, we'll be able to get rid of the if and just ... chipx86 chipx86
Must be one line. chipx86 chipx86
Make sure blank lines, indentation, types, and formatting are correct. Also, don't forget the , optional optional arguments. Types that ... chipx86 chipx86
unicode, not string. chipx86 chipx86
Please use kgb instead of mocks here. Also, these are very specific to flake8, which means this function is useless ... chipx86 chipx86
This must exist in a test case (and file) specific to this tool. chipx86 chipx86
The format for testing docstrings should be in the form of: """Testing <toolname> with <condition>""" This helps keep the output ... chipx86 chipx86
This must be in a Flake8-specific test case and file. chipx86 chipx86
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
Review request changed

Commit:

-70d819153ec3adb6251821610f273035087efc5d
+97f739c0faced78b8e09789511d08dadbdddfc3d

Diff:

Revision 5 (+157 -1)

Show changes

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
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. 
      
Loading...