Add a new test type to do real update testing

alexpott created an issue. See original summary.

#2984031: Create Build Tests For Composer and Drupal seems very related.

At DrupalCon 2019 sprints we brainstormed a little about this issue and similar issue #2984031: Create Build Tests For Composer and Drupal

I think the relatively narrow scope of both these issues helps us reach a conclusion.

Let's think about this issue first, since it addresses an existing gap in our test infrastructure. We'll keep in mind that the solution should be flexible enough to address #2984031: Create Build Tests For Composer and Drupal and similar build-based issues.

We can also have follow-ups to move existing unit-test-scoped builders to use the framework we make here. This would include the demo umami installer and so forth.

Tests should fail here. This is a proof of concept and is offered for discussion.

This patch adds a test type called BuildTestBase. The testsuite is 'build'.

The main concepts here are:

• A way to locate a workspace directory.
• A way to execute command line commands.
• A way to assert that comands worked.
• A way to use only the kernel to send 'requests.'

The use case for this is as outlined in the IS: Performing an installation and then performing and update, and then performing requests.

There's some infrastructure and one test: Drupal\BuildTests\BuildInstallTest. It looks like this:

  public function testInstall() {
    // Perform clone of drupalci's core repo into fixture dir.
    $this->assertCommand('git clone --branch 8.8.x --depth 1 https://git.drupalcode.org/project/drupal.git .');
    // composer install
    $this->assertCommand('composer install');
    // demo_install
    $this->assertCommand('php ./core/scripts/drupal install standard');
    // visit paths
    $this->kernelRequest($this->getWorkspaceDirectory(), new Request());
  }

This test fails locally like this:

$ ./vendor/bin/phpunit -c core/ --testsuite build
PHPUnit 6.5.14 by Sebastian Bergmann and contributors.

Testing 

THE ERROR HANDLER HAS CHANGED!

This means the kernel request began to work, but the kernel happily changed the error handler without concern for phpunit-bridge's feelings. (Actually this doesn't mean that, because we never get beyond trying to load the autoloader.) You can see that I marked it @group legacy, and I also tried to disable the bridge, but no luck.

Edit: I neglected to add this testsuite to drupalci.yml so it won't run. Sigh.

Composer lock file suitable for PHP 7.0, run-tests.sh --browser now works, truncated drupalci.yml.

Forgot interdiff...

An artifact from the last test:

PHP Fatal error:  Cannot redeclare composerRequireDrupal8() (previously declared in /var/www/html/vendor/composer/autoload_real.php:67) in /tmp/build_workspace_be4e9c40d642212cb9b50c7bb19ee8e9/vendor/composer/autoload_real.php on line 74

So we use require_once instead.

Rethinking so we're using PHP's test server as the HTTP server.

Currently does not work. Sharing here for review.

Here's a working test framework. You'll see there are plenty of @todos.

The main problem that exists now is that PHP's HTTP server isn't shut down by the time the next test is run. This would be ameliorated by using semirandom port numbers (so they can overlap for concurrency), and/or by adding a wait() in the tearDown() method.

Converted to Mink so we can do nifty assertions. We don't use UiHelperTrait because it needs a bunch of core stuff that is inappropriate for this test type.

Tests created with this framework can add all sorts of dependencies, like being able to run git. So we might consider being careful about that as we move forward.

The main TODOs here:

• Figuring out the timing on letting the PHP HTTP server spin up. I'm pretty sure that's why it's failing here.
• The tests rely on having different port numbers for concurrency. Since they'll be running *from* different processes, we're almost guaranteed that there will be port collisions under run-tests.sh. So we need a way to ensure that ports aren't being recycled before their time.
• The file workspaces are a little tricky to clean up due to permissions errors. Currently the workspace directories will just fill up /tmp.
• Since the workspaces are under /tmp, we might need to make a mode where it figures out that they should be under sites/simpletest/. I'm not sure whether this is purely necessary.
• It's going to be hard to turn any of the generated files into artifacts for DrupalCI. It might be that we don't want to, and only want outcomes.
• This framework has zero dependencies on Drupal. It could be a component.

