Enable Easy TestRunner Subclassing by Cadair · Pull Request #5505 · astropy/astropy

Cadair · 2016-11-25T13:47:18Z

This PR is designed to make it simple for affiliated packages to add or remove arguments from the astropy test runner based on custom needs not shared by astropy core.

The way this works is by separating each kwarg out into a separate method which can be overloaded by a subclass or new methods added. The __new__ constructor then looks for all the methods defined on the class which have been decorated with the keyword decorator and adds them to a dictionary which forms the allowed keyword arguments and their default values for the run_tests method. Documentation is also delegated to the keyword methods.

Removal of keyword arguments is supported by overloading the method and returning NotImplemented.

One (unintended) side effect of this is that the run_tests method is now keyword argument only.

Finally, I would be interested in extending this to automatically generate the setup.py command, that requires that command importing the TestRunner class, I don't know if that is going to break the setup.py machinery in any way?

ping @wkerzendorf @astrofrog

Cadair · 2016-11-25T21:23:33Z

This still needs tests and some better developer documentation, but before I put that work in I would like some buy-in on the idea. Are people generally supportive of this somewhat crazy suggestion?!

pllim · 2016-11-27T15:09:14Z

Is this backward compatible? How would this affect external customization like astropy/package-template#176 ?

Cadair · 2016-11-27T19:59:08Z

The tests pass, so I am hoping that means it's backwards compatible.

What's in astropy/package-template#176 is a pytest hook right, so it would not affect the run_tests() method, right? I also don't think that it should affect that kind of hooks in anyway, as this layer calls out to pytest.

astrofrog

This is a great idea, and it would be nice to get this into 1.3. I left a few small comments - my only other comment is that if you would make use of this in SunPy or in another package, I would recommend trying to do a PR to that package now just to make sure that it will suit your needs (in case you find any fundamental design flaws with this).

astrofrog · 2016-11-29T08:59:20Z

 from ..utils.exceptions import AstropyWarning

+class keyword(object):
+    def __init__(self, default_value, priority=0):


Can you make the spacing here PEP8-compliant, i.e. one more space above and below class keyword?

Also I think it would be good to maybe add a comment or a docstring somewhere here explaining what this is for and what the priority does.

astrofrog · 2016-11-29T09:01:24Z

+        keyword._default_value = self.default_value
+        keyword._priority = self.priority
+        keyword.__doc__ = f.__doc__
+        return keyword


To make a decorator that preserves call signature and docstring, try instead using the wraps function from astropy.utils (rather than explicitly copying __doc__).

wraps will not work because I explicitly use the function name below.

astrofrog · 2016-11-29T09:02:50Z

+        package : str, optional
+            The name of a specific package to test, e.g. 'io.fits' or 'utils'.
+            If nothing is specified all default Astropy tests are run.
+


What about not putting extraneous spaces here and then in __new__ adding spaces if needed between parameters?

isn't it good form to end a docstring with a blank line anyway?

I never do... It looks weird to me. What is it with style guides and docstrings? Why not just:

""" This is a docstring """

How does:

""" This is a docstring """

make any sense? 😛

Anyway, up to you - what I mean is more that if you don't assume that sub-classes include the whitespace, you can strip whitespace and add it back so that the output is always predictable in the generated docstring?

astrofrog · 2016-11-29T09:03:28Z

-        all_args = []
+        return []

+    @keyword(None, priority=1)


When the priority is >0, it would be good to add a comment above as to why that is, and what it depends on.

astrofrog · 2016-11-29T09:04:04Z

+
+        return []

+    @keyword(None)


Can't you make the default be None and just do @keyword in those cases?

astrofrog · 2016-11-29T09:04:44Z

+        """
+        if args:
+            return shlex.split(args,
+                               posix=not sys.platform.startswith('win'))


PEP8 gone mad - just put it one one line? :)

Cadair · 2016-11-29T09:57:19Z

+        """
+
+        # Don't import pytest until it's actually needed to run the tests
+        from .helper import pytest


Can we put this somewhere else in this file, so we don't enforce the use of vendored pytest? (SunPy dosen't use it.)

Good question - I'm not sure why it had to be this way. Maybe just try and put it top-level?

astrofrog · 2016-12-02T22:10:29Z

Hey @Cadair I rebased your PR. One interesting side-effect is that your commits are no longer verified. Oopsie.

eteq · 2016-12-03T07:12:20Z

@Cadair - this is looking great to me - If you think its done I think its basically there.

One thing that's missing , though: documentation. Can you add something to the testing part of the docs that shows how someone might add this extra one?

This actually breaks the whole point of this exercise, because it makes this class astropy specific.

If you don't want the docstring included subclass and don't set a docstring.

astrofrog · 2016-12-05T20:44:26Z

