Run the isolation tests on Travis, and the functional tests on Circle, and style checks on Shippable

[greg-1-anderson]

This PR aims to refactor the tests such that:

• Circle CI runs the lint / style checks
• Travis CI runs the isolation (unit) tests
• Shippable runs the functional tests

There are two advantages to splitting the isolation tests from the functional tests:

• These tests are quite different in terms of how they are set up; separating them into separate services allows the test scripts (*.yml files) to be simpler for both services.
• Putting the isolation tests in a separate service makes it easier to test more php versions for the isolation tests than for the functional tests, and allows you to get a "green" indicator for completed isolation tests faster, without having to wait for the functional tests to complete.

I'm reducing the functional tests to only run on one version of php; shippable does support Travis-style php version selection that will run tests across the whole matrix, though, so it would be possible to test both a 5.x and a 7.x version of php if desired.

It's better to run the unit tests on Travis and the functional tests on Shippable, as Travis has better availability for php versions (w/out making a custom container), and the functional tests are less sensitive to the exact php version used.

Maybe the README might need more badges if we do this. 😄

[greg-1-anderson]

Really close! Blocked on Shippable/support#4100

[greg-1-anderson]

We've already got improvement here!

Previously, we had to wait >1hr to run the functional + isolation tests on Travis alone. With this new division, we have:

• Isolation tests on Travis: 7 minutes
• Functional tests on Shippable: 33 minutes

These run in parallel, so we're already looking at cutting our test time about in half. However, I'm still not satisfied. Shippable is running all of our tests in series, so we loose all of our parallelism. There's also a lot of waiting around required before we get our one container to run a test in. However, once we have a container scheduled, our tests run really really fast.

I'm going to make one more test run, this time reducing the number of different builds variants that we queue on shippable. I think this will give even better results.

[greg-1-anderson]

Woah, now it's really fast! The shippable build is down to only 17 minutes!

We're not done yet, though. The thing is that each serial build only took about four minutes; the tests had to wait about a minute for the first test to start, and then there was another 8-minute delay before the second test started. If we just eliminate the matrix entirely, we can probably cut our test time in half yet again.

[greg-1-anderson]

Well that last run was super fast!

• Lint/style tests on Circle CI: 2 minutes
• Isolation tests on Travis CI: 7 minutes
• Functional tests on Shippable: 7 minutes!

Down from an hour to only 7 minutes! That's amazing!

The only problem is that there's one failure in the shippable tests. The first set of tests all pass, but then I just re-ran unish.sut.php to rebuild the sut. It did, in fact, build a new sut with Drupal 8.5.x as desired, but the composer-lock-update test failed for some reason. I probably need to rm some files in between the tests. Any ideas?

[greg-1-anderson]

So, I was sort of thinking that we could rely on the isolation tests for php version coverage, and just run the functional tests on a single version of php. The problem with this is that there is a lot of code in Drush that is never exercised unless there is a bootstrapped Drupal site. The isolation tests should still parse every file, which will catch some instances where a php-7-only feature creeps into the code base. However, I don't think it would catch everything.

I am reluctant to give up on the 7-minute tests. If we added php 5.6 to the functional tests, then we'd be at (7x2) + 1 (wait for first test) + 8 (wait for second test) = 25m. Way better than before, but so so much longer than 7m. We'd be up over an hour again if we split apart the highest / lowest tests again AND added php 5.6.

What if we did this instead:

• Declare php 5.6 "lightly maintained" and encourage everyone to use php 7+ for Drush 9
• Make a "standalone" group in the functional tests, for tests that do not require Drupal, and run these with the isolation tests on Travis
• Make a Drush release script that makes a PR that edits the shippable.yml file to add more php versions, so there's one slow test before every release

What do you think? Is it worth continuing here?

[weitzman]

Nice work!

• I think some of the speedup is due to us not testing as much. Specifically, we lost functional tests on php 5.6. IMO thats worth keeping.
• I'm not sure why that test is failing. I wanted to ssh into the build but that feature is only for premium subscriptions. So, my ideal is that the functional tests move to Circle where we do have this capability. We also have 4 machines available to us.
• I may need to be granted perms on Shippable. I logged in via Github and Idon't see a way to re-run the tests at https://app.shippable.com/github/drush-ops/drush/runs/23/1/tests. I'm taking that as a sign that I dont have perms.
• We could use a sentence in tests/README that describes how we use each service.

[greg-1-anderson]

Yes, the individual tests on shippable are super fast, but there's a long wait to get a container on the free service. We would be up to ~30m to test two versions of PHP on shippable. I can try Circle 2.0 with four parallel builds for the functional tests as you suggested, and perhaps run just the code style tests on shippable.

I sort of thought that anyone in drush-ops would have access to the shippable builds, since drush is in drush-ops. However, I logged in to shippable with my github account, so maybe that's the only account that gets admin permissions. This might be another paywall feature to encourage folks to update to premium. Maybe this will be less of an issue if we only use shippable for code style.

I was thinking about putting the test results in a table:

Test	Status
Code style	[Passing]
Isolation Tests	[Passing]
Functional Tests	[Passing]

If it's not clear from the badge what the service is, we could also put the service name next to the badge (or just let the user find out which service is which when they click through -- although it's good to know offhand which is which, because you only see the service name in pull requests)

[greg-1-anderson]

