Write tests that ensure hook_update_N is properly run

1. +++ b/core/modules/system/src/Tests/Update/UpdatePathTestBase.php
   @@ -0,0 +1,359 @@
   +    // We are going to set a missing zlib requirement property for usage
   +    // during the performUpgrade() and tearDown() methods. Also set that the
   +    // tests failed.
   +    if (!$this->zlibInstalled) {
   +      parent::setUp();
   +      return;
   +    }
   

   So does this throws an exception somehow? I'm curious what happens on hosts without zlib

2. +++ b/core/modules/system/src/Tests/Update/UpdatePathTestBase.php
   @@ -0,0 +1,359 @@
   +
   +    // Prepare installer settings that are not install_drupal() parameters.
   +    // Copy and prepare an actual settings.php, so as to resemble a regular
   +    // installation.
   +    // Not using File API; a potential error must trigger a PHP warning.
   +    $directory = DRUPAL_ROOT . '/' . $this->siteDirectory;
   +    copy(DRUPAL_ROOT . '/sites/default/default.settings.php', $directory . '/settings.php');
   +    copy(DRUPAL_ROOT . '/sites/default/default.services.yml', $directory . '/services.yml');
   +
   +    // All file system paths are created by System module during installation.
   +    // @see system_requirements()
   +    // @see TestBase::prepareEnvironment()
   +    $settings['settings']['file_public_path'] = (object) array(
   +      'value' => $this->publicFilesDirectory,
   +      'required' => TRUE,
   +    );
   +    $settings['settings']['file_private_path'] = (object) array(
   +      'value' => $this->privateFilesDirectory,
   +      'required' => TRUE,
   +    );
   +    // Save the original site directory path, so that extensions in the
   +    // site-specific directory can still be discovered in the test site
   +    // environment.
   +    // @see \Drupal\Core\Extension\ExtensionDiscovery::scan()
   +    $settings['settings']['test_parent_site'] = (object) array(
   +      'value' => $this->originalSite,
   +      'required' => TRUE,
   +    );
   +    // Add the parent profile's search path to the child site's search paths.
   +    // @see \Drupal\Core\Extension\ExtensionDiscovery::getProfileDirectories()
   +    $settings['conf']['simpletest.settings']['parent_profile'] = (object) array(
   +      'value' => $this->originalProfile,
   +      'required' => TRUE,
   +    );
   +    $settings['settings']['apcu_ensure_unique_prefix'] = (object) array(
   +      'value' => FALSE,
   +      'required' => TRUE,
   +    );
   +    // Remember the profile which was used.
   +    $settings['settings']['install_profile'] = (object) array(
   +      'value' => $this->installProfile,
   +      'required' => TRUE,
   +    );
   +
   +    // Generate a hash salt.
   +    // @todo should this be part of the exported database somehow?
   +    $settings['settings']['hash_salt'] = (object) array(
   +      'value'    => Crypt::randomBytesBase64(55),
   +      'required' => TRUE,
   +    );
   +
   +    // Since the installer isn't run, add the database settings here too.
   +    $settings['databases']['default'] = (object) array(
   +      'value' => Database::getConnectionInfo(),
   +      'required' => TRUE,
   +    );
   +
   

   I see some similarity to WebTestBase ... is there a sane way to extract things in a method so you don't have to duplicate everything? If you say its too complex and would make the patch way harder, don't do it, but I'm just curious here.

So ToolbarCacheContextsTest already has a protected installModules with a different signature.

So I've renamed it to installExtraModules() because the use-case is different.

Can you add the interdiff on top of your branch @jhedstrom and add a new do-not-test patch?

So ToolbarCacheContextsTest already has a protected installModules with a different signature.

Oh that is tricky :)

Since this is a sub-issue of #2447573: [meta] Make sure 8.x - 8.x hook_update_N() testing is possible, which is a D8 upgrade path blocker, tagging as such.

Keep up the fantastic work! :D

More metadata adjustment. Almost done, I swear...

Great work so far! Some small points.

It adds quite some test coverage, which is great!

1. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -880,6 +892,92 @@ protected function installParameters() {
   +  /**
   +   * Install modules defined by `static::$modules`.
   +   */
   +  protected function installModules(Container $container) {
   

   What about name it a bit more specific? installModulesFromClassProperty?

2. +++ b/core/modules/system/src/Tests/Update/UpdatePathTestBase.php
   @@ -0,0 +1,196 @@
   +  /**
   +   * Overrides WebTestBase::setUp() for upgrade testing.
   +   *
   +   * @see \Drupal\simpletest\WebTestBase::prepareDatabasePrefix()
   +   * @see \Drupal\simpletest\WebTestBase::changeDatabasePrefix()
   +   * @see \Drupal\simpletest\WebTestBase::prepareEnvironment()
   +   */
   

   Do you mind expanding the comment describing what you changed from the original?

3. +++ b/core/modules/system/src/Tests/Update/UpdatePathTestBase.php
   @@ -0,0 +1,196 @@
   +    // Set the update url.
   +    $this->updateUrl = Url::fromRoute('system.db_update');
   +
   

   Should we do that maybe in the constructor?

4. +++ b/core/modules/system/src/Tests/Update/UpdatePathTestBase.php
   @@ -0,0 +1,196 @@
   +   * Add settings that are missed since the installer isn't run
   

   Nitpick: Missing dot at the end

5. +++ b/core/modules/system/src/Tests/Update/UpdatePathTestBase.php
   @@ -0,0 +1,196 @@
   +    // @todo should this be part of the exported database somehow?
   +    $settings['settings']['hash_salt'] = (object) [
   +      'value'    => Crypt::randomBytesBase64(55),
   +      'required' => TRUE,
   +    ];
   

   Well its a setting so it can't be. I think this is fine

6. +++ b/core/modules/system/src/Tests/Update/UpdatePathTestBase.php
   @@ -0,0 +1,196 @@
   +    $this->drupalGet($this->updateUrl, ['external' => TRUE]);
   
   +++ b/core/modules/system/src/Tests/Update/UpdateSchemaTest.php
   @@ -0,0 +1,71 @@
   +    $this->updateUrl = Url::fromRoute('system.db_update');
   ...
   +    $this->drupalGet($this->updateUrl, ['external' => TRUE]);
   

   Do you need it to be external? I think this should work without it

7. +++ b/core/modules/system/src/Tests/Update/UpdatePathTestBaseTest.php
   @@ -0,0 +1,56 @@
   +namespace Drupal\system\Tests\Update;
   +use Drupal\Component\Utility\SafeMarkup;
   

   Nitpick: newline

8. +++ b/core/modules/system/src/Tests/Update/UpdatePathTestBaseTest.php
   @@ -0,0 +1,56 @@
   +  protected function setUp() {
   +    $this->databaseDumpFiles = [__DIR__ . '/../../../tests/fixtures/update/drupal-8.bare.standard.php.gz'];
   +    parent::setUp();
   +  }
   

   Isn't it possible to put all that into a property on the object?

9. +++ b/core/modules/system/src/Tests/Update/UpdatePathTestBaseTest.php
   @@ -0,0 +1,56 @@
   +    $this->databaseDumpFiles = [__DIR__ . '/../../../tests/fixtures/update/drupal-8.bare.standard.php.gz'];
   

   Should we add a suffix like beta_11, so its clear at which point this file got generated? Especially for system I could image that new update_N hooks are created

10. +++ b/core/modules/system/src/Tests/Update/UpdatePathTestBaseTest.php
    @@ -0,0 +1,56 @@
    +  protected function testDatabaseLoaded() {
    ...
    +  protected function testUpdateHookN() {
    

    Let's make them public ... technically protected works but it won't in case you would use phpunit as test runner

11. +++ b/core/modules/system/src/Tests/Update/UpdatePathTestBaseTest.php
    @@ -0,0 +1,56 @@
    +    $this->assertEqual(\Drupal::config('system.site')->get('name'), 'Site-Install');
    +    $this->drupalGet('<front>');
    +    $this->assertText('Site-Install');
    

    Do you think we can test a bit more here, is it worth to do?

12. +++ b/core/modules/system/src/Tests/Update/UpdateSchemaTest.php
    @@ -0,0 +1,71 @@
    +  function testUpdateHooks() {
    

    Let's make it a public function

13. +++ b/core/modules/system/tests/modules/update_test_schema/update_test_schema.install
    @@ -0,0 +1,41 @@
    +$next = \Drupal::state()->get('update_test_schema_version', 8000) + 1;
    +
    +if ($next > 8001) {
    

    What about not using +1 but name the variable $schema_version and check for >= 8001?

Thank you for your quick response!

Sadly your interdiff.txt is empty ... I'll just trust you :P

#28.8 Oddly it isn't possible to use __DIR__ when defining a class variable

Oh PHP!

I didn't add a suffix, but added .beta11. in the middle--I wonder if the script itself should be regenerated from beta11 instead of current HEAD?

I think for now its fine to use HEAD. Mh, maybe we should use the git commit hash instead? Do other people have an opinion about it?
For now this feels good enough though.

Testing the homepage was a very easy way to ensure the db was properly loaded and the site was working (I used this test while working on all the code in setUp(). I think it's worth leaving.

Oh yeah I think testing that for example config worked is not a bad thing at all.

Thank you!

I think someone with more knowledge of simpletest should have a look at it, but the split up of the setUp() method seemed to be reasonable for me.
Let's RTBC so alex can't have an excuse to not look at it :P

On the beta11 filename thing, in D7 these dump files weren't versioned. Which IMO is a good thing because we would now have at least 37 * 2 * 2 = 148 copies kicking around (one for each version of D7 * number of install profiles * filled/base), the 7.x-dev tarball would be ginormous. :(

We also do not seem to have any process for updating those dump files, since e.g. http://cgit.drupalcode.org/drupal/log/modules/simpletest/tests/upgrade/d... has not been updated since its initial commit in 2011, despite over 30 releases since then.

So I'm curious where this is coming from, since it's effectively a "new" feature to the upgrade test system to keep versioned database dumps, and not a feature I'm sure we want. What I would expect in D8 is prior to tagging each new minor version we'd just re-dump from the previous minor, and overwrite the existing file's contents (drupal-8.bare.standard.php.gz or whatever). There's no need IMO for core to support an upgrade path from 8.0.0 => 8.5.0 (or beta10 => beta14 or whatever) directly.

On the beta11 filename thing, in D7 these dump files weren't versioned. Which IMO is a good thing because we would now have at least 37 * 2 * 2 = 148 copies kicking around (one for each version of D7 * number of install profiles * filled/base), the 7.x-dev tarball would be ginormous. :(

I'm confused, we don't talk about making snapshots more often, but well, we can just ignore that part :|

We also do not seem to have any process for updating those dump files, since e.g. http://cgit.drupalcode.org/drupal/log/modules/simpletest/tests/upgrade/d... has not been updated since its initial commit in 2011, despite over 30 releases since then.

To be clear, there is no point in updating them. At some point you create the dump, for which you target for the hook_update_N function.

On database dump versioning:

In Drupal 7, we have two basic tests:

1. Test a full update from Drupal 6-7. Each time a new update function is added to Drupal 7, that would be covered implicitly. i.e. if you drop a Drupal 6 database into a Drupal 7 installation, hit update.php, then every update for any module in the D6 db will be run.

2. Test 'some version of Drupal 7' to 'the latest version of Drupal 7'. This will also test all the hook_update_N() added after the schema version of that dump.

There is a use case for versioned database dumps, but it's very limited:

Beta 12 is released. There is a new regression in the the schema (or actual data is corrupted) and an upgrade path is added which updates beta < 11 sites to the bad schema/data.

Beta 13 is released, the regression is fixed, and we add an upgrade path to migrate sites with the bad beta 12 schema.

Beta < 11 site, skips beta 12 and updates straight to beta 13 - never has the bad schema.
Beta < 11 site, updates to beta 12, then to beta 13 etc. - updates to, then away from , the bad schema
New install on beta 12 - starts off with the bad schema.

The upgrade path added in Beta 13 then has to differentiate between databases which have the schema defect, vs. ones that don't.

This isn't a theoretical situation - we had exactly such a bad update in the Drupal 6-7 cycle (at least one) which was a nightmare to fix.

Another issue is if we start to drop support for direct updates from older versions. For example we might not support upgrading Drupal 8.0.0-beta14 all the way to Drupal 8.4.6 - we might instead say you have to go 8.0.0 0 -> 8.1.6 -> 8.2.6 -> 8.3.6 -> 8.4.6 sequentially.

In such a case, the old database dump will start getting hook_update_last_removed() errors and we'd need a new one. But then we'd probably not keep the old one around (except to assert that error possibly).

In general we can assume that sites have identical schemas when they update between releases, so the 'starting' database version is OK and as we add more updates they'll be run on top of the old ones - until it isn't.

However documenting that the 'starting' dump comes form a particular version in the naming seems OK to me. But also since we don't support updating from the beta, we'll probably need to keep updating it until we get to release candidate, then stick from then onwards unless something comes up.

Leaving RTBC for now, but we're so, so out of practice having to deal with this, it'd be good to ensure everyone's on the same page.

1. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -733,15 +767,15 @@ protected function setUp() {
   +   * @param \Symfony\Component\DependencyInjection\Container $container
   +   */
   +  protected function initConfig(Container $container) {
   

   ContainerInterface.

2. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -773,34 +807,12 @@ protected function setUp() {
   +   * Reset and rebuild the environmentl after setup.
   

   environmentl

3. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -880,6 +892,92 @@ protected function installParameters() {
   +   * @param \Symfony\Component\HttpFoundation\Request $request
   +   *   Request object.
   +   * @return \Symfony\Component\DependencyInjection\ContainerInterface
   

   Missing an empty line between @param and @return

4. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -880,6 +892,92 @@ protected function installParameters() {
   +  /**
   +   * Install modules defined by `static::$modules`.
   +   */
   +  protected function installModulesFromClassProperty(Container $container) {
   

   Missing @param and should be ContainerInterface

The branch maintainers agreed this is obviously critical, so tagging.

+++ b/core/modules/simpletest/src/WebTestBase.php
@@ -773,9 +773,9 @@ protected function initSettings() {
-   * @param \Symfony\Component\DependencyInjection\Container $container
+   * @param \Symfony\Component\DependencyInjection\ContainerInterface $container
    */

@@ -952,8 +956,10 @@ protected function initKernel(Request $request) {
+   * @param \Symfony\Component\DependencyInjection\ContainerInterface $container
    */

Let's put a one-line description on there

The feedback from alex and myself got adressed

OK I'm happy with this, so went ahead and committed/pushed to 8.0.x, thanks!

We can revisit the database dump naming since we'll need a fresh database dump from the day we start supporting the upgrade path (next beta or the one after most likely).

Just a heads up, importing the beta11 schema with the patch in #1314214: MySQL driver does not support full UTF-8 (emojis, asian symbols, mathematical symbols) applied gives us some test failures as utf8mb4 support requires schema updates. Just wondering how we'd best deal with this?