Summary

Add BaseTestRunner in djblet

Review Request #7932 — Created Jan. 30, 2016 and discarded Sept. 16, 2016, 11 a.m.

Information

Owner

weijie

Repository

Djblets

Branch

master

Bugs

Depends On

Blocks

7934

Reviewers

Groups

djblets

People

Description

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

Testing Done

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

Issues

Description	From	Last Updated
'pkg_resources' imported but unused	reviewbot	Feb. 25, 2016, 3:28 p.m.
'ExtensionInfo' imported but unused	reviewbot	Feb. 25, 2016, 3:28 p.m.
'RegisteredExtension' imported but unused	reviewbot	Feb. 25, 2016, 3:28 p.m.
'ExtensionHookPoint' imported but unused	reviewbot	Feb. 25, 2016, 3:28 p.m.
'ExtensionHook' imported but unused	reviewbot	Feb. 25, 2016, 3:28 p.m.
'Settings' imported but unused	reviewbot	Feb. 25, 2016, 3:28 p.m.
FakeEntryPoint, FakeDistribution, TestExtensionManager, TestExtension, TestExtensionWithRegistration... I'm guessing this you're using this to test your testing framework? Very Matryoshka doll. :) …	mike_conley	Feb. 24, 2016, 8:54 p.m.
Col: 1 W191 indentation contains tabs	reviewbot	Feb. 25, 2016, 3:29 p.m.
Col: 1 W191 indentation contains tabs	reviewbot	Feb. 25, 2016, 3:29 p.m.
Col: 1 W191 indentation contains tabs	reviewbot	Feb. 25, 2016, 3:29 p.m.
Col: 1 W191 indentation contains tabs	reviewbot	Feb. 25, 2016, 3:29 p.m.
Col: 1 W191 indentation contains tabs	reviewbot	Feb. 25, 2016, 3:29 p.m.
Col: 1 W191 indentation contains tabs	reviewbot	Feb. 25, 2016, 3:29 p.m.
Col: 1 W191 indentation contains tabs	reviewbot	Feb. 25, 2016, 3:29 p.m.
Col: 1 W191 indentation contains tabs	reviewbot	Feb. 25, 2016, 3:29 p.m.
Col: 1 W191 indentation contains tabs	reviewbot	Feb. 25, 2016, 3:29 p.m.
Col: 1 W191 indentation contains tabs	reviewbot	Feb. 25, 2016, 3:29 p.m.
Col: 1 W191 indentation contains tabs	reviewbot	Feb. 25, 2016, 3:29 p.m.
Col: 1 W191 indentation contains tabs	reviewbot	Feb. 25, 2016, 3:29 p.m.
Col: 1 W191 indentation contains tabs	reviewbot	Feb. 25, 2016, 3:29 p.m.
Col: 1 W191 indentation contains tabs	reviewbot	Feb. 25, 2016, 3:29 p.m.
Col: 1 W191 indentation contains tabs	reviewbot	Feb. 25, 2016, 3:29 p.m.
Col: 1 W191 indentation contains tabs	reviewbot	Feb. 25, 2016, 3:29 p.m.
Col: 1 W191 indentation contains tabs	reviewbot	Feb. 25, 2016, 3:29 p.m.
Col: 1 W191 indentation contains tabs	reviewbot	Feb. 25, 2016, 3:29 p.m.
Col: 1 W191 indentation contains tabs	reviewbot	Feb. 25, 2016, 3:29 p.m.
Col: 1 W191 indentation contains tabs	reviewbot	Feb. 25, 2016, 3:29 p.m.
Col: 1 W191 indentation contains tabs	reviewbot	Feb. 25, 2016, 3:29 p.m.
Col: 5 E101 indentation contains mixed spaces and tabs	reviewbot	Feb. 25, 2016, 3:29 p.m.
Col: 6 E128 continuation line under-indented for visual indent	reviewbot	Feb. 25, 2016, 3:29 p.m.
Col: 1 W191 indentation contains tabs	reviewbot	Feb. 25, 2016, 3:29 p.m.
Col: 5 E101 indentation contains mixed spaces and tabs	reviewbot	Feb. 25, 2016, 3:29 p.m.
Col: 6 E128 continuation line under-indented for visual indent	reviewbot	Feb. 25, 2016, 3:29 p.m.
Col: 1 W191 indentation contains tabs	reviewbot	Feb. 25, 2016, 3:29 p.m.
Col: 5 E101 indentation contains mixed spaces and tabs	reviewbot	Feb. 25, 2016, 3:29 p.m.
Col: 6 E128 continuation line under-indented for visual indent	reviewbot	Feb. 25, 2016, 3:29 p.m.
Col: 1 W191 indentation contains tabs	reviewbot	Feb. 25, 2016, 3:29 p.m.
'tempfile' imported but unused	reviewbot	Feb. 25, 2016, 3:29 p.m.
'execute_from_command_line' imported but unused	reviewbot	Feb. 25, 2016, 3:29 p.m.
'generate_media_serial' imported but unused	reviewbot	Feb. 25, 2016, 3:29 p.m.
'tempfile' imported but unused	reviewbot	Feb. 25, 2016, 3:25 p.m.
'execute_from_command_line' imported but unused	reviewbot	Feb. 25, 2016, 3:25 p.m.
'generate_media_serial' imported but unused	reviewbot	Feb. 25, 2016, 3:25 p.m.
Col: 80 E501 line too long (91 > 79 characters)	reviewbot	Feb. 25, 2016, 3:26 p.m.
Col: 30 E251 unexpected spaces around keyword / parameter equals	reviewbot	Feb. 25, 2016, 3:28 p.m.
Col: 32 E251 unexpected spaces around keyword / parameter equals	reviewbot	Feb. 25, 2016, 3:28 p.m.
I talked to david, and he said we should probably just drop this license block. There's already a COPYING that's …	mike_conley	April 5, 2016, 8:12 p.m.
I imagine we'll want RBTestRunner to extend this in reviewboard/reviewboard/test.py, right?	mike_conley	April 5, 2016, 8:06 p.m.
What's this?	mike_conley	April 5, 2016, 8:19 p.m.
test_labels isn't actually be used here, btw. What's it for? Can we just remove it?	mike_conley	April 5, 2016, 8:17 p.m.
Out of curiosity, why do you not need the --with-id argument that is being used by RBTestRunner?	mike_conley	April 5, 2016, 8:19 p.m.
There shouldn't be anything reviewboard specific in here. RBTestRunner will still want this though, so you either need to have …	mike_conley	April 5, 2016, 8:14 p.m.
Please remove this.	mike_conley	April 5, 2016, 8:14 p.m.
Please remove this print	mike_conley	April 5, 2016, 8:15 p.m.
This stuff got yanked out of RBTestRunner in https://github.com/reviewboard/reviewboard/commit/001415dfaedb4a7db355e7547e4bf0497d3b3cd5 - you should probably do the same.	mike_conley	April 5, 2016, 8:57 p.m.
Instead of this inline documentation fragment, it'd probably be best to have a docstring. Same goes for the other methods …	mike_conley	April 5, 2016, 8:32 p.m.
Why is this environment variable needed?	mike_conley	April 7, 2016, 11:39 a.m.

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.

djblets/extensions/test/extension.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 'pkg_resources' imported but unused
```
djblets/extensions/test/extension.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 'ExtensionInfo' imported but unused
```
djblets/extensions/test/extension.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 'RegisteredExtension' imported but unused
```
djblets/extensions/test/extension.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 'ExtensionHookPoint' imported but unused
```
djblets/extensions/test/extension.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 'ExtensionHook' imported but unused
```
djblets/extensions/test/extension.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 'Settings' imported but unused
```
djblets/extensions/test/extension.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 1
 W191 indentation contains tabs
```
djblets/extensions/test/extension.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 1
 W191 indentation contains tabs
```
djblets/extensions/test/extension.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 1
 W191 indentation contains tabs
```
djblets/extensions/test/extension.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 1
 W191 indentation contains tabs
```
djblets/extensions/test/extension.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 1
 W191 indentation contains tabs
```
djblets/extensions/test/extension.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 1
 W191 indentation contains tabs
```
djblets/extensions/test/extension.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 1
 W191 indentation contains tabs
```
djblets/extensions/test/extension.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 1
 W191 indentation contains tabs
```
djblets/extensions/test/extension.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 1
 W191 indentation contains tabs
```
djblets/extensions/test/extension.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 1
 W191 indentation contains tabs
```
djblets/extensions/test/extension.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 1
 W191 indentation contains tabs
```
djblets/extensions/test/extension.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 1
 W191 indentation contains tabs
```
djblets/extensions/test/extension.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 1
 W191 indentation contains tabs
```
djblets/extensions/test/extension.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 1
 W191 indentation contains tabs
```
djblets/extensions/test/extension.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 1
 W191 indentation contains tabs
```
djblets/extensions/test/extension.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 1
 W191 indentation contains tabs
```
djblets/extensions/test/extension.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 1
 W191 indentation contains tabs
```
djblets/extensions/test/extension.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 1
 W191 indentation contains tabs
```
djblets/extensions/test/extension.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 1
 W191 indentation contains tabs
```
djblets/extensions/test/extension.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 1
 W191 indentation contains tabs
```
djblets/extensions/test/extension.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 1
 W191 indentation contains tabs
```
djblets/extensions/test/extension.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 5
 E101 indentation contains mixed spaces and tabs
```
djblets/extensions/test/extension.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 6
 E128 continuation line under-indented for visual indent
```
djblets/extensions/test/extension.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 1
 W191 indentation contains tabs
```
djblets/extensions/test/extension.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 5
 E101 indentation contains mixed spaces and tabs