https://github.com/twigphp/Twig/pull/2993 does some really interesting stuff that we might be able to leverage.

That does look interesting.

We'd still need to set up the PHP HTTP server and use assertCommand() and friends to create the environment, but functional tests could be blackfire player scripts.

https://github.com/blackfireio/player

It has a --json output option, but no idea how that would map to run-tests.sh or junit, or other sundry concerns.

Discovered this while making some tests for this issue.

Now that I'm back from vacation...

Adds some prototype tests for existing needs. This helps us understand whether the framework meets our needs.

Still non-concurrent. We'll need to figure out a solution using a lock file to protect concurrent HTTP ports.

There's a little bit of trouble in that in order to do real black box testing of update.php, our mink client has to respond to meta refresh tags. UiHelperTrait does this with checkForMetaRefresh(), but we can't inherit the dependencies of UiHelperTrait. That means we can either split up the trait or re-roll a similar solution here. The latter would involve expanding GoutteDriver::visit() to perform refreshes, which might be a better solution that could replace that part of UiHelperTrait. Or not. Dunno.

It's also true that as of 4.2.0, Goutte (via BrowserKit) will follow meta refresh tags for us: https://github.com/symfony/symfony/pull/27118

Anyway, the meat of the framework is in core/tests/Drupal/Tests/BuildTests/Framework. The rest are essentially POC tests.

@Mile23 amazing work.

Some thoughts:

• We can copy the meta refresh code - and add an @todo to remove it when we require Symfony 4. DRY is fine but there are times to ignore it.
• I'm not convinced that doing min max testing inside these tests as the latest patch adds for components is correct. If we look at travis and appveyor and probably many other CIs the build matrix is defined outside of test itself - as in what is being suggested in #2976407: Use drupalci.yml for composer dependency min/max testing - DO NOT COMMIT - just because we can do min max testing for components using this doesn't mean we should.
• Re getting a port - we could create something similar to \Drupal\Core\Test\TestDatabase to get a port number between 8080 and 65,535 - I think we'll also need to check whether the port is in use as well.

+++ b/core/tests/Drupal/BuildTests/Dependencies/CoreDependencyRegressionTest.php
@@ -0,0 +1,41 @@
+    $this->markTestIncomplete('Core cant pass this test yet. See https://www.drupal.org/project/drupal/issues/2976407');

Why doesn't core pass this test? I look on the linked issue but it is far from immediately obvious to me. But any as above I'm not sure we should be using this test type for this test anyways.

The core dependency regression test looks like this (once you comment out the line that marks it as incomplete):

$ ./vendor/bin/phpunit -c core/ --testsuite build --filter CoreDependencyRegressionTest

[...]

...........................................................  2773 / 17890 ( 15%)
...........................................................  2832 / 17890 ( 15%)
......................................
THE ERROR HANDLER HAS CHANGED!

Legacy deprecation notices (9)

ERROR: 

Failed asserting that 255 matches expected 0.

I was able to repro this on core like this:

$ git checkout 8.8.x
$ composer install
$ composer run-script drupal-phpunit-upgrade
$ ./vendor/bin/phpunit -c core/ --testsuite unit

THE ERROR HANDLER HAS CHANGED! is basically the least useful error message ever. :-)

Some hacking later and we discover that this error handler error goes away when we fix DrupalKernelTest: #3054649: DrupalKernelTest results in ERROR HANDLER CHANGED!

There are still a bunch of other fail/error tests, so we need to address those. But CoreDependencyRegressionTest is more of a proof of concept in this instance anyway.

I'm not convinced that doing min max testing inside these tests as the latest patch adds for components is correct.

Same deal... This patch only proves that the framework allows for that sort of thing, and all the tests outside of Drupal\BuildTests\Framework are POC, not for inclusion in the final patch. We can argue about them in other issues. :-)

Sorry if I didn't make that clear.

Here's another one: #3054674: ThemeHandlerTest and friends break isolation for constants

Not quite sure how the current work here ties back to issue summary, and whether that is still accurate, but, two thoughts about the current issue summary:

