The great tox-ification #4105

stephenfin · 2017-10-02T20:48:14Z

Subject: Start making more use of tox in our testing infrastructure.

Feature or Bugfix

Refactor

Purpose

To paraphrase the Zen of Python (import this):

tox is one honking great idea -- let's use more of it!

Start modernizing the test infrastructure that's using make and other hand-crafted tools by replacing them with tox-based derivatives.

Detail

This is a multi-step process, and this PR covers the first four of these steps:

Moving the easy targets from the Makefile to tox, namely PyLint and coverage
Removing anything we don't use/care about from the Makefile (reindent?)
Moving the less easy targets into existing tox-based tooling (the check_fileheader test)
Switching Travis to use tox for everything

There's still some other stuff left to do, which will be done in a separate PR (this one is getting pretty big):

Rework how requirements are managed: there's duplication all over the place and it's really unnecessary
Update the CONTRIBUTING document to recommend the use of tox
Switch any non-Travis CIs to tox
Integrate codecov.io to make use of the coverage stats we moved to tox in step 1

Relates

N/A

It's 'py:mod', not 'py:module'. I don't know why this wasn't caught by Sphinx itself. Signed-off-by: Stephen Finucane <stephen@that.guru>

Shuffle things around in order of most importance. The 'py27' target is removed as its completely unnecessary. Signed-off-by: Stephen Finucane <stephen@that.guru>

This isn't enabled by default because it throws so many damn errors. Perhaps we should simply remove this? Signed-off-by: Stephen Finucane <stephen@that.guru>

There is value in examining coverage for something like Sphinx. Start doing this by default, in anticipation of increasing coverage over time. Signed-off-by: Stephen Finucane <stephen@that.guru>

Nobody seems to be using this, probably because of the 'flake8' target, and it hasn't been touched, some Python/flake8 updates aside in years. Just remove it. Signed-off-by: Stephen Finucane <stephen@that.guru>

We're going to be doing some surgery on this function, so first clean it up before we do anything else. Signed-off-by: Stephen Finucane <stephen@that.guru>

tk0miya

It seems this PR includes many changes. I can accept some, but I feel worries for some. So please separate to multiple PRs if possible.

Thanks,

tk0miya · 2017-10-03T16:31:38Z

.travis.yml

 env:
  global:
    - TEST='-v --durations 25'
    - PYTHONFAULTHANDLER=x
    - PYTHONWARNINGS=all
    - SKIP_LATEX_BUILD=1
-  matrix:
-    - DOCUTILS=0.13.1
-    - DOCUTILS=0.14


How do you go these matrix tests?

We get these and more via the du* tests in (du11, du12, du13, du14). These are run automatically via tox. If you check the Travis log, you'll see this

Where can I see the result of them? It seems no du* tasks are not executed.
https://travis-ci.org/sphinx-doc/sphinx/builds/282468735?utm_source=github_status&utm_medium=notification

Oh, damn - I was mistaken. Turns out you need to use Advanced Configuration to enable this. I've fixed this now.

@tk0miya I have now fixed the du* issue, along with ensuring the docs and mypy targets run. You can see the results here

Okay, I confirmed. But it seems testing time has been increased. both total time and throughput become long. Why?
Result for this: https://travis-ci.org/sphinx-doc/sphinx/builds/283292020
master branch: https://travis-ci.org/sphinx-doc/sphinx/builds/283254206

In addition, I noticed --durations 25 option for pytest has been disabled. To know which testcases are slow, I need to enable it.

tk0miya · 2017-10-03T16:32:40Z

Makefile

@@ -1,44 +1,9 @@
 PYTHON ?= python

-.PHONY: all style-check type-check clean clean-pyc clean-patchfiles clean-backupfiles \
-        clean-generated pylint reindent test covertest build


why removing these one? we are using some targets on release process manually.

This is a mistake. I'll fix this now.

EDIT: Wait - I'm removing four targets: pylint, covertest, style-check and reindent. The first three of these are now covered via tox targets while I considered the latter one, reindent useless because we're catching things like hard tabs with flake8 already. Did I miss something?

make style-check is used on our release-checklist. So please update utils/release-checklist also.

@shimizukawa do you use these commands locally? I don't use them locally, so I don't object to remove them, but I worry about somebody uses it.

@tk0miya I've updated the utils/release-checklist document now.
@shimizukawa Please note, as mentioned above, that all but one of the targets I've removed has a (superior) replacement option in tox, and only the useless reindent tool is removed. Hopefully if you do you use these, migrating will be possible

Note: we need to notify removing these targets to all developers. I don't know who uses them. The maintainers are not only shimizukawa.

tk0miya · 2017-10-03T16:36:19Z

sphinx/checks.py

@@ -0,0 +1,104 @@
+# -*- coding: utf-8 -*-


Why are you moving this to under sphinx package? This file will be installed to all users' environment. I feel very strange. Is that really needed?

I didn't think it mattered, to be honest. I know in OpenStack projects keep everything, tests and all, in the one project. Do I need to make changes here?

