Opened 12 years ago

Closed 12 years ago

Last modified 12 years ago

#4004 enhancement closed fixed (fixed)

Add a subunit reporter to Trial

Reported by: robertc Owned by:
Priority: normal Milestone:
Component: trial Keywords:
Cc: Thijs Triemstra, therve, Jean-Paul Calderone Branch: branches/subunit-4004
branch-diff, diff-cov, branch-cov, buildbot
Author: jml

Description (last modified by Jonathan Lange)

Subunit is a wire protocol for communicating test results. See https://edge.launchpad.net/subunit for more details.

One of its main advantages is that it makes it much easier to run tests out of process, which in turn is useful for writing GUIs and for writing distributed testing tools.

Subunit has a Python library. If this library is installed on a system, Trial should offer a reporter that prints test results in subunit format.

Attachments (3)

draft.patch (1.5 KB) - added by robertc 12 years ago.
Working reporter without tests
subunit.patch (8.0 KB) - added by robertc 12 years ago.
Complete patch for review
simpler-subunit.patch (1.5 KB) - added by Jean-Paul Calderone 12 years ago.
Patch against the branch which highlights all untested code by deleting it

Download all attachments as: .zip

Change History (46)

Changed 12 years ago by robertc

Attachment: draft.patch added

Working reporter without tests

comment:1 Changed 12 years ago by Jonathan Lange

Component: coretrial
Owner: changed from Glyph to robertc

Changed 12 years ago by robertc

Attachment: subunit.patch added

Complete patch for review

comment:2 Changed 12 years ago by robertc

Keywords: review added
Owner: changed from robertc to Jonathan Lange

comment:3 Changed 12 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: changed from Jonathan Lange to robertc

Thanks. This might be interesting as more and more tools are added to the testing arena.

  1. I think it would be good if there were a release of subunit before Twisted gained an (even optional) dependency on it. I see that "recent" (this seems to mean karmic and newer - ie, future) versions of Ubuntu and Debian have packages for it. What about Windows, OS X, BSD, etc? A source tarball release would be nice. Aside from just making it easier for people to get this functionality on more than one platform, it's nice to know what version of subunit this is expected to work against.
  2. TestReporterInterface needs a class docstring
  3. None of the tests on TestReporterInterface have any assertions. I suppose the point of these tests is to make sure the required methods are implemented with the required signatures, not to verify anything about their behavior otherwise? If so, then zope.interface.verify.verifyObject is a more concise way to achieve the same thing.
  4. TestReporter needs a class docstring
  5. TestSubunitReporter needs a class docstring.
  6. _exc_info_or_Failure_to_exc_info is not named or documented in compliance with the coding standard. Plus, it's private... but exported?
  7. There's no test coverage for the subunit-not-installed code paths.
  8. There's no coverage for the plugin definition.

comment:4 Changed 12 years ago by Thijs Triemstra

Cc: Thijs Triemstra added

New methods and classes need an @since: 9.0 addition and make sure all modified files have an updated copyright header.

comment:5 in reply to:  3 Changed 12 years ago by robertc

Owner: changed from robertc to Jonathan Lange

Replying to exarkun:

Thanks. This might be interesting as more and more tools are added to the testing arena.

  1. I think it would be good if there were a release of subunit before Twisted gained an (even optional) dependency on it. I see that "recent" (this seems to mean karmic and newer - ie, future) versions of Ubuntu and Debian have packages for it. What about Windows, OS X, BSD, etc? A source tarball release would be nice. Aside from just making it easier for people to get this functionality on more than one platform, it's nice to know what version of subunit this is expected to work against.

We're working on a 0.0.2 release; just waiting on a couple of reviews and some perl binding improvements.

  1. TestReporterInterface needs a class docstring
  2. TestReporter needs a class docstring
  3. TestSubunitReporter needs a class docstring.

Is there value in these? The other tests that I rearranged to make these three test classes don't have class strings :- TestReporter already existed. *maybe* 50% of the test classes in that file have class strings, and of those 2 are not trivial s/[A-Z]/ &/g conversions of the class.