* The reason we didn't saw those extension issues in our update tests is that our dumps have empty caches. That would be relatively easy to address by creating a dump that includes all cache data, would just be quite a bit of data. Maybe we can keep it out of git or use that git LFS stuff that gitlab supports? Or somehow fetch it on demand from somewhere?
* You can't install old versions of Drupal 8 on modern PHP versions, 8.4 and older doesn't install on PHP 7.2 for example.

The patch to review is in #20, and the stuff with fixing tests is a little off topic, yah.

I talked with @alexpott about this at DrupalCon and basically it's a black box test, where you do a build and then check expectations. This way we can perform an install -> upgrade -> see if we got what we want process.

There's a little scope creep in that I did some prototypes of other test types this build system would likely be used with, such as #2984031: Create Build Tests For Composer and Drupal and #2984068: Create a test that upgrades from one version of core to the next. and could be used for #2962157: TestSiteApplicationTest requires a database despite being a unit test

Another new child: #3055495: Search page migration tests are misplaced kernel tests

This patch adds a subclass of Behat\Mink\Driver\Goutte\Client that can follow meta redirects, with some tests.

Improves port seeking for the HTTP server.

The big patch includes all the child issue patches which allow the composer min/max test to pass.

The framework_only patch does not include any of these changes, or the prototype tests.

Did I crack the code of concurrent ports? Let's ask the testbot.

Edit: Woops, the framework_only patch isn't only the framework.

The previous fail shows us this error which fails the tests which copy core:

RecursiveDirectoryIterator::__construct(/var/www/html/sites/simpletest/91070516/files/dir): failed to open dir: Permission denied

This is a fail because we don't have permission to do anything with sites/simpletest/dir/. And why is that? Because Drupal\Tests\migrate\Kernel\process\FileCopyTest::testNonWritableDestination() does not clean up after itself. #2793121-15: FileTestBase doesn't confirm its filesystem exists, does not clean up

So that leaves us with some options: Wait for FileCopyTest to get fixed or just never copy sites/simpletest/ to the system under test. The latter seems completely reasonable regardless of FileCopyTest, so there we are.

We add symfony/finder so it can make the file iterator for us, and then we copy core without sites/files/, sites/simpletest/, or vendor/.

This patch also adds the ability to say @requires externalCommand git in the test annotation, so it can skip instead of doing a bunch of build operations only to fail for obvious reasons.

I don't understand why this has to be part of core. Why can't we setup deadicated project/jobs to run this?

Core maintainers seem reticent to trust a test that doesn't run under drupalci. For instance, this languishes, and I can only assume it's because Travis CI is inadmissable in court: #2876669-60: Fix dependency version requirement declarations in components

There was some small degree of work put into having different build targets within drupalci.yml. That way you could specify a different build process for RTBC or weekly regression test or whatever. Also languishes, not updated in a year, because its really only me and @mixologic: #2951375: Allow for build targets

So that leaves us with baking it into core, unless there's a better idea.

Core maintainers seem reticent to trust a test that doesn't run under drupalci.

Why can't we setup a new project on drupal.org?

What sort of project?

I don't understand why this has to be part of core. Why can't we setup deadicated project/jobs to run this?

Because if a change to core breaks this we shouldn't be committing it.

Note the vital part of this is not min max testing of our components. It's the ability to do proper update testing as a user would do updates.

Note the vital part of this is not min max testing of our components. It's the ability to do proper update testing as a user would do updates.

That's stubbed out in Drupal\BuildTests\Update\UpdateTest, which demos updating from arbitrary releases to a newer release using git, and also updating from a release tag to the current codebase (including everything patched in DrupalCI).

It uses the Quickstart feature, which might not be ideal. Do you have any specs for how this update should occur?

It's the ability to do proper update testing as a user would do updates.

if a change to core breaks this we shouldn't be committing it.

I totally understand both points but it doesn't mean it has to be in Drupal core codebase. We can still set up a drupal CI job(along with the test runs) for each patch in Drupal core issue queue to do the following things:

