Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

doctests in TestCase classes #4

Merged
merged 10 commits into from Jul 15, 2015
Merged

doctests in TestCase classes #4

merged 10 commits into from Jul 15, 2015

Conversation

jimfulton
Copy link
Member

Added support for creating doctests as methods of
unittest.TestCase classes so that they can found automatically
by test runners, like nose that ignore test suites.

Jim Fulton added 2 commits July 12, 2015 13:49
  unittest.TestCase classes so that they can found automatically
  by test runners, like *nose* that ignore test suites.
r"""Doctests in TestCase classes

The original doctest unittest integration was based on unittest test
suites, which have falled out of favor. This module provides a way to
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've always seen it spelled "have fallen out of favor". Typo?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:) typo fixed

@mgedmin
Copy link
Member

mgedmin commented Jul 13, 2015

nose has --with-doctest and --doctest-tests. Granted, that has some disadvantages (e.g. you cannot specify custom checkers or optionflags), so there's room for other solutions.

I have two reservations for this PR:

  • the camel-casing of the @DocText decorator seems weird to me
  • ignoring the module globals for doctests defined in docstrings feels alien (and makes it harder to share import statements and stubs defined in the test module)

@jimfulton
Copy link
Member Author

The biggest problems I have wrt current nose support is the poor support for tst-specific test fixtures, which I find unusable.

WRT naming, I'm open to other suggestions. I was originally going to use doctest, but then that would have been awkward when importing the standard doctest module to get at option flags and othe rmodule contents. doc_test would look weird too. At least camel-case make sense because this is a constructor.

WRT module globals, I have mixed feelings about this. Would you expose module globals for test files too? I assume not.

A bigger question I had was whether to include test-case instance dictionary data in the doctest globals. Eventually decided it would be better to expose a test object, which would give access to test case methods.

@jimfulton
Copy link
Member Author

Oh, and another missing feature is the ability to declare execution of doctests from a module.

I'll add that in a later PR. I'm kinda anxious to get the PR done cuz U need it for work. :)

@jimfulton
Copy link
Member Author

wrt naming: doctestcase?

@jimfulton
Copy link
Member Author

I think having access to module globals makes more sense for traditional doctests that exist in non-test modules.

I'm inclined, for now, to not include module globals, which are somewhat less likely to be interesting in test modules.

It's interesting to note that nose bothers not to look in test modules for doctests, by default.

@mgedmin
Copy link
Member

mgedmin commented Jul 13, 2015

As for naming, yeah, the most sense-making choice (@doctest) conflicts with using doctest.ELLIPSIS etc. Good point there.

I can get used to @DocTest. I've been using @Lazy for a long time without complaining.

As for module globals: right, sharing globals with separate files feels Wrong, just like not letting doctests in docstring access globally defined names feels Wrong to me. I'd prefer consistency with the stdlib doctest module.

I'm tempted to ask for an option @DocTest(globals=True), but that's a bad way to design APIs, so I won't ;).

@leorochael
Copy link

I mildly dislike the DWIMing of the first argument acting as either a filename or the test itself, but I can't convince myself that using differently named decorators or different named parameters would make it any better, so I won't complain.

WRT globals, I do think that sharing globals into inlined doctests is important (both the 1st argument tests and the decorated method docstrings), mainly because if I'm mixing doctests with regular test methods in the same test file I'd find it annoying to have to reimport modules that were imported globally only inside the inlined doctests and not in the test methods.

The external doctests I agree shouldn't share globals with the test module.

(I guess a case could be made here that the "test" global should be called (or aliased) "self" in the inlined tests)

WRT naming, @doctestcase seems wrong since the decorator creates a test (method), not a test-case (which is a class with several tests, a setup and a teardown). I'd also prefer a lowercased name, but anything I can think of would be weirder or less convenient than @DocTest.

All in all, I really like this PR for allowing doctests being able to reuse TestCase setup/teardown and being locatable by nose!

@jimfulton
Copy link
Member Author

After reading Leonardo's comments, separate function names is growing on me:

docteststring
doctestfile
doctestmethod

doctestmethod and docteststring would inject module globals.

@jimfulton
Copy link
Member Author

I prefer test for referring to the test. Having 'self' at the top level of doctest code seems pretty weird.

Also, if a test defines a class, the self in the example class would hide access to the test.

But perhaps this could work differently for doctestfile vs docteststring and doctestmethod.

function. The decorated function is run before the test is executed.
This provides a way to provide some test-specfic setup, if desired.

Note that the docstring, if any, of the decorated function is ignored.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docstring is obviously not ignored in the "naked decorator" case. So this sentence, in isolation, is slightly confusing.

@leorochael
Copy link

In my mind, self in the doctest doesn't look so weird when it's, for instance, the docstring of the test method where the developer just did self.something = something_else to prepare for the test. But I agree it only makes sense in the docteststring/method cases.

And the hiding of self would be undesirable indeed, so if it's there it should be an alias to test.

Again, this is minor IMO, and maybe not worth the extra effort.

But speaking of globals, it would be extra nice (perhaps in another pull request) if there was a way of passing new top-level names into the doctest from the decorated function.

@jimfulton
Copy link
Member Author

I've rewritten this based on comments:

  • Now have separate constructors, doctestmethod, docteststring, and doctestfile, with aliases method,
    string, and file.
  • doctestmethods and docteststrings have access to module globals and access the test case
    instance as self.
  • If a test case defines a globs attribute, it must be a dictionary and it's contents are added to test globs.

@jimfulton
Copy link
Member Author

BTW, I plan to submit a Python PR after we've used this in zope.testing for a while.

@jimfulton
Copy link
Member Author

What do y'all think? :)

'''
self.x = 3

@doctestcase.doctestfile('test4.txt')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need two different decorators @doctestcase.file and @doctestcase.doctestfile?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, one's an alias for the other. Never mind.

instance is available as the ``test`` variable.

- If a test case defines a globs attribute, it must be a dictionary
and it's contents are added to the test globals.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: it's -> its.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This and other typos fixed.

@mgedmin
Copy link
Member

mgedmin commented Jul 15, 2015

Looks good to me (other than some typos)!

I didn't mention that some docstings ought to have a period at the end of the first sentence (as per PEP-257), since I didn't want to appear too nitpickery.

@jimfulton
Copy link
Member Author

Thanks for the comments! I've addresses all of them.

@leorochael
Copy link

Didn't do as thorough a review as @mgedmin, but looks nice.

The independent functions are nicer and more readable than before.

jimfulton added a commit that referenced this pull request Jul 15, 2015
@jimfulton jimfulton merged commit 3d93ea5 into master Jul 15, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants