Add BaseTestRunner in djblet

Review Request #7932 — Created Jan. 30, 2016 and discarded

weijie
Djblets
master
7934
djblets

Creates a test runner class called BaseTestRunner which uses the nose unit test framework. Then apps can subclass to run their test suites.

I have tested the test runner in the real and fake extensions.
I add the document to introduce how to use the test extension framework
in reviewboard/docs/manual/extending/extensions/testing.rst

Description From Last Updated

'pkg_resources' imported but unused

reviewbotreviewbot

'ExtensionInfo' imported but unused

reviewbotreviewbot

'RegisteredExtension' imported but unused

reviewbotreviewbot

'ExtensionHookPoint' imported but unused

reviewbotreviewbot

'ExtensionHook' imported but unused

reviewbotreviewbot

'Settings' imported but unused

reviewbotreviewbot

FakeEntryPoint, FakeDistribution, TestExtensionManager, TestExtension, TestExtensionWithRegistration... I'm guessing this you're using this to test your testing framework? Very Matryoshka doll. :) ...

mike_conleymike_conley

Col: 1 W191 indentation contains tabs

reviewbotreviewbot

Col: 1 W191 indentation contains tabs

reviewbotreviewbot

Col: 1 W191 indentation contains tabs

reviewbotreviewbot

Col: 1 W191 indentation contains tabs

reviewbotreviewbot

Col: 1 W191 indentation contains tabs

reviewbotreviewbot

Col: 1 W191 indentation contains tabs

reviewbotreviewbot

Col: 1 W191 indentation contains tabs

reviewbotreviewbot

Col: 1 W191 indentation contains tabs

reviewbotreviewbot

Col: 1 W191 indentation contains tabs

reviewbotreviewbot

Col: 1 W191 indentation contains tabs

reviewbotreviewbot

Col: 1 W191 indentation contains tabs

reviewbotreviewbot

Col: 1 W191 indentation contains tabs

reviewbotreviewbot

Col: 1 W191 indentation contains tabs

reviewbotreviewbot

Col: 1 W191 indentation contains tabs

reviewbotreviewbot

Col: 1 W191 indentation contains tabs

reviewbotreviewbot

Col: 1 W191 indentation contains tabs

reviewbotreviewbot

Col: 1 W191 indentation contains tabs

reviewbotreviewbot

Col: 1 W191 indentation contains tabs

reviewbotreviewbot

Col: 1 W191 indentation contains tabs

reviewbotreviewbot

Col: 1 W191 indentation contains tabs

reviewbotreviewbot

Col: 1 W191 indentation contains tabs

reviewbotreviewbot

Col: 5 E101 indentation contains mixed spaces and tabs

reviewbotreviewbot

Col: 6 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 1 W191 indentation contains tabs

reviewbotreviewbot

Col: 5 E101 indentation contains mixed spaces and tabs

reviewbotreviewbot

Col: 6 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 1 W191 indentation contains tabs

reviewbotreviewbot

Col: 5 E101 indentation contains mixed spaces and tabs

reviewbotreviewbot

Col: 6 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 1 W191 indentation contains tabs

reviewbotreviewbot

'tempfile' imported but unused

reviewbotreviewbot

'execute_from_command_line' imported but unused

reviewbotreviewbot

'generate_media_serial' imported but unused

reviewbotreviewbot

'tempfile' imported but unused

reviewbotreviewbot

'execute_from_command_line' imported but unused

reviewbotreviewbot

'generate_media_serial' imported but unused

reviewbotreviewbot

Col: 80 E501 line too long (91 > 79 characters)

reviewbotreviewbot

Col: 30 E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

Col: 32 E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

I talked to david, and he said we should probably just drop this license block. There's already a COPYING that's ...

mike_conleymike_conley

I imagine we'll want RBTestRunner to extend this in reviewboard/reviewboard/test.py, right?

mike_conleymike_conley

What's this?

mike_conleymike_conley

test_labels isn't actually be used here, btw. What's it for? Can we just remove it?