It will install the minimum version of Drupal supported determined by our policy and the branch that is being tested \Drupal::VERSION
It will install Drupal codebase using composer
It will install Drupal somehow
It will log into the site as user 1
It will then be able to do upgrades via composer / drush / tarball?
It will then test the resulting upgrade.

+++ b/core/composer.json
@@ -63,6 +63,8 @@
+        "symfony/filesystem": "^3.4",
+        "symfony/finder": "~3.4",

We have 7 years old issue to add these components #1451320: Evaluate Symfony2's Finder Component to simplify file handling and we also have #3029088: Replace file system component with a 3rd party library. I think adding these components here just for this patch is not right. We need a wider discussion and other things to consider like #1451320-25: Evaluate Symfony2's Finder Component to simplify file handling here.

#3043235: [Policy, No patch] How to provide automated, in place security updates of Drupal core is an issue currently in a sandbox project. But parts will start land into drupal core over the next few releases. And in that case, we want to be able to download certain, fixed versions of Drupal and provide a mechanism to update to a newer release. We could _possibly_ work around that, but it would be ideal to run actual, in-place updates of the version of files in a project.

@jibran

I think adding these components here just for this patch is not right.

We're not adding these components to core. We're adding them as dev requirements. This means that any project built on top of Drupal does not inherit them (well it would lovely if that was 100% true but things a very inside out in Drupal land). But the point is that we're not bringing them into core in the same way as a regular dependency which is what those issues are doing and can still do regardless of this issue.

With respect to

We can still set up a drupal CI job(along with the test runs) for each patch in Drupal core issue queue to do the following things:

I don't think what being suggested is realistic. Adding matrix builds to DrupalCI is not an inconsiderable amount of work. The association has many other priorities. We have a desperate need to prove updates and do them in as real a way as possible. And as @heddn points out we other things coming in the pipeline that really really need this functionality.

If someone is willing to put the work into DrupalCI to make it work that way or has a realistic architecture that can point to the needed changes and inspire others to work on it then great but otherwise I think we should proceed here. But before someone invests the time and energy to do that I might point out that the main contributor to this issue @Mile23. @Mile23 is one of the two main current contributors to drupalci and I trust that if this was simple to implement there they might be doing just that.

Currently DrupalCI has command and container_command plugins. It's entirely possible to do all of this in either a script that is called by container_command, or as a series of container_command directives in a drupalci.yml.

So we could, conceivably, without changing DrupalCI at all, add a build phase to the current drupalci.yml which does all this stuff. It would run with every patch. It would be cumbersome but it's do-able.

The down-sides of doing that would be that if you're normalizing on using PHPUnit to run tests locally, you wouldn't be able to run these important update (or build regression) tests unless you learned how to run DrupalCI locally. We'd pretty much still have to wait for a patch run to see whether the changes broke updating.

The up-side is that someone would have to write it, and then maintain it. And I'm not sure either of those is really up-sides. Because when you write that script, you're basically re-implementing some of the build test suite I just wrote, or you're stretching the boundaries of container_command and DrupalCI scripting.

As far as adding a build matrix or other features to DrupalCI, it has a build matrix already for environments. Just start another test for whichever PHP version or DB. It's not scriptable, but it can be set up to be automatic.

As far as whether or not to use symfony/finder or symfony/filesystem, like @alexpott says these aren't in core, per se, and guess what? If we have a build system, it can build a Drupal site that is build as composer install --no-dev and find out whether someone did a no-no and used symfony/finder in production code or whatever. Without it we can't, full stop.

Also, we shouldn't use the library we're testing to build the SUT. That leaves bare-metal PHP and that makes the test system less maintainable.

And also also: symfony/finder, for instance, just speeds up development. That's another thing we don't have to test in this new infrastructure. However, if it's absolutely necessary that we not have rapid development on this thing, then review the patch and say, "Don't use symfony/finder." Then it will take much longer and result in twice as much code to maintain.

As far as development on the testbot, it seems to be mostly me and @mixologic, but really mostly @mixologic. So if you can clone @mixologic and get the clone on the task of telling me what's his priority, then maybe there's enough time in the day. Also I'm currently looking for work and so therefore the testbot gets low priority.