I just checked and it seems this file need to be in a module. I can either make utils a module, but that seems silly. Alternatively we can just distribute this. I don't imagine it would be any harm, to be honest.

Sure, I understand this does not harm anybody. But, personally, I don't like to export meaningless packages to users. I believe this is only used for maintenance.That means it does not bring worth to users. So I'd like not to include it to the release package.
Could you move this outside of package?

I can exclude it from the package via MANIFEST.in. Before I do this whoever, I should point out that we package both the tests and utils folders. If we are packaging these, is there any reason not to package this file?

Sorry, my words are wrong. It should be packaged as tarball, but not be installed as python package (sphinx.checks).

tk0miya · 2017-10-03T16:40:58Z

sphinx/util/smartypants.py

-    See the LICENSE file and the original docutils code for details.
-
+    :copyright: Copyright 2003-2010 by Günter Milde, John Gruber,
+                  Chad Miller.


I'm not good at license and copyright. Is this modification allowed?

Yup, I'm retaining the copyright and given that the license is the same as the main code, we don't need to explicitly call this out.

Could you explain for clarification which "main code" you are referring too?
And how does this change relate to the general purpose of the PR?

Could you explain for clarification which "main code" you are referring too?

I'm referring to the bulk of the Sphinx code: this file is licensed as BSD 2-Clause, and Sphinx itself is licensed as BSD 2-Clause. It's not possible for the latter to change without getting permission from every contributor to Sphinx, which means it's never going to happen. We're safe here.

And how does this change relate to the general purpose of the PR?

In this commit, I move a custom header check to a flake8 plugin. Previously we were able to skip this particular file from that check because it doesn't fit the standard format. However, we're not able to do this in flake8 without skipping the entire file or inserting a special check for this file in the plugin. Personally, I'd rather modify the header a small bit than do one of those two things.

Thanks for explanation. For the header check, you mean flake8 compliance entailed removing all lines starting at 505e148#diff-148ee4b7705bb41cbeae2ac61d69e1d5L14 (about 12 lines)? there was no way to amend them in some less drastic way? Perhaps indeed they are redundant. Regarding copyright, the copyright notice in its original formulation is the one of the full Docutils docutils/utils/smartquotes.py. The Sphinx smartypants.py is a small portion of this, which is only included to backport from Docutils 0.14 some features to Sphinx users with earlier Docutils. I think most if not all of this code was contribued by Günter Milde. Thus I am not sure about the way the copyright notice is set-up here. But the previous paragraph explains the situation to some extent.

I am a bit worried that flake8 normalization goes far here, but I am in no way knowledgeable about copyright, so I don't think this is blocking.

arrggh...I meant to write: I don't know enough to argue in anyway this could be blocking.

I've just gone and skipped the file. You're right - it is taking flake8 normalization a bit far

I only worried about merging three copyright entries to one. It seems like they are team, and worked until 2010.

I've reverted this change now and simply skipped the file. We can re-evaluate this later

stephenfin · 2017-10-03T20:21:24Z

It seems this PR includes many changes. I can accept some, but I feel worries for some. So please separate to multiple PRs if possible.

I can, but these PRs need to be submitted sequentially to prevent merge conflicts (they touch pretty much the same code). Any chance you could look at the individual commits instead?

If we want to check style, we run 'tox -e flake8': it shouldn't be necessary to run some obscure 'make' command too. Make this possible by moving the sole useful test from the target of this make command to a flake8 plugin. This includes a fix for a header that was previously excluded from checks, but is now included. Signed-off-by: Stephen Finucane <stephen@that.guru>

There are still a couple of checks here but all of them can be removed now: - Check if using valid Python syntax - Check if line length too long - Check if using 'x == None/True/False' - Check if using old HTML 3 tags The first three are already handled by default by the 'flake8' tool. The last one isn't replaced by anything, but it really isn't worth worrying about now because the tags it checks for have been dead for a really long time and would not be used by anyone writing HTML in the last 10 years. Combined, it means we can remove the entire file. The release-checklist document is update to reflect the removal of the 'style-check' target. Signed-off-by: Stephen Finucane <stephen@that.guru>

Less top-level files = winning [1]. [1] https://mypy.readthedocs.io/en/stable/config_file.html Signed-off-by: Stephen Finucane <stephen@that.guru>

While there are already some skips included here, they clearly aren't doing their job and the test fail locally. Resolve this. Signed-off-by: Stephen Finucane <stephen@that.guru>

This allows us to greatly simplify the '.travis.yml' file and eventually remove even more Makefile targets. This involves adding two new targets to the list of default targets run by tox - docs and mypy - and adding an addition 'travis' section per the 'tox-travis' docs [1] [1] https://tox-travis.readthedocs.io/en/stable/toxenv.html#advanced-configuration Signed-off-by: Stephen Finucane <stephen@that.guru>

stephenfin · 2017-10-04T17:24:34Z

OK, I've pushed some further changes (GitHub Pull Requests really don't work well with rebase):