mike_conleymike_conley

Out of curiosity, why do you not need the --with-id argument that is being used by RBTestRunner?

mike_conleymike_conley

There shouldn't be anything reviewboard specific in here. RBTestRunner will still want this though, so you either need to have ...

mike_conleymike_conley

Please remove this.

mike_conleymike_conley

Please remove this print

mike_conleymike_conley

This stuff got yanked out of RBTestRunner in https://github.com/reviewboard/reviewboard/commit/001415dfaedb4a7db355e7547e4bf0497d3b3cd5 - you should probably do the same.

mike_conleymike_conley

Instead of this inline documentation fragment, it'd probably be best to have a docstring. Same goes for the other methods ...

mike_conleymike_conley

Why is this environment variable needed?

mike_conleymike_conley
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        djblets/testing/testrunner.py
        djblets/extensions/test/extension.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        djblets/testing/testrunner.py
        djblets/extensions/test/extension.py
    
    
    WARNING: Number of comments exceeded maximum, showing 30 of 190.
  2. djblets/extensions/test/extension.py (Diff revision 1)
     
     
     'pkg_resources' imported but unused
    
  3. djblets/extensions/test/extension.py (Diff revision 1)
     
     
     'ExtensionInfo' imported but unused
    
  4. djblets/extensions/test/extension.py (Diff revision 1)
     
     
     'RegisteredExtension' imported but unused
    
  5. djblets/extensions/test/extension.py (Diff revision 1)
     
     
     'ExtensionHookPoint' imported but unused
    
  6. djblets/extensions/test/extension.py (Diff revision 1)
     
     
     'ExtensionHook' imported but unused
    
  7. djblets/extensions/test/extension.py (Diff revision 1)
     
     
     'Settings' imported but unused
    
  8. djblets/extensions/test/extension.py (Diff revision 1)
     
     
    Col: 1
     W191 indentation contains tabs
    
  9. djblets/extensions/test/extension.py (Diff revision 1)
     
     
    Col: 1
     W191 indentation contains tabs
    
  10. djblets/extensions/test/extension.py (Diff revision 1)
     
     
    Col: 1
     W191 indentation contains tabs
    
  11. djblets/extensions/test/extension.py (Diff revision 1)
     
     
    Col: 1
     W191 indentation contains tabs
    
  12. djblets/extensions/test/extension.py (Diff revision 1)
     
     
    Col: 1
     W191 indentation contains tabs
    
  13. djblets/extensions/test/extension.py (Diff revision 1)
     
     
    Col: 1
     W191 indentation contains tabs
    
  14. djblets/extensions/test/extension.py (Diff revision 1)
     
     
    Col: 1
     W191 indentation contains tabs
    
  15. djblets/extensions/test/extension.py (Diff revision 1)
     
     
    Col: 1
     W191 indentation contains tabs
    
  16. djblets/extensions/test/extension.py (Diff revision 1)
     
     
    Col: 1
     W191 indentation contains tabs
    
  17. djblets/extensions/test/extension.py (Diff revision 1)
     
     
    Col: 1
     W191 indentation contains tabs
    
  18. djblets/extensions/test/extension.py (Diff revision 1)
     
     
    Col: 1
     W191 indentation contains tabs
    
  19. djblets/extensions/test/extension.py (Diff revision 1)
     
     
    Col: 1
     W191 indentation contains tabs
    
  20. djblets/extensions/test/extension.py (Diff revision 1)
     
     
    Col: 1
     W191 indentation contains tabs
    
  21. djblets/extensions/test/extension.py (Diff revision 1)
     
     
    Col: 1
     W191 indentation contains tabs
    
  22. djblets/extensions/test/extension.py (Diff revision 1)
     
     
    Col: 1
     W191 indentation contains tabs
    
  23. djblets/extensions/test/extension.py (Diff revision 1)
     
     
    Col: 1
     W191 indentation contains tabs
    
  24. djblets/extensions/test/extension.py (Diff revision 1)
     
     
    Col: 1
     W191 indentation contains tabs
    
  25. djblets/extensions/test/extension.py (Diff revision 1)
     
     
    Col: 1
     W191 indentation contains tabs
    
  26. djblets/extensions/test/extension.py (Diff revision 1)
     
     
    Col: 1
     W191 indentation contains tabs
    
  27. djblets/extensions/test/extension.py (Diff revision 1)
     
     
    Col: 1
     W191 indentation contains tabs
    
  28. djblets/extensions/test/extension.py (Diff revision 1)
     
     
    Col: 1
     W191 indentation contains tabs
    
  29. djblets/extensions/test/extension.py (Diff revision 1)
     
     
    Col: 5
     E101 indentation contains mixed spaces and tabs
    
  30. djblets/extensions/test/extension.py (Diff revision 1)
     
     
    Col: 6
     E128 continuation line under-indented for visual indent
    
  31. djblets/extensions/test/extension.py (Diff revision 1)
     
     
    Col: 1
     W191 indentation contains tabs
    
  32. djblets/extensions/test/extension.py (Diff revision 1)
     
     
    Col: 5
     E101 indentation contains mixed spaces and tabs
    
  33. djblets/extensions/test/extension.py (Diff revision 1)
     
     
    Col: 6
     E128 continuation line under-indented for visual indent
    
  34. djblets/extensions/test/extension.py (Diff revision 1)
     
     
    Col: 1
     W191 indentation contains tabs
    
  35. djblets/extensions/test/extension.py (Diff revision 1)
     
     
    Col: 5
     E101 indentation contains mixed spaces and tabs
    
  36. djblets/extensions/test/extension.py (Diff revision 1)
     
     
    Col: 6
     E128 continuation line under-indented for visual indent
    
  37. djblets/extensions/test/extension.py (Diff revision 1)
     
     
    Col: 1
     W191 indentation contains tabs
    
  38. djblets/testing/testrunner.py (Diff revision 1)
     
     
     'tempfile' imported but unused
    
  39. djblets/testing/testrunner.py (Diff revision 1)
     
     
     'execute_from_command_line' imported but unused
    
  40. djblets/testing/testrunner.py (Diff revision 1)
     
     
     'generate_media_serial' imported but unused
    
  41. 
      
mike_conley
  1. 
      
  2. djblets/extensions/test/extension.py (Diff revision 1)
     
     

    FakeEntryPoint, FakeDistribution, TestExtensionManager, TestExtension, TestExtensionWithRegistration... I'm guessing this you're using this to test your testing framework? Very Matryoshka doll. :)

    I want to make sure the mental model is correct here - the framework you're building is to help extension developers test the extensions they've built for Review Board. That's very important to focus on. I don't want you to fall into the trap of writing tests for the extension framework itself.

  3. 
      
WE
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        djblets/testing/testrunner.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        djblets/testing/testrunner.py
    
    
  2. djblets/testing/testrunner.py (Diff revision 2)
     
     
     'tempfile' imported but unused
    
  3. djblets/testing/testrunner.py (Diff revision 2)
     
     
     'execute_from_command_line' imported but unused
    
  4. djblets/testing/testrunner.py (Diff revision 2)
     
     
     'generate_media_serial' imported but unused
    
  5. djblets/testing/testrunner.py (Diff revision 2)
     
     
    Col: 80
     E501 line too long (91 > 79 characters)
    
  6. djblets/testing/testrunner.py (Diff revision 2)
     
     
    Col: 30
     E251 unexpected spaces around keyword / parameter equals
    
  7. djblets/testing/testrunner.py (Diff revision 2)
     
     
    Col: 32
     E251 unexpected spaces around keyword / parameter equals
    
  8. 
      
WE
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        djblets/testing/testrunner.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        djblets/testing/testrunner.py
    
    
  2. 
      
