Use Travis-CI stages in build, and add a stage before tests for short-lived tasks

[kinow]

In our build process in Travis-CI, we have 4 chunks, each being executed in parallel and running the cylc test-battery command.

Within those 4 chunks, any of them can pick up the test that executes pycodestyle. This test is triggered via cylc test-battery.

But in case pycodestyle failed to validate our syntax, it could take minutes (even more than 10 minutes) after the build started until the build was marked as failure.

So what we have in this pull request is a simple change to use stages in Travis-CI. One stage called Unit-test, which will run pycodestyle and pytest, and another stage that keeps the old behaviour, running the test battery.

As in the screen shot, it is possible to see that pycodestyle and pytest currently take together 1 minute and 15 seconds to run. And the other steps won't start until this first stage is done.

This means that before you are able to get a cup of coffee, Travis-CI will have failed if you added a bug found in one of the unit tests, or if you forgot the 80 characters limit in your Python source code (which I did a few times and had to edit my commits 😠 hence this pull request).

Cheers

(moved some parts of the build to external scripts, so that they could be re-used between stages)

[kinow]

Oh, and happy to fix conflicts here if the coverage pull request is merged first. Ditto if this one is merged first, no problems with fixing conflicts in the other pull request 👍

[matthewrmshin]

Minor comments, but otherwise OK to go in.

[oliver-sanders]

currently take together 1 minute and 15 seconds to run

Most of that is probably installation!

A couple of thoughts:

I would have thought that we might as well run the unittests as a parallel job along with the functional tests to avoid making the functional tests wait 1:15 before they start running? Or is the idea to avoid unnecessary runs of the functional tests?

We should probably run the unittests using the minimum and maximum supported Python versions (presently 2.6 & 2.7, near future 3.? & 3.7). Luckily TravisCI makes this really easy.

[kinow]

Hi @oliver-sanders

Most of that is probably installation!

Most likely! pytest is taking ~0.4 seconds to run the unit tests on master in my computer. That number, however, will increase a bit as we add more tests.

I would have thought that we might as well run the unittests as a parallel job along with the functional tests to avoid making the functional tests wait 1:15 before they start running? Or is the idea to avoid unnecessary runs of the functional tests?

Good point. The idea is indeed to avoid the runs of functional tests, but in theory starting unit tests alongside the functional tests could speed it up even more.

Only issue that I have with that, is that if you look at Travis-CI after you push changes, it starts the 4 jobs in our build matrix right now, one for each CHUNK env var. But sometimes one or two jobs take a long time to start.

I assume that's Travis-CI's internal scheduler/workload system that is waiting for a container or VM to start up, or to become free, until the job is assigned and started up. I've seen jobs taking more than 7 minutes to start, while other jobs for other CHUNK values were already running.

It might increase a bit our build time, but I hope to be able to work on improvements for the functional tests later too 😬

We should probably run the unittests using the minimum and maximum supported Python versions (presently 2.6 & 2.7, near future 3.? & 3.7). Luckily TravisCI makes this really easy.

Agreed. I am waiting until the test coverage pull request, and the - currently a WIP - setup.py pull requests to be reviewed / merged, and then the Python 3 branch work starts, so that we can start using tox.

With tox and tox-travis, we can easily run it in Travis-CI against different versions of Python. Main advantage of tox is that it handles creating isolated virtualenv per run, and also tests the installation of the module through setup.py.

That way it reduces the chances of having surprises when uploading the package to PYPI or Conda 👍

[kinow]

Fixed issues pointed by @matthewrmshin (thanks!). Rebased, now Travis-CI is happily building it again 👍

[oliver-sanders]

+1 for moving the testing script out of the .travis.yml, I'm trying to do the same for Rose!

Alongside the pycodestyle test (tests/syntax) there is also a pep8 test which should be removed as well (pep8 was renamed pycodestyle so we have different tests to allow running on legacy systems [like ours]).

I think there should still be an easy way to run these code checkers locally, especially to help new contributors who won't be aware that to test their change they must run cylc test-battery and pytest and pycodestyle (where before they just ran cylc test-battery).

Is it possible to slot syntax checks into the pytest battery? Or perhaps we could leave all of these tests in the Cylc test battery but make them skip when run on Travis-CI (using a SKIP_UNIT_TESTS environment variable or something of that sort).

[kinow]

Triggered build twice after @oliver-sanders comments, first time the unit tests were executed in 1 minute and 6 seconds, second time in 1 minute and 2 seconds. So down 10-20 seconds 🎉