Reroll and updated the core min/max test after #3008870: Drop support for PHPUnit 4.8 once PHP 5 is no longer supported (8.8.x)

Dunno how I missed @heddn's earlier comment in #39. #3043235-21: [Policy, No patch] How to provide automated, in place security updates of Drupal core looks like a pretty solid endorsement.

Combined with #2984031: Create Build Tests For Composer and Drupal this means that in addition to the existing update needs, two important initiatives also need a way to do this. Both should be scoped for running in a patch evaluation context. It's much easier to snap together some Symfony components with Mink to accomplish this than to add more infrastructure to DrupalCI, and the tests which use this framework would have a better DX than a private build or a shell script.

This would give us velocity on both Automatic Updates and Composer initiatives so maybe we'll accomplish one or both before 8.8.0 is released.

If we're talking about the latter (running the existing test suite against a variety of scenarios), then we'll need to be *very* sure about how/what/when we do those kinds of tests, because that sounds like a lot more testing, and perhaps a lot of it that is not useful except in very specific circumstances.

Yes, exactly.

The reason some of these demo tests run tests subsequently is to prove that they worked. They're also demos to prove the concept of a building a thing and then proving it works.

Again: The thing to review here is the _framework patches, not the full one.

If anyone has any suggestions about how to organize this better or specific use cases that are not addressed, please offer it so we can have some forward motion.

This patch changes the way copyCodebase() works so that you can inject whatever iterator you please. By default it excludes site-specific files.

Because of this change, I'm including both the framework patch and the full proof-of-concept patch. In the future it'll just be the framework.

The patch also adds some tests to the framework.

Anyone please chime in with some suggestions or next steps. Thanks.

Some experimentation tells me that we only need to tell the PHP HTTP server to sleep for 5 seconds by default when it starts up.

This patch:

• Fixes CS issues
• Improves the BuildTestInterface docblock docs.
• Adds getMink() to the interface since assertRequest() tells you to look at the Mink object.

I think you're referring to #2982684-177: Add a composer scaffolding plugin to core which, yah, bad. That's referring to the version baked into the composer.phar file you run at the command line.

These tests don't operate under the context of a Composer plugin, so we're good.

The only caveat I can think of is if someone runs their tests with a command like composer run-script tests.

I must have been trying to apply the patch to 8.7.x or something when I added that tag, because it applies now.

LGTM, just a few minor issues.

1. +++ b/core/tests/Drupal/BuildTests/Framework/BuildTestBase.php
   @@ -0,0 +1,365 @@
   +    // @todo Glean working directory from env vars, etc.
   

   @todo requires a follow-up issue (or just implement something reasonable).

2. +++ b/core/tests/Drupal/BuildTests/Framework/BuildTestBase.php
   @@ -0,0 +1,365 @@
   +    //   to fully start up.
   

   I used to use netstat -an for this. See wait-for-webserver script. Is netstat available on drupal-ci?

3. +++ b/core/tests/Drupal/BuildTests/Framework/DrupalMinkClient.php
   @@ -0,0 +1,65 @@
   + * @todo Update this client when Drupal starts using Symfony 4.2.0+.
   

   Needs follow-up issue

4. +++ b/core/tests/Drupal/BuildTests/Framework/ExternalCommandRequirementsTrait.php
   @@ -0,0 +1,98 @@
   +    // @todo Figure out the Windows version of this.
   

   On Windows, use `where`. See ExecTrait in Drush.

Let's first reroll this thing.

This picks up #54.3 and 54.4.

I'd argue for 54.1 we should remove the TODO as sys_get_temp_dir seems like a fine default location.

#54.2 could be a follow-up me thinks. Is netstat available on windows?

Looking into 54.2 right now. Found https://symfony.com/doc/current/components/process.html#running-processe.... Might be able to look for the results:

 php -S localhost:9090 -t .
PHP 7.2.21-1+0~20190807.25+debian9~1.gbp935ebf Development Server started at Wed Aug 28 20:37:50 2019
Listening on http://localhost:9090
Document root is /var/www/html
Press Ctrl-C to quit.