WE
mike_conley
  1. Thanks for your work Weijie! See below.

  2. djblets/testing/testrunner.py (Diff revision 3)
     
     

    I talked to david, and he said we should probably just drop this license block. There's already a COPYING that's applied to the whole project except for special cases, and this can just inherit that if we remove this entire license block.

  3. djblets/testing/testrunner.py (Diff revision 3)
     
     

    I imagine we'll want RBTestRunner to extend this in reviewboard/reviewboard/test.py, right?

    1. Acctually not, since the RBtest has set some special settings, which do not need in the extension test.

  4. djblets/testing/testrunner.py (Diff revision 3)
     
     
     

    What's this?

    1. At that moment I am not sure the os.environ[b'BaseTestRunner'] should be right format. But Currently it should be the correct way. I will delete the comment. My bad.

  5. djblets/testing/testrunner.py (Diff revision 3)
     
     

    Out of curiosity, why do you not need the --with-id argument that is being used by RBTestRunner?

    1. You are right. that might be my problem to delete it.

  6. djblets/testing/testrunner.py (Diff revision 3)
     
     

    There shouldn't be anything reviewboard specific in here.

    RBTestRunner will still want this though, so you either need to have this base class use something it already knows to fill this field out, or have subclasses provide something that can be put in here.

  7. djblets/testing/testrunner.py (Diff revision 3)
     
     
     

    Please remove this.

  8. djblets/testing/testrunner.py (Diff revision 3)
     
     

    Please remove this print

  9. djblets/testing/testrunner.py (Diff revision 3)
     
     
     
     
     
     
     
     

    This stuff got yanked out of RBTestRunner in https://github.com/reviewboard/reviewboard/commit/001415dfaedb4a7db355e7547e4bf0497d3b3cd5 - you should probably do the same.

    1. Yes, What should I do is just reference the RBtestrunner, but delete the reviewboard specific stuff, since I don't want to change the RBtestrunner.

  10. djblets/testing/testrunner.py (Diff revision 3)
     
     
     

    Instead of this inline documentation fragment, it'd probably be best to have a docstring. Same goes for the other methods in here.

  11. 
      
mike_conley
  1. 
      
  2. djblets/testing/testrunner.py (Diff revision 3)
     
     

    test_labels isn't actually be used here, btw. What's it for? Can we just remove it?

  3. 
      
WE
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        djblets/testing/testrunner.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        djblets/testing/testrunner.py
    
    
  2. 
      
WE
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        djblets/testing/testrunner.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        djblets/testing/testrunner.py
    
    
  2. 
      
mike_conley
  1. Hey weijei - just one question here. Also, you need to fill in the Testing Done section.

  2. djblets/testing/testrunner.py (Diff revision 5)
     
     

    Why is this environment variable needed?

  3. 
      
WE
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        djblets/testing/testrunner.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        djblets/testing/testrunner.py
    
    
  2. 
      
mike_conley
  1. Can you also please fill in the Testing Done section?

  2. 
      
WE
mike_conley
  1. Sorry, weijei - a few more non-code things just caught my eye.

    In the summary, "djblet" -> "djblets". Also, you're not actually adding an extension testing framework to Djblets; you're adding a BaseTestRunner.

    The description of this review request is going to be used to create the commit message that this lands with, so can you please update the description so that it's phrased in the present tense?

    Example:

    "Create a test runner class in Djblets, called BaseTestRunner in the djblets/testing/testrunner.py"

    should be

    "Creates a test runner class called BaseTestRunner which apps can subclass to run their test suites", or something along those lines.

    'something still confused "have it run nose, but allow a subclass to define the more specific options to pass in (like, let it define the list of modules and such)'
    ^-- I'm not sure what this means, and it probably shouldn't be in here. If this is a question, you should ask it in Slack and get it answered.

    'Build a testcase class for the extension testing stuff that registers/enables the extension in setUp , and shuts it down in tearDown'
    ^-- this isn't part of this patch.

    1. Problem solved, the question about subclass option is the testpackage which uses the settings to pass the parameter.
      Sorry for forgetting updating it.

  2. 
      
WE
david
Review request changed

Status: Discarded

Change Summary:

A similar but separate implementation has landed for Djblets 0.10

Loading...