Ensure the du* jobs run in Travis with the tox-travis plugin
Keep the same copyright header in sphinx/util/smartypants.py and simply exclude the file instead
Move sphinx/checks.py to extras/checks.py so we don't install this
Update the release-checklist document to remove references to make style-check

I note that the CONTRIBUTING.rst document also needs to be updated. I have done this on a separate branch and will submit a PR once this one is finished with.

@tk0miya Anything else needed?

shimizukawa · 2017-10-04T22:29:04Z

-1: This PR has multiple purposes and can not be validated. Please make individual PR.

tk0miya · 2017-10-05T02:30:28Z

utils/release-checklist

@@ -3,7 +3,7 @@ Release checklist

 * open https://travis-ci.org/sphinx-doc/sphinx/branches and check stable branch is green
 * Check `git status`
-* Run `make style-check`
+* Run `tox`, which will run style checks, type checks, unit tests and doc tests


Really? Did you mean our release manager should run all tests on local before release?
All tests takes very long time (see Travis, it takes a hour!).

stephenfin · 2017-10-05T08:12:29Z

-1: This PR has multiple purposes and can not be validated. Please make individual PR.

Sigh. I blame the GitHub Pull Request model entirely for this: it completely abstracts the idea of a commit and thus both encourages poor practices (like not writing well formatted commit messages, adding extra commits to fix previously broken ones) and discourages good ones (one change per commit, using rebase and squashing fixes into previous patches). Torvalds called it right on this one.

However 😄, I can split this into multiple patches. They should be incoming shortly.

stephenfin · 2017-10-05T08:49:50Z

Splitting into multiple PRs. Sigh...

stephenfin · 2017-10-05T09:22:20Z

This is done now in #4117, #4118, #4119, and #4120

stephenfin added 6 commits October 2, 2017 23:16

Fix typo in doc

0ec39b7

It's 'py:mod', not 'py:module'. I don't know why this wasn't caught by Sphinx itself. Signed-off-by: Stephen Finucane <stephen@that.guru>

tox: Restructure file

35b8115

Shuffle things around in order of most importance. The 'py27' target is removed as its completely unnecessary. Signed-off-by: Stephen Finucane <stephen@that.guru>

tox: Migrate pylint target to tox

886b236

This isn't enabled by default because it throws so many damn errors. Perhaps we should simply remove this? Signed-off-by: Stephen Finucane <stephen@that.guru>

tox: Always run coverage

f2bcaac

There is value in examining coverage for something like Sphinx. Start doing this by default, in anticipation of increasing coverage over time. Signed-off-by: Stephen Finucane <stephen@that.guru>

utils: Remove the 'reindent' tool

9787e91

Nobody seems to be using this, probably because of the 'flake8' target, and it hasn't been touched, some Python/flake8 updates aside in years. Just remove it. Signed-off-by: Stephen Finucane <stephen@that.guru>

utils: Use better variable names in check_fileheader

187f887

We're going to be doing some surgery on this function, so first clean it up before we do anything else. Signed-off-by: Stephen Finucane <stephen@that.guru>

stephenfin force-pushed the utils-cleanup branch from cc41102 to 442daef Compare October 2, 2017 22:17

stephenfin changed the title ~~Utils, Makefile cleanup~~ The great tox-ification Oct 3, 2017

tk0miya requested changes Oct 3, 2017

View reviewed changes

stephenfin force-pushed the utils-cleanup branch from 442daef to 79d7acd Compare October 4, 2017 16:28

stephenfin added 5 commits October 4, 2017 18:17

Move mypi configuration to 'setup.cfg'

4f76728

Less top-level files = winning [1]. [1] https://mypy.readthedocs.io/en/stable/config_file.html Signed-off-by: Stephen Finucane <stephen@that.guru>

tests: Skip tests with missing binaries

8794dc9

While there are already some skips included here, they clearly aren't doing their job and the test fail locally. Resolve this. Signed-off-by: Stephen Finucane <stephen@that.guru>

stephenfin force-pushed the utils-cleanup branch from 79d7acd to 40f320e Compare October 4, 2017 17:19

tk0miya reviewed Oct 5, 2017

View reviewed changes

stephenfin mentioned this pull request Oct 5, 2017

Some testing related enhancements #4113

Merged

stephenfin closed this Oct 5, 2017

stephenfin deleted the utils-cleanup branch October 5, 2017 08:49

github-actions bot locked as resolved and limited conversation to collaborators Sep 10, 2021

The great tox-ification #4105

The great tox-ification #4105

Conversation

stephenfin commented Oct 2, 2017 • edited

Feature or Bugfix

Purpose

Detail

Relates

tk0miya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephenfin Oct 3, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephenfin commented Oct 3, 2017

stephenfin commented Oct 4, 2017 • edited

shimizukawa commented Oct 4, 2017

Choose a reason for hiding this comment

stephenfin commented Oct 5, 2017

stephenfin commented Oct 5, 2017

stephenfin commented Oct 5, 2017

stephenfin commented Oct 2, 2017 •

edited

stephenfin Oct 3, 2017 •

edited

stephenfin commented Oct 4, 2017 •

edited