Update testing documentation to reflect plugin changes by drdavella · Pull Request #6811 · astropy/astropy

drdavella · 2017-11-06T18:13:47Z

This PR contains documentation updates that correspond to the changes that have been introduced with #6606. The changes have been submitted as a separate PR in order to make them easier to review, but they should be merged at the same time as #6606.

astropy-bot · 2017-11-06T18:13:49Z

Hi there @drdavella 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labeled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃.

Everything looks good from my point of view! 👍

If there are any issues with this message, please report them here.

drdavella · 2017-11-06T18:16:35Z

+
+The test suite can be run directly from the native ``pytest`` command. In this
+case, it is important for developers to be aware that they must manually
+rebuild any extensions by running ``setup.py build`` before testing.


This should be setup.py build_ext.

bsipocz · 2017-11-07T12:09:56Z

+As of Astropy 3.0, the dependencies used by the Astropy test runner are
+provided by a separate package called ``pytest-astropy``. This package provides
+the ``pytest`` dependency itself, in addition to several ``pytest`` plugins
+that are used by Astropy, and will also be of general use to other packages.


I would say managed rather than used, but if the plan is used, then I suggest to add pytest-mpl to pytest-astropy, too as astropy uses it.

Hmm, that's a good thought. Does @astrofrog think that pytest-mpl should be added to pytest-astropy as well? The main benefit would be a slight simplification to the CI scripts.

bsipocz · 2017-11-07T12:10:59Z

+Developers who want to run the test suite will need to install the testing
+package using pip::
+
+    > pip install pytest-astropy


do we plan to conda package them, or pip will always be the preferred way of installation?

It seems like a good idea to eventually distribute this through conda. We can leave the documentation as-is for now and update later if appropriate.

sure, this was more like a question than a review item to be addressed here.

bsipocz · 2017-11-07T12:25:15Z

+    # Note, this is retained for backwards compatibility but the remote_data
+    # decorator is now provided by the pytest-remotedata plugin, and does not
+    # need to be imported from astropy.
    from ...tests.helper import remote_data


I would totally remove this from the docs. We provide the backward compatibility, but the narrative docs should only contain the recommended way of usage.

bsipocz

Overall looks good, I only had a few inline comments.

mhvk · 2017-11-07T14:47:20Z

+
+Below is an outline of the required changes:
+
+* It is no longer necessary to import ``remote_data`` from ``astropy.tests.helper``


This is confusing for affiliated packages. Write explicitly what one should do to ensure one can mark @remote_data and get no errors upon execution (from flake8 if nothing else)

Thanks for the feedback. Hopefully this is clearer now.

Yes, now it is very clear! Thanks.

pllim

Currently, this seems the only place where the new standalone plugins' usage is documented. That is not ideal for people who want to use these new plugins but have no interest in astropy itself. Are there any plans to move the usage doc here out to the new packages? Then, we can simply link to those docs from here.

I'm a bit late to the code review part, but when I opened up the new display.py and config.py, I see complains about "unused imports" (are they imported but unused on purpose?).

Also see some individual comments below.

Thank you for all the hard work!

pllim · 2017-11-07T20:34:30Z

+
+Since the testing dependencies are not actually required to install or use
+Astropy, they are not included in ``install_requires`` in ``setup.py``.
+However, for technical reasons it is not currently possible to express these


Is there an issue or PR that explains this technical reason? If so, is it overkill if we link that here? This is because, "for technical reasons" is too vague if a future developer that is not Dan tries to address this.

There is not currently an issue. Basically it's inherent in the design of the test runner, which is one motivation for getting rid of it if possible.

This issue doesn't exactly explain the problem, but is relevant: #6787

pllim · 2017-11-07T20:38:53Z

+The plugin also adds the ``--remote-data`` option to the ``pytest`` command
+(which is also made available through the Astropy test runner).
+
+The default behavior is to skip tests that are marked with ``remote_data``. If


The second sentence seems to be repeating the first sentence but with more details. Is the first sentence even necessary?

pllim · 2017-11-07T21:13:30Z

+
+Providing either the ``--remote-data`` option, or ``--remote-data=any``, will
+cause all tests marked with ``remote_data`` to be executed. Any tests that are
+marked with ``internet_off`` will be skipped.


I never knew --remote-data skips internet_off -- Has this always been the case? I don't see why they have to be mutually exclusive. Just because I want one test to access internet doesn't mean I want to skip another test that does not need internet. But then again, I never used internet_off option myself...

The internet_off decorator was introduced by the latest changes. However, I agree that the current implementation is not the most useful. There's an issue for this now: astropy/pytest-remotedata#17

pllim · 2017-11-07T21:13:57Z

+cause all tests marked with ``remote_data`` to be executed. Any tests that are
+marked with ``internet_off`` will be skipped.
+
+Running the tests with ``--remote-data=astropy`` will cause only tests that


Does this also skip internet_off like --remote-data=any?

Yes, I'll add a comment here.

pllim · 2017-11-07T21:18:11Z

+pytest-doctestplus
+==================
+
+The `pytest-doctestplus`_ plugin provides advanced doctest features, including:


Nitpick: Do we need to use double-backticks around doctest to render it as doctest since "doctest" is not a real word? I'll leave it to the linguist to decide.

Hmm, personally I think it's common enough term in python circles to not need backticks 😄

pllim · 2017-11-07T21:31:25Z

+pytest-openfiles
+================
+
+The `pytest-openfiles`_ plugin allows for the detection of open IO resources at


Nitpick: "IO" -> "I/O"?

pllim · 2017-11-07T21:36:16Z