+        """
+        pep8 : bool, optional
+            Turn on PEP8 checking via the pytest-pep8 plugin and disable normal
+            tests. Same as specifying `--pep8 -k pep8` in `args`.


Here and elsewhere, you need double backticks, otherwise the Sphinx build fails looking for references

eteq

@Cadair - a few teaks required to make the docs finish without warnings (including @astrofrog's comment above), and a couple quick additions to the docs, but then I think this is good!

eteq · 2016-12-05T23:04:03Z

    above.

+Astropy Test Function
+#####################


This needs to be ^ instead of #, because Sphinx needs to have consistent section headings or else it spurts out all kinds of warnings.

eteq · 2016-12-05T23:04:20Z

-Astropy Testing helpers (`astropy.tests.helper`)
-************************************************
+***************
+Astropy Testing


Maybe "Astropy Testing Tools" or "Astropy Testing Machinery"? That would make it clearer that this is the machinery, while the guidelines are below.

Along the same lines, perhaps under this title (but before the helper module heading), you should add a sentence with a link to the testing guidelines. Something like This section is primarily a reference for developers that want to understand or add to the Astropy testing machinery. See :doc:/development/testguide for an overview of running or writing the tests.

eteq · 2016-12-05T23:04:40Z

+Astropy Test Runner
+===================
+
+When executing tests with either `astropy.test` or ``python setup.py test`` the call to pytest is controlled by the `astropy.tests.runner.TestRunner` class.


Can you add a few sentences explaining the changes in this PR and maybe giving a very simple example of how to write your own test keyword? (even if it just produces "hello world").

I put an example of writing a keyword in the docstring for TestRunnerBase, but I will add something here as well.

Cadair · 2016-12-06T13:26:21Z

@eteq all sorted I hope.

eteq · 2016-12-06T21:10:16Z

@Cadair - had to make one change in Cadair/astropy@d7d3391 - you were missing backticks around a link, but the rest looks good. Merging!

astrobot · 2016-12-06T21:10:26Z

@eteq - thanks for merging this! However, I noticed the following issue with this pull request:

Changelog entry not present (or pull request number missing) and neither the Affects-dev nor the no-changelog-entry-needed label are set

Would it be possible to fix this? Thanks!

This is an experimental bot being written by @astrofrog - let me know if the message above is incorrect!

Cadair changed the title ~~[WIP] Enable TestRunner subclassing~~ [WIP] Enable Easy TestRunner Subclassing Nov 25, 2016

Cadair force-pushed the runner_subclassing branch from 479f3eb to c6cc5f8 Compare November 25, 2016 14:48

pllim added testing Work in progress labels Nov 25, 2016

Cadair force-pushed the runner_subclassing branch 2 times, most recently from e450f2f to a2c4368 Compare November 25, 2016 21:11

Cadair changed the title ~~[WIP] Enable Easy TestRunner Subclassing~~ Enable Easy TestRunner Subclassing Nov 25, 2016

astrofrog requested changes Nov 29, 2016

View reviewed changes

Cadair force-pushed the runner_subclassing branch 2 times, most recently from 2c57c44 to c9e6c2b Compare November 29, 2016 09:55

Cadair commented Nov 29, 2016

View reviewed changes

astrofrog approved these changes Dec 1, 2016

View reviewed changes

Cadair referenced this pull request in chianti-atomic/ChiantiPy Dec 1, 2016

import astropy init to setup testing framework

1a45a1d

astrofrog added this to the v1.3.0 milestone Dec 2, 2016

astrofrog force-pushed the runner_subclassing branch from 71fe0a5 to 9f392c7 Compare December 2, 2016 22:09

Cadair force-pushed the runner_subclassing branch from 9f392c7 to a482523 Compare December 2, 2016 23:39

Cadair force-pushed the runner_subclassing branch from a482523 to 7bc7540 Compare December 5, 2016 18:40

Cadair added 8 commits December 5, 2016 18:42

Make keyword arguments to the TestRunner methods

a181d8d

import vendored pytest where it used to be.

f823b00

This actually breaks the whole point of this exercise, because it makes this class astropy specific.

don't need to explictly turn off docstring

4d9a1c4

If you don't want the docstring included subclass and don't set a docstring.

catch a couple of sneaky leftover non kwargs

1d8c527

I dislike Python 2

34335c7

Changes based on @astrofrog review

688469f

Write tests: break everything

250b625

Include test helpers in coverage report

0272433

Cadair and others added 4 commits December 5, 2016 18:44

Fix tests increase coverage

aec5065

exclude test_ files in astropy/tests from coverage

1026edc

Make remote_data return a list

16d01a4

Add docs to test runner and astropy.test

fb6e109

Cadair force-pushed the runner_subclassing branch from 7bc7540 to fb6e109 Compare December 5, 2016 18:45

astrofrog reviewed Dec 5, 2016

View reviewed changes

eteq requested changes Dec 5, 2016

View reviewed changes

Cadair force-pushed the runner_subclassing branch from 80cb8fe to 298df93 Compare December 6, 2016 12:18

Update docs

298df93

Add backticks around doc link

d7d3391

eteq approved these changes Dec 6, 2016

View reviewed changes

eteq merged commit 9564a9e into astropy:master Dec 6, 2016

pllim removed the Work in progress label Dec 6, 2016

Cadair deleted the runner_subclassing branch December 7, 2016 10:53

MSeifert04 mentioned this pull request Jan 25, 2017

Coverage testing is stopped since 1.2.2 release #5701

Closed

Cadair mentioned this pull request Feb 8, 2017

Fix disabling arguments in test runner subclasses. #5758

Merged

larrybradley mentioned this pull request Feb 24, 2017

Update development workflow doc #5838

Merged

Conversation

Cadair commented Nov 25, 2016

Uh oh!

Cadair commented Nov 25, 2016

Uh oh!

pllim commented Nov 27, 2016

Uh oh!

Cadair commented Nov 27, 2016

Uh oh!

astrofrog left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astrofrog commented Dec 2, 2016

Uh oh!

eteq commented Dec 3, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eteq left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Cadair commented Dec 6, 2016

Uh oh!

eteq commented Dec 6, 2016

Uh oh!

astrobot commented Dec 6, 2016

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

eteq left a comment •

edited

Loading