```
djblets/extensions/test/extension.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 6
 E128 continuation line under-indented for visual indent
```
djblets/extensions/test/extension.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 1
 W191 indentation contains tabs
```
djblets/extensions/test/extension.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 5
 E101 indentation contains mixed spaces and tabs
```
djblets/extensions/test/extension.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 6
 E128 continuation line under-indented for visual indent
```
djblets/extensions/test/extension.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 1
 W191 indentation contains tabs
```
djblets/testing/testrunner.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 'tempfile' imported but unused
```
djblets/testing/testrunner.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 'execute_from_command_line' imported but unused
```
djblets/testing/testrunner.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 'generate_media_serial' imported but unused
```

djblets/extensions/test/extension.py (Diff revision 1)

The issue has been resolved. Show all issues

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.

Diff:

Revision 2 (+137)

Show changes

djblets/testing/testrunner.py

Tool: PEP8 Style Checker
Processed Files:
    djblets/testing/testrunner.py



Tool: Pyflakes
Processed Files:
    djblets/testing/testrunner.py

djblets/testing/testrunner.py (Diff revision 2)
The issue has been resolved. Show all issues
```
 'tempfile' imported but unused
```
djblets/testing/testrunner.py (Diff revision 2)
The issue has been resolved. Show all issues
```
 'execute_from_command_line' imported but unused
```
djblets/testing/testrunner.py (Diff revision 2)
The issue has been resolved. Show all issues
```
 'generate_media_serial' imported but unused
```
djblets/testing/testrunner.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (91 > 79 characters)
```
djblets/testing/testrunner.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 30
 E251 unexpected spaces around keyword / parameter equals
```
djblets/testing/testrunner.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 32
 E251 unexpected spaces around keyword / parameter equals
```

Diff:

Revision 3 (+134)

Show changes

djblets/testing/testrunner.py

Tool: PEP8 Style Checker
Processed Files:
    djblets/testing/testrunner.py



Tool: Pyflakes
Processed Files:
    djblets/testing/testrunner.py

Summary:: [WIP]in djblet add Extension testing framework in reviewboard
Add Extension testing framework in reviewboard in djblet

```
Thanks for your work Weijie! See below.
```

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

The issue has been resolved. Show all issues

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.

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

The issue has been resolved. Show all issues

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

weijie April 5, 2016, 8:10 p.m.

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

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

The issue has been resolved. Show all issues

What's this?

weijie April 5, 2016, 8:19 p.m.

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.

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

The issue has been resolved. Show all issues

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

weijie April 5, 2016, 8:19 p.m.

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

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

The issue has been resolved. Show all issues

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.

djblets/testing/testrunner.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Please remove this.
```
djblets/testing/testrunner.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Please remove this print
```

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

The issue has been resolved. Show all issues

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

weijie April 5, 2016, 8:19 p.m.

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.

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

The issue has been resolved. Show all issues

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

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

The issue has been resolved. Show all issues

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

Diff:

Revision 4 (+106)

Show changes

djblets/testing/testrunner.py

Tool: PEP8 Style Checker
Processed Files:
    djblets/testing/testrunner.py



Tool: Pyflakes
Processed Files:
    djblets/testing/testrunner.py

Diff:

Revision 5 (+97)

Show changes

djblets/testing/testrunner.py

Tool: PEP8 Style Checker
Processed Files:
    djblets/testing/testrunner.py



Tool: Pyflakes
Processed Files:
    djblets/testing/testrunner.py

Hey weijei - just one question here. Also, you need to fill in the Testing Done section.

djblets/testing/testrunner.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Why is this environment variable needed?
```

Diff:

Revision 6 (+96)

Show changes

djblets/testing/testrunner.py

Tool: PEP8 Style Checker
Processed Files:
    djblets/testing/testrunner.py



Tool: Pyflakes
Processed Files:
    djblets/testing/testrunner.py

Can you also please fill in the Testing Done section?

Testing Done:: +
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

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.

weijie April 7, 2016, 2:37 p.m.

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

Summary:: Add Extension testing framework in reviewboard in djblet
Add BaseTestRunner in djblet
Description:: ~
Create a test runner class in Djblets, called BaseTestRunner in the djblets/testing/testrunner.py
~
Creates a test runner class called BaseTestRunner which uses the nose unit test framework. Then apps can subclass to run their test suites.
-
-
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)"
-
-
Build a testcase class for the extension testing stuff that registers/enables the extension in setUp , and shuts it down in tearDown

Status:: Discarded
Change Summary:: A similar but separate implementation has landed for Djblets 0.10

	+	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

~		Create a test runner class in Djblets, called BaseTestRunner in the djblets/testing/testrunner.py
	~	Creates a test runner class called BaseTestRunner which uses the nose unit test framework. Then apps can subclass to run their test suites.
-
-		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)"
-
-		Build a testcase class for the extension testing stuff that registers/enables the extension in setUp , and shuts it down in tearDown