I'm certainly happy to make these changes, but IMO docstrings that are no different to the class detract from readability, not add to it.

  1. None of the tests on TestReporterInterface have any assertions. I suppose the point of these tests is to make sure the required methods are implemented with the required signatures, not to verify anything about their behavior otherwise? If so, then zope.interface.verify.verifyObject is a more concise way to achieve the same thing.

verifyObject asserts that 'foo' has the attributes, and that they are callable. It doesn't call them. Those tests failed as I wrote them, in a couple of cases: they added value. If there was an 'assertDoesNotRaise', I might use that ;). I think verifyObject is nice, and would use it, though the interface would need to be split into deprecated and non deprecated variants, otherwise you cause all new code to implement deprecated API's, which doesn't make sense.

  1. _exc_info_or_Failure_to_exc_info is not named or documented in compliance with the coding standard. Plus, it's private... but exported?

I can drop the _; In unittest.py (upstream), there is a 'protected' method _exc_info, which was overridden in unittest.py (twisted) and then had the function pulled out from it in this patch for reuse. I think its actually more recognisable to folk that work on unittest's guts as it stansd, but let me know.

  1. There's no test coverage for the subunit-not-installed code paths.

Suggestions on how to do that cleanly, pithily, and without starting a new interpreter appreciated.

  1. There's no coverage for the plugin definition.

I couldn't find any coverage for the others either.

I'm doing this patch opportunistically, so I'll probably leave it at this point, and someone more familiar can make these tweaks as they land it.

Assigning back to jml as thats where he asked me to leave it last night.

comment:6 Changed 12 years ago by Jonathan Lange

Author: jml
Branch: branches/subunit-4004

(In [27637]) Branching to 'subunit-4004'

comment:7 Changed 12 years ago by Jonathan Lange

(In [27639]) Add docstrings for new test classes, addressing 2, 3, 4, 5 from the review.

refs #4004

comment:8 Changed 12 years ago by Jonathan Lange

(In [27640]) Rename _exc_info_or_Failure_to_exc_info to excInfoOrFailureToExcInfo and update the docs to be as standard-compliant as I can remember.

Addresses review point 6.

refs #4004

comment:9 Changed 12 years ago by Jonathan Lange

(In [27641]) Mention the optional subunit dependency, incl version number, in INSTALL, addressing point 1. refs #4004

comment:10 Changed 12 years ago by Jonathan Lange

(In [27645]) Test the plugin definition itself, addressing point 8. refs #4004.

comment:11 Changed 12 years ago by Jonathan Lange

  1. Subunit has been released. I've added notes to INSTALL about the optional dependency and minimum supported version number.
  2. Docstring added for TestReporterInterface.
  3. I think Rob's comment about the lack of assertion methods and not using verifyObject is appropriate. I've modified that comment and added it to the source.
  4. Docstring added for TestReporter.
  5. Docstring added for TestSubunitReporter.
  6. _exc_info_or_Failure_to_exc_info renamed to excInfoOrFailureToExcInfo. Still exported.
  7. Tests added for subunit not being installed. This required changing the subunit reporter to use composition and not inheritance.
  8. I added tests for the plugin definition.

comment:12 Changed 12 years ago by Jonathan Lange

Keywords: review added

comment:13 Changed 12 years ago by Jonathan Lange

Owner: Jonathan Lange deleted

comment:14 Changed 12 years ago by therve

Cc: therve added
Keywords: review removed
Owner: set to Jonathan Lange
  • 2 (worrying) pyflakes:
    twisted/trial/reporter.py:951: undefined name 'addSuccess'
    twisted/trial/test/test_reporter.py:906: redefinition of function 'test_addUnexpectedSuccess' from line 851
    
  • +    # Most of the tests here don't have assertions. They are here to test that
    +    # the required methods are implemented with the required signatures, and
    +    # that these methods don't raise exceptions when called with sane
    +    # arguments.
    
    You're stating a fact, but why is it a good idea? I think those tests should have assertions, maybe using mocking if necessary. For example addExpectedFailure is completely broken right now, and no tests manage to show that.
  • The test methods docstrings are useless.

Thanks!

comment:15 Changed 12 years ago by robertc