Also update Travis-CI to build against the latest Ubuntu distro in Travis, xenial. We were building against trusty, which I think is the old LTS. The current LTS, bionic, is not available in Travis-CI yet.

[kinow]

@oliver-sanders just removed the pep8 test. For what's worth, I sent a pull request to another project (Tornado I think?) and it failed running black.... I had a look, and found out they were using black instead of pycodestyle or pep8.

Then later I read a blog post and the author mentioned black again... so maybe we can look into black later (I think it's Python 3 only), though I'm quite happy with pycodestyle for now (just wished we could perhaps increase the line length to 100 or 120... so much unused space in my monitor 😬 )

[matthewrmshin]

so much unused space in my monitor

I normally have my monitor turned 90°. Perfect for code development (and for reading web sites that insist on displaying everything in a vertical block).

Wide screen is for watching films.

[oliver-sanders]

Wide screen is for watching films

Agreed but it is also the assumed aspect. The ideal?

`----------`
|          |
|          |   `----------------------`
|          |   |                      |
|          |   |                      |
|          |   |                      |
|          |   |                      |
|          |   `----------------------`
`----------`

[kinow]

Well, never tried using my monitor like that. Might give it a try then :-)

[oliver-sanders]

I just tried running the pytest command from the .travis.yml file and got a surprising result. It seems to run the Cherrypy test modules and then just three of the Cylc test modules!

$ cd "${CYLC_HOME}"
$ PYTHONPATH="$(pwd -P)/lib/" pytest lib/cylc/
going into /net/home/h06/osanders/cylc/lib/cherrypy/test
========================  test_auth_basic.py  ========================

... and so on for all the cherrypy test modules

going into /net/home/h06/osanders/cylc/lib/cylc/tests
==================  test_conditional_simplifier.py  ==================

=======================  test_graph_parser.py  =======================

==========================  test_rundb.py  ===========================

I'm bamboozled.

[kinow]

Oh, yup. I think the test coverage pull request includes a pytest.ini. Which specifies where to look for tests.

For now you would have to trigger pytest lib/cylc.