Let's test it out.

Seems we have an older symfony component. But there seems to be a comparable means. Here's a test.

+++ b/core/tests/Drupal/BuildTests/Framework/BuildTestBase.php
@@ -0,0 +1,364 @@
+    // @todo Glean working directory from env vars, etc.
+    $this->workspaceDir = sys_get_temp_dir() . '/build_workspace_' . md5($this->getName() . microtime());

Workspace directories per process have a hashed dir name. The idea with the todo was that if drupalci tells us an environmental variable for the temp directory, we should use that. Leaving it alone right now is probably OK, though, because I think this issue needs more consensus before we fill in too many details.

If drupalci would just make one temp directory for every test, and configure sys_get_temp_dir() to return it, it would make it much easier to avoid contention between different tests.

The php process seems to be special and know when it is attached to a tty terminal or not. But I think we have all the building blocks already. Let's try this on for size. It passes on local.

#68: Good idea. Here's an implementation that might suffice. Feel free to continue from #67 if this is out of scope for this issue.

Do we have a better alternative than adding simpletest_rmdir() here? I wasn't sure if it was safe / prudent to use Symfony components in run_tests.sh.

Here's some additional test coverage and demonstrates a more typical test scenario (along my thinking) of how to use this new build type.

Oops, cross post w/ #69. I'll merge them next.

Syntax errors? Bah.

microtime() has a space in it, so we'll use microtime(true) instead.

Missing a space.

... and setting the status.

Code style and minor cleanup.

Build failing with:

15:02:43 Build step 'Publish JUnit test result report' changed build result to UNSTABLE
15:02:43 [Deprecation] [-ERROR-] No files found for pattern 'jenkins-drupal_patches-6699/artifacts/phpstan/*.xml'. Configuration error?

Did not search to find out where that's coming from.

@heddn If you want to back out my addition so that the tests pass, that would be fine. I'm not sure why making a dedicated temporary directory interfered with asset collection. Maybe I deleted the temporary directory too soon. Perhaps someone else can help with this.

The lates failures seem to be from my test additions. I'm limiting the testing scope in drupalci.yml since otherwise takes a really long time when we can be pretty sure its just this new test type we want to turn green.

Fewer failures. And some minor refactoring for DX.

Set sendmail_path = "true" in php.ini to avoid errors sending email when installing a site. Unfortunately, this setting cannot be set via ini_set(); it has to be done in php.ini or via -d on the commandline.

I had the wonderful help of a colleague assist here. drush uli has an extra enter added to it that was messing up the http request and resulting in a 404. This should fix it now.

It was requesting:
http://localhost:8000/user/reset/1/1567199950/bwW9hwnekXsqOTBRWlbdrILBhT9uzBxg5DC-0Uf4Kz8/login\n

Notice the newline character "\n" in the URL at the end? It isn't easily visible in xdebug.

re #94: switching to executeCommand instead. It will fix the sendmail thing and let us install on php 7.2 even w/ warnings about the installer not supporting PHP 7.2.

re 96: this is about the drush uli command in the tests. I was hitting my head against a specific bug on my local for a really long time as described in that comment.

And hopefully this passes green.

Use Symfony Filesystem to remove our temp directory, since run_tests.sh is already loading the autoload file, and it is therefore available.

Clean up the 'uli' output a bit. It turns out that uli does not support output formatters, so `--format=json` is a non-starter.

Have we considered postponing this on #2982680: Add composer-ready project templates to Drupal core? Once that issue is committed then we can install multiple version of Drupal via composer to whatever folder we want using .drupalci.yml and simple upgrade script in the core can update the core the latest dev, run the upgrades and tests.

+++ b/core/tests/Drupal/BuildTests/Framework/Tests/BuildTestTest.php
@@ -0,0 +1,191 @@
+ * @requires externalCommand drush
...
+  public function testSiteUpdate() {
...
+    $this->executeCommand('drush site-install --db-url=sqlite://db.sqlite -y');
...
+    $this->assertCommand('drush updb -y');

I'm not sure where this was introduced, but a big ol' -1 on this for a couple of reasons:

1) The whole point of this test framework is that we can test Drupal core's install and update systems on a brand new codebase. If we let drush do that, then it's not the test we actually want. In fact, the reason we have an HTTP server in there is so we can test update.php.