I figured out why that test above is failing. Building the SUT is correctly getting the COMPOSER environment variable set, but I didn't pass the same environment variable along to phpunit, so the test is looking for the wrong composer.lock filename. Not fixing for now, since I'm going to move functional tests to Circle.

[greg-1-anderson]

Working on swapping the responsibilities of shippable (will run style checks) and circle (will run functional tests). Starting with just one test suite on one php version for now. Eventually we can use our four parallel jobs on Circle to run php 5.6 and 7.1 with composer.json and composer-highest.json.

[greg-1-anderson]

MUCH fiddling to get mysql databases working on Circle.

[greg-1-anderson]

Almost 12m to run just one test suite on one version of php on circle; a fair bit slower than the same treatment on shippable. Tomorrow I'll see how much that scales up when we run four tests in parallel. This could be quite a big win if they all start up pretty close together.

[greg-1-anderson]

So the good news is that the latest failed test took only 15:38 in aggregate, when the slowest individual step in the parallel test suite took 15:35:

https://circleci.com/workflow-run/37644a5a-5b8f-489b-85b1-8d9e72e4aac7

By coincidence, the slowest test ran first. It's not clear how much wait time there was for the other tests to start up. In some instances, we might have more than 3s overhead, but overall the efficiency of the parallelism is looking very promising. The failed test will run longer when it succeeds (one step was skipped), but it looks like we stand to gain quite a bit here.

One of the odd things about Circle 2.0 is that it requires that you have a step called "build". Since we want to run unish.sut.sh separately for each permutation, we do not need or want to share any build data between the processes. Therefore, I am using the build step to run one of the tests, to save the time we'd have to wait to schedule an extra container.

I have therefore named the steps as follows:

build: PHP 7.1 + composer.json (Drupal 8.4)
build_highest: PHP 7.1 + composer_highest.json (Drupal 8.5)
build_56: PHP 5.6 + composer.json (Drupal 8.4)

We have one more parallel process available to us. We could do a PHP 5.6 + composer_highest, but I thought that maybe it might be worthwhile to make a composer_lowest.json, and test Drupal 8.3 as well. Let me know what you think would be the best use of this last permutation. We could also choose to punt shippable, and use this last slot to run the code style checks. (We'd loose the ability to separately badge the style checks if we did this, though, so I don't think there's any point.)

I'm a little mystified by the test failures. In the PHP 5.6 build, it looks like the timezone is not getting set correctly. Maybe the PHP 5.6 container is placing the php.ini file in a different location. On the composer_highest test, I can't come up with any hypothesis about why the tests are failing. The good news, though, is that now that we are running the functional tests on Circle, this gives us the added benefit of being able to ssh into the build to figure stuff like this out. I'll do that later.

Moving forward, there are a couple more optimizations we could make. The setup script takes about a minute to run; we could save all of this time if we made a custom docker container that was pre-built with the binaries and settings we need. It is also really easy to use a custom docker container on shippable. I was thinking of making a docker container that has phpenv and all of the main versions of php that we want to support. (Likely someone has already built something like this.) With this sort of container, we could run the lint tests with each supported version of php. This could really help catch instances where version-specific features slip into a PR.

[greg-1-anderson]

All green! I'm unsure why there is that entry ci/circleci -- Waiting for status to be reported when all of the individual Circle tests are passing. It is also kind of a bummer that we have one build too many, which results in the Travis build summery being just off the bottom of the scrollable region.

We still have one more parallel Circle build we could use.

I added some badges to the README in a table. Made a typo in one, but I'll fix that. It's a little large; not sure how we should format and place this information. I'll do something similar on the page that has the existing build status badges.

The current build times are:

Code style on shippable: 1m total, 1m running
Isolation on travis: 10m total, 3m running
Functional on circle: 13m total, 13m / 11m / 10m running in parallel

The amount of time required to wait for a container can be highly variable on shippable and travis; circle, on the other hand, seems to start for us right away. In aggregate, this gets us down from over an hour to under 15 minutes! Pretty darn good.

I'm going to do one more build just to clean up the badges.

[greg-1-anderson]

I figured out how to fix the problem with the phantom ci/circleci build that never came in. This was a required test that no longer exists. I altered the GLOBAL PROJECT SETTINGS to fix this. We should therefore probably merge this branch pretty soon, as I've sort of messed up all the other PRs here. 😝

[weitzman]

Just minor comments. Feel free to merge.

[weitzman]

does this override *defaults?

[greg-1-anderson]

Yes. I could remove the image in *defaults, since it is never used.

[weitzman]

Hmm. My understanding is that all we need to do is assure that TEMP env variable is available. Unish does the rest.

[greg-1-anderson]

Yes, I should have commented on this. The problem is that on Circle CI 2.0 with composer-highest.json, when Unish launches drush in a subprocess, somehow the HOME environment variable is whatever it was set to by the process that called Unish, regardless of what Unish tries to set it to. It is very odd that a) this does not seem to happen with the composer.json test (implying that the cause is in symfony/process), although b) this does not seem to happen with any test in the old Travis CI scripts. Setting HOME to what the drush subprocess expects it to be is a workaround, but this would break again if the Unish path calculation for HOME changed again in the future. 😞

[greg-1-anderson]

I guess this is related to the comment in CommandUnishTestCase::execute()

[weitzman]

not needed anymore?