+
+The test suite can be run directly from the native ``pytest`` command. In this
+case, it is important for developers to be aware that they must manually
+rebuild any extensions by running ``setup.py build_ext`` before testing.


I think setup.py test automatically runs doctest as well (does it still?), but that won't be the case here, right? We need to document any behavior difference, if applicable.

Yes, good point. It also supplies --doctest-rst by default.

pllim · 2017-11-07T21:39:27Z

 API, please see the :ref:`changelog`.
+
+
+Renamed/removed functionality


Nitpick: Should "removed" be capitalized by grammatical rules?

No, I think this is fine.

[edit]
It could really probably go either way, but I just copied it from the docs/whatsnew/2.0.rst

If there is a precedent, then it is okay. :)

pllim · 2017-11-07T21:40:52Z

+pytest plugins
+**************
+
+The following pytest plugins were previously provided as part of the Astropy


In the diff above, there are double-backticks around pytest to render it as pytest; Should we be consistent here?

pllim · 2017-11-07T21:47:38Z

+from ``astropy.tests.pytest_plugins`` will need to make updates, although
+backwards compatibility will be maintained in the meantime.
+
+Below is an outline of the required changes:


This list is very valuable and should be move up to the actual testing doc, rather than be buried in "what's new". Instead what's new here can link to the relevant testing doc section for this.

I actually disagree with this: I think it's not particularly relevant to most readers of the testing documentation, but it is important for affiliated package developers who need to know what changed in this Astropy release. But I can be persuaded otherwise if everyone else feels strongly about it.

In my past experience with config refactoring back in 0.4, I never looked in What's New because I already knew what was new. Instead, I went straight to astropy.config section for the transition doc. But let's wait a bit for inputs from others.

I would suggest to keep it; for my own affiliated package, I can use any help I can get in finding what things change!

drdavella · 2017-11-07T22:52:19Z

@pllim, to address your general comment about plugin documentation, I'm in the process of improving the READMEs for each for the new plugin packages. I don't think we'll be publishing those on readthedocs, but we already link to the github repos where the READMEs will be available.

pllim · 2017-11-08T21:08:48Z

These are the remaining issues brought up in this PR but out of scope. Please open new issues for them as needed:

Include pytest-mpl in pytest-astropy?
Distribute pytest-astropy via conda?
Get rid of test runner due to "technical reason"?
Update this doc when internet_off is fixed on the plugin side.
Automatically enable FLOAT_CMP?

bsipocz · 2017-12-05T18:42:45Z

Now that the plugins will be backwards compatible with the 2.0.x branch (once #6918 is merged), I wonder whether we can backport this PR, too?

drdavella added Docs testing labels Nov 6, 2017

drdavella added this to the v3.0.0 milestone Nov 6, 2017

drdavella requested review from astrofrog, bsipocz and pllim November 6, 2017 18:13

drdavella added the no-changelog-entry-needed label Nov 6, 2017

drdavella commented Nov 6, 2017

View reviewed changes

drdavella force-pushed the update-plugins-docs branch from c1b8c64 to e5fc0c1 Compare November 6, 2017 18:39

drdavella mentioned this pull request Nov 6, 2017

Relocate generic pytest plugins to separate repositories #6606

Merged

bsipocz reviewed Nov 7, 2017

View reviewed changes

bsipocz approved these changes Nov 7, 2017

View reviewed changes

mhvk reviewed Nov 7, 2017

View reviewed changes

drdavella added 5 commits November 7, 2017 11:57

Update testing documentation to reflect new plugins [docs only]

912dac5

Update testing documentation on install page [docs only]

ede0496

Update whatsnew for new pytest plugin packages [docs only]

724eabe

Remove reference to remote_data in test helpers [docs only]

c47c2ec

Address code review feedback, other improvements [docs only]

cfa198e

drdavella force-pushed the update-plugins-docs branch from 623255a to cfa198e Compare November 7, 2017 17:07

pllim reviewed Nov 7, 2017

View reviewed changes

Apply more feedback from code review [docs only]

17a381b

drdavella force-pushed the update-plugins-docs branch from 6690da6 to 17a381b Compare November 7, 2017 22:58

pllim merged commit 169afbb into astropy:master Nov 8, 2017

drdavella mentioned this pull request Nov 9, 2017

Long-term testing improvements and future directions #6451

Closed

drdavella mentioned this pull request Dec 5, 2017

Backport new plugin documentation to v2.0.x #6931

Merged

bsipocz modified the milestones: v3.0.0, v2.0.3 Dec 5, 2017

bsipocz added the Manual Backport label Dec 5, 2017

bsipocz modified the milestones: v2.0.3, v3.0.0 Dec 7, 2017

bsipocz removed the Manual Backport label Dec 7, 2017


		Below is an outline of the required changes:

		* It is no longer necessary to import ``remote_data`` from ``astropy.tests.helper``

		API, please see the :ref:`changelog`.


		Renamed/removed functionality

Conversation

drdavella commented Nov 6, 2017

Uh oh!

astropy-bot Bot commented Nov 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bsipocz Nov 7, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

bsipocz 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!

pllim 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!

drdavella Nov 7, 2017 • 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!

drdavella Nov 7, 2017 • 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!

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!

drdavella Nov 7, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

astropy-bot Bot commented Nov 6, 2017 •

edited

Loading

bsipocz Nov 7, 2017 •

edited

Loading

drdavella Nov 7, 2017 •

edited

Loading

drdavella Nov 7, 2017 •

edited

Loading

drdavella Nov 7, 2017 •

edited

Loading

bsipocz commented Dec 5, 2017 •

edited

Loading