So, I chatted to therve to get some clarity.

Specifically, he wants tests to prevent this bug:

+ def addExpectedFailure(self, test, failure, todo): + """ + Not implemented in some subunit versions. + """ + failure = util.excInfoOrFailureToExcInfo(failure) + addExpectedFailure = getattr(self._subunit, 'addExpectedFailure', None) + if addExpectedFailure is None: + self.addSuccess(test) + else: + addSuccess(self, test, failure)

(the last line should be addExpectedFailure).

I don't think that that is doable in the 'interface' tests, but additional specific tests for the subunit report can be added that will catch that, using appropriate doubles for different versions of subunit.

Another way to avoid that bug would be testtools.ExtendedToOriginalDecorator - less code existing is less code that can be wrong.

He also says 21:55 < therve> lifeless: right, but I think others too 21:55 < therve> for example, when util.excInfoOrFailureToExcInfo is called

but I don't really know what that means.

comment:16 Changed 12 years ago by Jonathan Lange

Keywords: review added
Owner: changed from Jonathan Lange to therve

Thomas, thanks for the review. I've fixed the pyflakes errors, together with an added test.

I'm having difficulty addressing your other comments though.

I had thought that the comment I added explained why the tests don't have assertions. I don't know how I can expand the comment to add more justification, and I'm honestly not sure how to rewrite the tests to have meaningful assertions if they are to remain generic reporter interface tests. I'd very much appreciate it if you could make a concrete suggestion.

Your final comment says that the test docstrings are useless. I agree that some of them are, but I don't think that all of them are. In particular, the ones for test_startStop, test_addUnexpectedSuccess, test_addSuccess, test_addError, test_addFailure, test_addExpectedFailure, test_addUnexpectedSuccess_todo and test_addSkip could all be improved. However, I'm not exactly sure what they should say. My uncertainty is heightened by the fact the tests themselves are being called into question, and because I don't know what you think about the other test docstrings that I consider to be acceptable. Here also, I'd appreciate a concrete recommendation.

I look forward to hearing back from you.

jml

comment:17 Changed 12 years ago by Jean-Paul Calderone

Hi lifeless, jml,

I'm attaching a patch which I hope will clarify what kind of tests are being requested. The patch removes a portion of the subunit reporter implementation without modifying the unit tests in any way. All of the unit tests still pass. Any part of the subunit reporter implementation which should not actually be removed from this branch should have unit tests added which verify its behavior. Ideally, these things would be re-added in a test-driven manner.

I did try to delete all of the untested implementation, but I may have missed some, so it wouldn't hurt to double check.

Changed 12 years ago by Jean-Paul Calderone

Attachment: simpler-subunit.patch added

Patch against the branch which highlights all untested code by deleting it

comment:18 Changed 12 years ago by Jean-Paul Calderone

Cc: Jean-Paul Calderone added
Keywords: review removed
Owner: changed from therve to Jonathan Lange

I should also mention that I think it's fine to have "interface tests" (but I'll re-iterate that verifyObject/verifyClass is a more concise way to write these, despite lifeless's disagreement above). They're just not sufficient. The implementation actually needs to be unit tested as well.

I also still think that proxyForInterface could be handy here, but I'll leave that choice up to someone else.

comment:19 Changed 12 years ago by Jonathan Lange

OK, thanks for clarifying. I'll rewrite the tests in this branch accordingly, perhaps after work today.

comment:20 Changed 12 years ago by Jonathan Lange

(In [27710]) Comprehensive, direct test coverage of SubunitReporter.

refs #4004

comment:21 Changed 12 years ago by Jonathan Lange

Keywords: review added
Owner: Jonathan Lange deleted

I think I've got full test coverage, and the tests without assertions are gone. I haven't edited any of the docstrings for their own sake.

comment:22 Changed 12 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Jonathan Lange