2) You've added drush as a dependency of a unit test of BuildTestBase, which compounds the issue in #1 above.

Back in #46 there were tests of installing and updating core. I took them out so potential reviewers could concentrate on the framework instead of the test cases.

It seems like folks want the framework, so I've added those PoC tests back so we can talk about what sorts of test cases this framework should and shouldn't support.

Also moves the drush install/update test to its own test class.

Yes, regarding Drush, wouldn't it be better to use Drupal\Core\Command\InstallCommand instead of drush si? Similarly, it seems it would be worthwhile to also have an updatedb command in core.

As for min/max testing, I am wondering if perhaps we should hold off on this until we have a way to run the min tests in parallel. The existing test suite already takes over an hour to run; that duration could increase substantially if we reran a substantial portion of the suite in series.

That's the thing: The PoC tests are PoC and not the actual tests. :-)

The one that's in the patch does in fact use the install command. Check QuickStartTestBase. And again: While it would be nice to have an updatedb command, the raison d'etre for this test framework is to test existing update systems in core. While it's possible that making (and testing) the command would give some tertiary benefits for people who use update.php, the point of this issue is testing existing stuff.

So I'll add needs followup here, so we can go on to write actual tests based on what we land on in this framework-based issue.

The min/max testing part is somewhat contentious here and in other min/max issues, like this one: #2976407: Use drupalci.yml for composer dependency min/max testing - DO NOT COMMIT So I'm setting to needs work to remove the component and dependency regression tests. According to the log, all of the build tests took 4 minutes total, which includes re-running some unit tests with different dependencies. But adding it back can be a follow-up.

17:35:23 All tests will run.
17:35:23 
17:35:23 Test run started:
17:35:23   Saturday, August 31, 2019 - 17:35
17:35:23 
17:35:23 Test summary
17:35:23 ------------
17:35:23 
17:35:24 Drupal\BuildTests\Dependencies\CoreDependencyRegressionTest    1 passes                                      
17:35:24 Drupal\BuildTests\TestSiteApplication\InstallTest              1 passes                                      
17:35:24 Drupal\BuildTests\Framework\Tests\DrupalMinkClientTest        14 passes                                      
17:35:24 Drupal\BuildTests\Framework\Tests\ExternalCommandRequirement   2 passes                                      
17:35:24 Drupal\BuildTests\Framework\Tests\BuildTestTest                5 passes                                      
17:35:38 Drupal\BuildTests\Dependencies\ComponentDependencyBoundsTest   5 passes                                      
17:38:02 Drupal\BuildTests\Update\UpdateSiteDrushTest                   1 passes                                      
17:38:21 Drupal\BuildTests\Update\UpdateTest                            4 passes                                      
17:39:04 Drupal\BuildTests\QuickStart\InstallTest                       3 passes                                      
17:39:20 
17:39:20 Test run duration: 3 min 57 sec
17:39:20 

Removes the dependency tests and their fixtures.

Next question: Should this framework know about the database under test? That is: We probably want to install a test site using MySQL instead of the SQLite that is used by default in the quickstart site. How should that work?

I'm fine w/ removing the additional tests. I probably just got too excited. I'm going to do one final tweak to the drush build test here, then strip them out in the next comment so we can do what @Mile23 suggests in #101. I'd really rather have the framework then have us block ourselves on bikeshedding the contents of futher tests.

Actually, the PoC tests are in really, really nice shape. It seems a shame to strip them. How about some minor tweaks? I say minor, because as always, the tests written by Paul are already in very good shape.

Some minor comment improvements. Who else needs to review this at this point to get us to RTBC?