And if that fails to import cylc, then try PYTHONPATH=lib/cylc pytest lib/cylc (on mobile, not tested, but that's the gist).

Travis I think isn't setting pythonpath. But IIRC Cylc had a script (cylc.init?) That modifies sys.path... I'm planning to look into that after the setup.py is in

Sorry the mess!

[oliver-sanders]

maybe we can look into black later

So black is a code formatter rather than a linter, that is it doesn't make suggestions it actually makes changes to your code.

Black is opinionated so you don't have to be

[matthewrmshin]

So, finally tidy for Python!

[oliver-sanders]

@matthewrmshin spotted that pytest seems to run the correct tests if run from within lib/cylc:

(cd "${CYLC_DIR}/lib/cylc"; pytest -c)  # -c suppresses output unless errors occur

To allow us to run unittests outside of Travis we could add something like this to the cylc test-battery command (we may want to capture and raise any error codes):

diff --git a/bin/cylc-test-battery b/bin/cylc-test-battery
index c34dde88e..388363588 100755
--- a/bin/cylc-test-battery
+++ b/bin/cylc-test-battery
@@ -149,6 +149,29 @@ if [[ -w "$CYLC_DIR/lib" ]]; then
     python -mcompileall -q "$CYLC_DIR/lib"
 fi
 
+LINTERS=(pycodestyle pep8)
+
+# Run python tests.
+if [[ ! -n "${@:-}" ]]; then
+    # Code style.
+    for linter in "${LINTERS[@]}"; do
+        if which "${linter}" >/dev/null 2>&1; then
+            "${linter}" --ignore=E402,W503,W504 \
+                lib/cylc \
+                lib/Jinja2Filters/*.py \
+                lib/parsec/*.py \
+                $(grep -l '#!.*\<python\>' bin/* | grep -v 'cylc-test-battery')
+            break  # run the first linter present on the system
+        fi
+    done
+
+    # Unittests (including doctests).
+    (cd "${CYLC_DIR}/lib/cylc"; pytest -c)
+fi
+
+# Run functional tests.
 if perl -e 'use Test::Harness 3.00' 2>/dev/null; then
     NPROC=$(cylc get-global-config '--item=process pool size')
     if [[ -z "${NPROC}" ]]; then

Unfortunately the unittests are not Python 2 friendly, on 2.6 I'm getting the following errors:

1. TypeError: failUnlessRaises() takes at least 3 arguments (2 given)
2. ImportError: No module named mock

Suppressing these issues is awkward since:

1. unittest.skip is a 2.7 feature
2. We cant do except ImportError: sys.stderr.write('no mock: skip tests\n'); sys.exit(0) as that would exit the test process.

[kinow]

@oliver-sanders once we have the setup.py merged, we can simply run python setup.py test, or pytest (as there will be a pytest.ini). I think it might be easier to leave the unit tests outside test-battery. Other devs seeing a setup.py or pytest.ini file would probably try running pytest rather than the test-battery.

[kinow]

On black, it does change the files automatically for you (some people use that in git hooks), but I'm a bit paranoid to have something changing files without my review 🤓 so I like the way Tornado guys used black:

https://github.com/tornadoweb/tornado/blob/8abbb90f339341da610a784e4eb248508ad7ba70/.travis.yml#L102:

    - if [[ $TRAVIS_PYTHON_VERSION == '3.6' ]]; then (cd .. && black --check --diff tornado demos); fi

With the --check --diff flags it will verify the syntax, but won't change the files. So I think it's quite like a replacement for pycodestyle in this way.

[matthewrmshin]

(I wouldn't want to use it in an automated hook either. However, the option to change my files in a working tree - so I can inspect the diff before committing (or reset) is a good one.)

[matthewrmshin]

Push back to soon - given the dependency of this PR on #2834 - or we may want to have setup.py moved forward as part of this PR?

Either way, we should ensure that our local site tests do not become many commands?

[kinow]

@matthewrmshin I think it's easier to keep both separated for now, so that feedback on each can be addressed separately (or the work can be postponed/included in a release more easily).

If we merge the setup.py branch first, I can quickly fix this branch.

Otherwise, if this one is merged first, I would be happy to fix the other branch as well :)

[matthewrmshin]

Hi @kinow Is it OK to update and complete this one first before #2834?

[kinow]

Sure thing @matthewrmshin . As this one is smaller, we will have less risks working on this one for perhaps 7.8.1 or later 👍

Will rebase, and review last comments to see what's missing here.

[kinow]

Done @matthewrmshin

• Rebased & fixed conflicts
• Fixed unit tests that were not passing (even though they run headless, looks like some graphing unit tests depend on having pygraphviz... might need to keep an eye on those tests when moving away from the GUI to web)
• Added back coverage (this PR was created separately, without it). Looks to be working fine: https://codecov.io/gh/kinow/cylc/commit/fb532123f22254732851a95d7bcae17bd6d754af/build the 5 items are 1 unit test build and 4 functional tests that reported the coverage. The coverage number looks correct too.

Ready for review again I believe.

Cheers
Bruno

[matthewrmshin]

@oliver-sanders Has @kinow addressed your concerns?

[oliver-sanders]

Has @kinow addressed your concerns?

Almost, a couple of outstanding issues:

1. pycodestyle is now only run on Travis and is not included in either test battery. We will be adding more linters in the near future (e.g. Javascript) so need a workable approach for this.
2. To run the test battery we now need three commands (more looking into the future) rather than one.

Perhaps we should accept this now and work at unifying the test approaches in a future PR.

A further issue not entirely related to this change is that pytest tests break on import errors. At our site we don't have pytest installed in the standard Python stack so we will need to install it in a Python environment where we wont have pygraphviz which causes the graphing tests to break. We should adapt these tests to skip on ImportError.

Otherwise this is all working fine and I'm happy to approve this. pytest is working well, in the future if the unittests start taking longer to run there are pytest extensions which add parallelism (e.g. pytest-xdist).

[kinow]

Perhaps we should accept this now and work at unifying the test approaches in a future PR.

+1

Otherwise this is all working fine and I'm happy to approve this. pytest is working well, in the future if the unittests start taking longer to run there are pytest extensions which add parallelism (e.g. pytest-xdist).

+1

And I believe you are right @oliver-sanders about the new linters. Not too sure where they will be called. I have IDE integration, and also try to remember to run them before committing (though many times I run them after I see Travis-CI failing... 😥 )

If that's OK, I think we can address the pending issues in further pull requests.

[hjoliver]

2nd approval, on the basis that @oliver-sanders and @kinow agreed on further tweaks in follow-up PRs, (and I think @oliver-sanders is away for a few days?).