Cool! Some minor issues:

  1. Several docstrings use backticks where L{} would be more useful
  2. The comment before TestReporterInterface.setUp still says most tests don't have assertions. They do now, though.
  3. Is TestReporterInterface still the right name for this test case?
  4. You recently lamented test methods with multiple assertions. I'm not seriously bothered by the multiple assertions in the test methods of TestReporterInterface, but I thought I should point it out, lest you have something new to regret later. :)
  5. We're trying to switch entirely to assertEquals. Can you adjust the newly tests which are using assertEqual instead?
  6. Some tests deal with addExpectedFailure. subunit trunk doesn't seem to have this function. If not that version, what version does?
  7. Some test methods, eg test_addExpectedFailure_passed_through, aren't named according to the naming convention.
  8. The user experience for subunit not being installed isn't super awesome. Can you file a ticket for adding a way for a reporter plugin to explicitly say that it's not usable, and for having the command line trial script present this fact to the user nicely?
  9. excInfoOrFailureToExcInfo docstring is somewhat vague on its return type. Doesn't seem like much of a win to add this as a new public api, but as long as we are, let's at least make it clear what it does.

comment:23 Changed 12 years ago by Jonathan Lange

(In [27741]) * Use L{} instead of backticks

  • Remove obsolete comment
  • Split up tests in TestReporterInterface
  • Use assertEquals rather than assertEqual

This should have been four commits.

refs #4004

comment:24 Changed 12 years ago by Jonathan Lange

Keywords: review added
Owner: Jonathan Lange deleted
  1. Backticks changed to L{}. I could only find backticks in test_reporter, so that's all I changed.
  2. Comment removed.
  3. I can't think of a better name. Can you?
  4. I've exploded these tests. I think assertions in multiple phases of the test are a bad smell. This smell wasn't particularly pungent, but I felt like getting rid of it.
  5. Changed all the assertEqual I could find to use assertEquals.
  6. addExpectedFailure is being introduced very shortly to subunit trunk -- it's necessary to support Python 2.7 correctly.
  7. I filed ticket #4164.
  8. I've expanded the docstring to include a @param and @return section. Hope this is enough.

comment:25 Changed 12 years ago by Jonathan Lange

Ugh. 7 => 8; 8 => 9.

  1. Fixed tests to match naming convention. test_someMethod_does_a_thing to test_someMethodDoesAThing.

comment:26 Changed 12 years ago by Jonathan Lange

addExpectedFailure is now in subunit trunk.

comment:27 Changed 12 years ago by robertc

addExpectedFailure is in Subunit 0.0.3, which I released yesterday.

comment:28 Changed 12 years ago by Jean-Paul Calderone

Owner: set to Jean-Paul Calderone
Status: newassigned

comment:29 Changed 12 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: changed from Jean-Paul Calderone to Jonathan Lange
Status: assignednew

A few more simple things:

  1. TestSubunitReporter.test_oldSubunitInstalled still has a tiny problem. Trying to delete a class attribute (such as a method) from an instance will always raise an AttributeError. I think you'll have to modify the class directly to test this. (del probably works just as well as delattr, in any case.)
  2. test_addExpectedFailure_passed_through still misnamed
  3. The SubunitReporter class docstring should cover the instance attributes.
  4. SubunitReporter also has a lot of methods with no docstrings. At least a couple of them also do a little bit more than just forward on the call to subunit.
  5. One missing blank line in test_plugins.py between getPluginsByLongOption and test_subunitPlugin. Also, some more assertEqual uses here.
  6. One build slave appears to actually have some version of subunit installed. Unfortunately it must be a pretty old version, because it fails twisted.trial.test.test_reporter.TestSubunitReporter.test_addSkipSendsSubunitAddSkip with exceptions.AttributeError: 'TestProtocolClient' object has no attribute 'addSkip'. Perhaps this method needs a special case as well?

I'm not sure why I was objecting to the name TestReporterInterface before. It seems fine to me now.

Branch needs a news file too, of course. Overall though, looks good. Hopefully this will be the end of my constructive criticism. :)

comment:30 Changed 12 years ago by Jonathan Lange

Keywords: review added
Owner: Jonathan Lange deleted

comment:31 Changed 12 years ago by radix