diff --git a/core/tests/Drupal/BuildTests/Update/UpdateTest.php b/core/tests/Drupal/BuildTests/Update/UpdateTest.php
index 46235aaa41..e4bc50d76b 100644
--- a/core/tests/Drupal/BuildTests/Update/UpdateTest.php
+++ b/core/tests/Drupal/BuildTests/Update/UpdateTest.php
@@ -6,10 +6,7 @@
 use Symfony\Component\Filesystem\Filesystem;
 
 /**
- * Test updates to core.
- *
- * This is a proof-of-concept test for the build test framework. Any resemblance
- * to actual tests, living or dead, is purely coincidental.
+ * Test updates to core using update.php

This update should happen if we re-roll this again. Or if not, on commit.

The first failure is an easy fix.

The failure for the site application I'm able to track down to the following error. I saw this a couple times earlier in my local testing but don't know what made it go away or if I did things differently and then it went away. For now I've marked that test as a WIP. Maybe someone else has some suggestions?

$ php ./core/scripts/test-site.php install --base-url=http://localhost:8001 --db-url=sqlite://localhost/foo.sqlite --install-profile=minimal --json
PHP Warning:  Class 'PHPUnit\Framework\AssertionFailedError' not found in /private/var/folders/2h/rkh_prld5gl2tftzbst02cc40000gn/T/build_workspace_5e91481e47f92248ad5178315cc98471/core/tests/bootstrap.php on line 190
PHP Stack trace:
PHP   1. {main}() /private/var/folders/2h/rkh_prld5gl2tftzbst02cc40000gn/T/build_workspace_5e91481e47f92248ad5178315cc98471/core/scripts/test-site.php:0
PHP   2. require_once() /private/var/folders/2h/rkh_prld5gl2tftzbst02cc40000gn/T/build_workspace_5e91481e47f92248ad5178315cc98471/core/scripts/test-site.php:19
PHP   3. class_alias() /private/var/folders/2h/rkh_prld5gl2tftzbst02cc40000gn/T/build_workspace_5e91481e47f92248ad5178315cc98471/core/tests/bootstrap.php:190

Warning: Class 'PHPUnit\Framework\AssertionFailedError' not found in /private/var/folders/2h/rkh_prld5gl2tftzbst02cc40000gn/T/build_workspace_5e91481e47f92248ad5178315cc98471/core/tests/bootstrap.php on line 190

Call Stack:
    0.0004     391792   1. {main}() /private/var/folders/2h/rkh_prld5gl2tftzbst02cc40000gn/T/build_workspace_5e91481e47f92248ad5178315cc98471/core/scripts/test-site.php:0
    0.0014     429504   2. require_once('/private/var/folders/2h/rkh_prld5gl2tftzbst02cc40000gn/T/build_workspace_5e91481e47f92248ad5178315cc98471/core/tests/bootstrap.php') /private/var/folders/2h/rkh_prld5gl2tftzbst02cc40000gn/T/build_workspace_5e91481e47f92248ad5178315cc98471/core/scripts/test-site.php:19
    0.7112    1311272   3. class_alias() /private/var/folders/2h/rkh_prld5gl2tftzbst02cc40000gn/T/build_workspace_5e91481e47f92248ad5178315cc98471/core/tests/bootstrap.php:190
<snip/>
    5.7568   44862752  17. Symfony\Component\HttpFoundation\Session\Storage\Proxy\SessionHandlerProxy->setId() /private/var/folders/2h/rkh_prld5gl2tftzbst02cc40000gn/T/build_workspace_5e91481e47f92248ad5178315cc98471/vendor/symfony/http-foundation/Session/Storage/NativeSessionStorage.php:167
    5.7568   44862752  18. session_id() /private/var/folders/2h/rkh_prld5gl2tftzbst02cc40000gn/T/build_workspace_5e91481e47f92248ad5178315cc98471/vendor/symfony/http-foundation/Session/Storage/Proxy/AbstractProxy.php:94

{"db_prefix":"test37124925","user_agent":"simpletest37124925:1567450397:5d6d651de85848.28066836:iG2f4psQ0MOufHSECa_06xUus3tjpje_svP2sT1oIr8","site_path":"sites\/simpletest\/37124925"}

Re: #106 Thanks. :-) We'll have these patches as starting points when we get to actual test implementations, assuming they're needed at all.