Owner: set to Jonathan Lange
  1. SubunitReporter.shouldStop and stop don't have documentation. I'm not sure how pydoctor deals with docstrings in property-methods, so you may need to document shouldStop as an @ivar in the class docstring.
  1. addSkip's docstring is just explaining the interface. The implementation is more interesting than that. It's worth knowing that sometimes it'll be reported as a success instead of a skip.
  1. The new paragraph in INSTALL is kind of confusing. It doesn't mention trial.
  1. I don't understand what the intent of this branch is. I think Robert deserves a punch in the arm for writing such a bad ticket summary and description. I'd really appreciate it if someone could fix write a better one.

How do I use this? Okay, I can run trial with the subunit reporter, but what does that mean? I don't know how to actually take advantage of multiple processes in my trial-based unit tests.

By the way, your last submission for review didn't include a response to the points in the previous review. I could have sworn we had something in ReviewProcess that recommended doing that, but maybe I deleted it when I reorganized it last. It's good to be confident that all previous reviews have been addressed in *some* way, whether they've been implemented as directed or whether the author disagrees with the comment.

comment:32 Changed 12 years ago by radix

Keywords: review removed

comment:33 Changed 12 years ago by Jonathan Lange

Description: modified (diff)
Summary: subunit reporter. FTW.Add a subunit reporter to Trial

comment:34 Changed 12 years ago by Jonathan Lange

Commenting on my last submission:

  1. TestSubunitReporter.test_oldSubunitInstalled was changed to delete the class attribute. I also added a new test to deal with point 6, so I renamed test_oldSubunitInstalled.
  2. test_addExpectedFailure_passed_through now renamed to test_addExpectedFailurePassedThrough
  3. The SubunitReporter class docstring now covers the (private!!!) instance attributes.
  4. I've added a bunch of blank lines and changed every assertEqual I could find to assertEquals.
  5. I added special support for missing addSkip.

comment:35 Changed 12 years ago by Jonathan Lange

Also, I added docstrings to all of the methods without docstrings that I could find, even though I missed shouldStop.

comment:36 Changed 12 years ago by Jonathan Lange

Keywords: review added
  1. Documentation added for shouldStop and stop. I've only added a docstring to the property and haven't tested it in pydoctor.
  2. I've updated the addSkip docstring.
  3. I've changed INSTALL to refer to trial. It seems pretty clear to me.
  4. I've updated the ticket. The intent, to me, is clear. It adds a reporter that prints out subunit output. It adds no functionality to trial for subprocess support, it just adds a new reporter that users of trial have been asking for.

You use this with trial --subunit twisted. You can benefit from it by piping it through some of the existing subunit filters. You can implement distributed / parallelized testing by generating a list of tests, trial --subunit twisted | subunit-ls, splitting the list into N parts, then running each sublist in a separate subprocess.

comment:37 Changed 12 years ago by Jonathan Lange

Owner: Jonathan Lange deleted

comment:38 Changed 12 years ago by Jonathan Lange

trial --reporter=subunit, rather.

comment:39 Changed 12 years ago by Jonathan Lange

(In [27873]) Updated copyright headers and a since 9.0 marker. refs #4004.

comment:40 Changed 12 years ago by radix

Keywords: review removed
Owner: set to Jonathan Lange

Ok, I understand this better. I didn't realize there were any other standalone utilities that already parsed the subunit protocol.

Oh. I should have mentioned this on IRC: you shouldn't be @sincing 9.0, you should be @sincing 10.0, since 9.0 is already out and 10.0 is going to be the next version. Sorry :-)

I don't think there's anything else. +1!

comment:41 Changed 12 years ago by Jonathan Lange

Resolution: fixed
Status: newclosed

(In [27875]) Merge subunit-4004: Add a subunit reporter to trial.

  • Authors: jml, robertc
  • Reviewers: exarkun, therve, thijs, radix
  • Fixes #4004

You can now run tests with trial --reporter=subunit and have Trial generate subunit output. This output can then be used with other tools that parse the subunit protocol.

comment:42 Changed 12 years ago by Jean-Paul Calderone

This was a duplicate of #2163.

comment:43 Changed 11 years ago by <automation>

Owner: Jonathan Lange deleted
Note: See TracTickets for help on using tickets.