Impossible to generate meaningful diffs of upgrade db dumps

I'm making the (safe) assumption that upgrade path tests are ran by core developers and testbots only.

This means that we have a D8 git clone readily at hand.

We can leverage it.

Instead of doing database dumps, we can install D7, configure it, and upgrade it - and on top of that, we can do all of that in a clean environment that is not faked at all (contrary to now).

Attached patch is by far the most advanced and most weird testing system patch I ever worked on. :)

Benefits:

- Removes the need to create, update, maintain db dumps.
- Is always up to date. It always tests latest D7 core -> D8 upgrade path.

This is crazy stuff, I like it al lot! Made the couple small fixes below. Let's see if now tests pass.

+++ b/core/modules/simpletest/drupal_web_test_case.php
@@ -1272,31 +1272,19 @@ class DrupalWebTestCase extends DrupalTestCase {
+      $this->prepareDatabasePrefix;

Missing parentheses :)

+++ b/core/modules/simpletest/drupal_web_test_case.php
@@ -1307,12 +1295,24 @@ class DrupalWebTestCase extends DrupalTestCase {
+    // Save and clean shutdown callbacks array because it static cached and
+    // will be changed by the test run. If we don't, then it will contain
+    // callbacks from both environments. So testing environment will try
+    // to call handlers from original environment.

This comment needs some clean-up

+++ b/core/modules/simpletest/tests/upgrade/upgrade.test
@@ -331,3 +331,250 @@ abstract class UpgradePathTestCase extends DrupalWebTestCase {
+    // @todo ¶

Trailing whitespace

-2 days to next Drupal core point release.

+++ b/.htaccess
@@ -98,11 +98,9 @@ DirectoryIndex index.php index.html index.htm
   # Redirect common PHP files to their new locations.
-  RewriteCond %{REQUEST_URI} ^(.*)?/(update.php) [OR]
-  RewriteCond %{REQUEST_URI} ^(.*)?/(install.php) [OR]
-  RewriteCond %{REQUEST_URI} ^(.*)?/(cron.php)
-  RewriteCond %{REQUEST_URI} !core
-  RewriteRule ^ %1/core/%2 [L,QSA,R=301]
+  RewriteCond %{REQUEST_URI} ^/((update|install|cron)\.php)
+  RewriteCond %{REQUEST_FILENAME} !-f
+  RewriteRule ^ /core/%1 [L,QSA,R=301]

Drupal installation likely fails due to these changes. However, they're working for me.

The changes are required, because install|update|cron.php are also rewritten anywhere in the filesystem, so it's impossible to request install.php of the D7 checkout in the public filesystem.

That said, I couldn't resist and tried to optimize the rewrite conditions. Merely adding the RewriteCond that checks whether the file does not exist should be sufficient...

+++ b/core/modules/simpletest/tests/upgrade/upgrade.test
@@ -331,3 +331,250 @@ abstract class UpgradePathTestCase extends DrupalWebTestCase {
+   * @todo Switch to Minimal.

I only had to use Standard, because performUpgrade() checks for the "Powered by Drupal" block at its end. We should find a better assertion there, so we can use the Minimal profile by default.

+++ b/core/modules/simpletest/tests/upgrade/upgrade.test
@@ -331,3 +331,250 @@ abstract class UpgradePathTestCase extends DrupalWebTestCase {
+  protected $oldProfile = 'standard';

Didn't have the passion to think about a proper property name, but this is really a poor and wonky name. ;)

+++ b/core/modules/simpletest/tests/upgrade/upgrade.test
@@ -331,3 +331,250 @@ abstract class UpgradePathTestCase extends DrupalWebTestCase {
+  /**
+   * Original cURL options backup.
+   *
+   * Drupal doesn't allow to install Drupal with a user agent containing
+   * "simpletest", so we need to temporarily switch it to something else.
+   */
+  protected $originalAdditionalCurlOptions = array();
...
+    // install.core.inc actively prevents a user agent containing "simpletest"
+    // to install Drupal.  DrupalWTF!
+    $this->originalAdditionalCurlOptions = $this->additionalCurlOptions;
+    $this->additionalCurlOptions[CURLOPT_USERAGENT] = 'Drupal';
...
+    // Revert everything we changed.
+    $this->additionalCurlOptions = $this->originalAdditionalCurlOptions;

Let's please open a separate issue for that. This is the most stupid "security" check I've ever seen. And as you can see, one can easily hack around it...

+++ b/core/modules/simpletest/tests/upgrade/upgrade.test
@@ -331,3 +331,250 @@ abstract class UpgradePathTestCase extends DrupalWebTestCase {
+    // Setup verbose files directory etc.
+    // @todo Hm. We only need prepareEnvironment(), but without a databasePrefix
+    //   the test batch throws fatal errors, as it tries to access the original
+    //   db connection.
+    $this->prepareDatabasePrefix();
+    $this->prepareEnvironment();

It is still not 100% clear to me as to why prepareDatabasePrefix() is required for prepareEnvironment().

We need to know why.

In any case, this likely means that every web test needs to update the databasePrefix for the test_id, so the prepareDatabasePrefix() code can likely be moved into DrupalWebTestCase::__construct().

+++ b/core/modules/simpletest/tests/upgrade/upgrade.test
@@ -331,3 +331,250 @@ abstract class UpgradePathTestCase extends DrupalWebTestCase {
+  protected function prepareCheckout() {
...
+    if (!is_dir("$this->upgrade_root/.git")) {
+      // @todo We actually don't need the .git repo - a simple export would be
+      //   sufficient. As soon as D7 is installed + configured, we can change
+      //   the DB prefix to the one of the parent site and perform the upgrade.
+      debug(`cp -R .git {$this->upgrade_root}/`);

Does anyone know whether git is able to export a different branch (not the currently checked out)?

In fact, we don't really need the .git repo - once the D7 code exists, it's ready for usage and can even be retained for all upgrade tests.

+++ b/core/modules/simpletest/tests/upgrade/upgrade.test
@@ -331,3 +331,250 @@ abstract class UpgradePathTestCase extends DrupalWebTestCase {
+    // Revert public filesystem security.
+    $htaccess_file = $this->upgrade_root . '/.htaccess';
+    $htaccess = file_get_contents($htaccess_file);
+    if (strpos($htaccess, 'SetHandler') === FALSE) {
+      $htaccess = 'SetHandler none
+Options All
+' . $htaccess;
+      file_put_contents($htaccess_file, $htaccess);
+    }

It's really sad that file_put_contents() doesn't have a FILE_PREPEND flag ;)

Anyway, this could be slightly cleaned up to make it more readable and clear that we're rewriting the core .htaccess file in the D7 root and prepending some directives to revert security directives being set higher up in the filesystem tree.

Also, the "Options All" is required, but may break in other environments, as you're not allowed to set more Options than globally limited on the server. I'm not sure which Options are actually required to execute PHP.

Merely testing the reduced .htaccess changes for the testbot, no other changes from #7.

And yes; I intentionally keep trailing white-space after a @todo that absolutely needs to be resolved before commit, and I do not use any automation for removing trailing white-space ;)

This is a brilliant idea. I wholly approve as well. I just tried running the test on my own server and I seem to end up with the same set of issues that are coming up on trying to get to http://localhost/sites/default/files/upgrade/install.php which redirects off to http://localhost/sites/default/files/upgrade/core/install.php. Since it doesn't exist, it gives an error and all other tests seem to fail after that point (and end up with table not found fatal errors). I'm trying to figure out where the issue could be coming from (hopefully not my setup) and update once I figure it out.

So, reverting the changes from #7 made the Drupal installer work again, which means that the original changes were bogus.

However, the mere addition of testing whether the requested file exists does not seem to be sufficient either - or - potentially tests the wrong file.

Essentially, those .htaccess lines need to be rewritten, so they only apply to the top-level / root directory of Drupal, but the rewrite rule is not applied to any other requested file deeper down in the filesystem.

In other words:

Rewrite this:

But do NOT rewrite this:

Apparently, I forgot most of my mod_rewrite fu over the past months, so I'd be glad if someone could help out here.

@sun, I finally figured out things that were happening on my end. When I tried to actually do a git clone in php, it would not execute. The reason was it did not know where git was (on osx, it is stored at /usr/local/git/bin/git for me, which I will guess is not part of its regular places but this may need to get documented somewhere). So the install was not actually coming in. Once I set that up, things started to get weird. I'm not sure what I had done but I had it mostly working at one point. However, the default behavior at this stage with your patch is that the htacess rules don't need to change; they seem to be working just fine (sethandler none seems to do the trick). What happens now though is that I get shown a 'no profiles available'. I'm trying to figure out why this is occurring and will update if I find anything.

Ok, so I figured it out; the issue seems to stem from
git --git-dir={$this->upgrade_root}/.git --work-tree={$this->upgrade_root} checkout origin/7.x. For some reason, the .profile files are not pulled in. I changed the code so it actually ran git clone (the test is much slower) and that seemed to pass all but the last 2 tests (which then falls off into a fatal error). One other part that I had to modify was the mysql info. I'm wondering if we could copy default.settings.php to settings.php and then at the bottom append something like the following:

$settings_file .= "$databases['default']['default'] =" var_export($connection_info['default']) .";
$databases['default']['default']['prefix'] = ". $connection_info['default']['prefix']['default'] . $this->databasePrefix .";";

Anyways, I'm attaching a patch file along with the interdiff to show its current status (which is mostly green except fails at the requirement of the user module near the end which I suspect might be some other issue).

I'm not sure why it fails where it does. Perhaps the settings for mysql need to be changed? Anyways, I get past that point and the error I run into is:

require_once(/Users/amodi/Sites/d8_dev/modules/user/user.module): failed to open stream: No such file or directory

Somehow, I suspect the code registry gets cached and that is the cause of the issue. I'm wondering if there is a way we can set for caching to be wholly disabled for testing.

I'm not able to get around this require_once fatal error that pops up. Its strange because I am able to do *everything* inside installDrupal (I can actually access update.php just fine in there) but there is some spot along the way where things mess up and I can't figure out where :/

However, I saw you had a TODO regarding database settings and I had an idea for that. What if we create the settings.php file in the first place and append the actual information in there at the end of the file? I also changed the caching backend to be DrupalFakeCache so we ensure nothing gets cached (or do - easy enough to modify). I tested it out with mysql and it seems to work quite well so I'm attaching the patch for that. Also attaching an interdiff so you see the difference.

This patch seems to leave sites/default/files/upgrade/sites/default unwriteable, meaning that the testbot can't clear the checkout directory afterwards and the testbot begins to malfunction. I opened #1436412: chmod -R u+w checkout directory before clearing, but would it be possible for the patch to not do that in the meantime?

I'm sorry for that :( The reason this occurs is because when the d7 site is installed, it changes the settings for the default directory. And so its happening automatically in this case. I'm attaching a patch that forces the permissions to change after install. I've tested it a couple of times and it seems to be working. Though could certainly use a lot more of a review and hopefully doesn't mess around with the testbots.

make testbot active.

I changed the use of debug() a little so other testers see the STD_ERR output.

@BTMash I too failed to make apache find git (on Mac) so please update the issue summary to tell us what to do. I fixed it by setting full path /usr/local/git/bin/git in prepareCheckout() but this should be a config of apache somehow right?

My simpletest hanged so teardown was not executed.

Why are we not doing a local clone from the D8 checkout?
- would be faster for local testers. (no network downloads)
- give local testers the possibility to change local D7 and D8 branches. IE checkout 8.2 and 7.4 (not sure how to prepare the 7.4)
- contrib testers can use this for sure

What are the ideas for contrib testing?

@clemens.tolboom, the way I ended up doing this on my computer was actually adding a symlink for git at /usr/bin (or it can be at /usr/local/bin). I believe the other way is you can set the PATH variable (where php can find files) or change settings.php and set an include path (see http://php.net/manual/en/function.set-include-path.php). Anyways, to answer your question re: doing a local clone. I was initially having issues with it and once I changed it to the checkout from d.o, it seems to mitigate the issues I was having. But if we're able to get it to work cleanly off the local clone, I'm all for it :) However, do note that since the tests are actually included with when people download the stable versions (which are not packaged with the git history), then this could lead to issues if they run tests on their servers based off that (I'm not sure of who actually does it in practice but its something to strongly consider).

I was able to partially narrow down that the issue was occurring when drupal_save_session(FALSE) is called. When I removed it (and did a git checkout to 8.x for that local test), most of the upgrade actually seems to work (I make calls for the user to log in / whanot at that stage).

We want to test the latest code of the previous version, not a stable release. Ideally, we want to export that code from the local .git directory.

I just updated the issue summary and added the path to git. Next

drush test-run BareUpgradePathTestCase --verbose --debug

which [edit] failed to [/edit] download git.

[edit]removed rest[/edit]

Updated issue summary.

Test run with a hardwired path to git now failed though the webbrowser gives a failure on

Entity module enabled after upgrade.
Other
upgrade_bare.test
24
BareUpgradePathTestCase->testBareUpgrade()

drush test-run BareUpgradePathTestCase --verbose --debug

Failed on Drupal 8 codebase err.

<h1>Additional uncaught exception thrown while handling exception.</h1><h2>Original</h2><p>PDOException: SQLSTATE[42S02]: Base table or view not found: 1146 Table &amp;#039;drupal_d8.simpletest679881node_type&amp;#039; doesn&amp;#039;t exist: SELECT nt.*
FROM 
{node_type} nt
WHERE  (disabled = :db_condition_placeholder_0) 
ORDER BY nt.type ASC; Array
(
    [:db_condition_placeholder_0] =&amp;gt; 0
)
 in _node_types_build() (line 717 of /Users/clemens/Sites/drupal/d8/www/core/modules/node/node.module).</p><h2>Additional</h2>

This is weird. It is different from #15 so what am I missing?

I refactored the shell_exec flow. Made the commands single step which helps debugging. I documented a few methods.

I added a few TODOs as the shell execs should be written in php functions to make this work on windows.

Debugging through the code the final assertion fails.

    // Ensure that the new Entity module is enabled after upgrade.
    $this->assertTrue(module_exists('entity'), 'Entity module enabled after upgrade.');

Why should entity module be enabled? The fields module is disabled in the setup.

  protected function uninstallModulesExcept(array $modules) {
    $required_modules = array('block', 'dblog', 'filter', 'node', 'system', 'update', 'user');

Below are all listed enabled modules.

mysql> select name, status, schema_version from simpletest943076system where status =1;
+-------------------+--------+----------------+
| name              | status | schema_version |
+-------------------+--------+----------------+
| block             |      1 |           8000 |
| color             |      1 |           7001 |
| comment           |      1 |           8000 |
| contextual        |      1 |              0 |
| dashboard         |      1 |              0 |
| entity            |      1 |              0 |
| field             |      1 |           7001 |
| field_sql_storage |      1 |           7002 |
| field_ui          |      1 |              0 |
| file              |      1 |              0 |
| filter            |      1 |           7010 |
| help              |      1 |              0 |
| image             |      1 |           7004 |
| list              |      1 |           7002 |
| menu              |      1 |           7003 |
| node              |      1 |           8001 |
| number            |      1 |              0 |
| options           |      1 |              0 |
| path              |      1 |              0 |
| rdf               |      1 |              0 |
| search            |      1 |           7000 |
| shortcut          |      1 |              0 |
| system            |      1 |           8002 |
| taxonomy          |      1 |           8000 |
| text              |      1 |           7000 |
| toolbar           |      1 |              0 |
| user              |      1 |           8000 |
| standard          |      1 |              0 |
| bartik            |      1 |             -1 |
| seven             |      1 |             -1 |
| dblog             |      1 |           7001 |
| overlay           |      1 |              0 |
+-------------------+--------+----------------+
32 rows in set (0.00 sec)

Followed the spirit of http://xjm.drupalgardens.com/blog/rerolling-drupal-8-core-patches

Reapplied patch attached.

Fixed the path to git.

Replaced tasks.

I haven't had a chance to review your code change (though first off, thank you!) but regarding your question on why entity should be enabled, it should be enabled if node is enabled (node depends on entity).

Code fails now on finding UserController

function entity_get_controller($entity_type) {
  $controllers = &drupal_static(__FUNCTION__, array());
  if (!isset($controllers[$entity_type])) {
    $type_info = entity_get_info($entity_type);
    $class = $type_info['controller class'];
    $controllers[$entity_type] = new $class($entity_type);
  }
  return $controllers[$entity_type];
}

called from upgrade.test

UpgradePathNGTestCase::setup() {
...
    $user = user_load(1);
}

I've had a chance to test out your code and I run into an issue at the same spot that the testbot seems to run into the issue. All I did was add a drupal_flush_all_caches() and the tests are able to resume. So I am adding the updated patch. However, the issue that I still see coming up is that somehow the installation does not end up using the prefix when the update.php function is called. So it seems like the site update page gets called but its on the default connection without the simpletest prefix. It tries to run update.php but there is nothing to update (or if there was, it would essentially be a 8.x -> 8.x update) and when the module_exists('entity') checks on entity being enabled come up, it is not because the simpletest prefix does not have it enabled. I'm trying to resolve how to ensure the simpletest prefix version gets called for these tests.

Adding to needs review just to make sure the test runs up to the entity check.

Forgot to set to 'needs review'. Ok, I was wrong - the updates actually *do* occur. So I'm not entirely sure on why module_exists is not providing the correct information. Going to delve into this a bit more.

Hmm, I'm not sure what to do about this error; not sure where the testbot error is coming from.

However, I was able to resolve why entity was not showing up as enabled. The problem actually stems from module_list() (see http://api.drupal.org/api/drupal/core!includes!module.inc/function/modul...). From the link, you can see that module_list actually has its own static module list array and is not using drupal_static to hold the variables. That is why when the static variables are reset, this is not flushed out as well. After the upgrade is performed, I make a call module_list(TRUE) which rebuilds the module list and after that the tests pass. I will have a patch with that call and hopefully the testbot behaves; but I do think as a longer term with this patch that module_list should also call on drupal_static and use the variable from there so it can be easily flushed when needed.

Here is a version that uses drupal_static in module_list. I'm not entirely sure if I have implemented the drupal_static portion correctly since I've not used it before but the tests pass on my local installation.

The patch in #34 was a testbot problem.

Oops, crosspost.

Wow, that failed non-upgrade tests! For now then, here is a patch that just calls on module_list(TRUE)

Since the test is failing on the check to update.php, I added in a variable_set for when to bypass the update.php when doing the drupal_valid_ua_test() check.

Set to 'needs review'.

I'm still awaiting a review (and getting ripped apart ^_~) but since this has worked out against the testbot, the new patch also attempts to tackle 8.x -> 8.x upgrades (right now, it is does on a bare install but much like with 7.x, hopefully we'll be able to tackle a lot more :)) so we have a chance to test 8.x upgrades against itself.

Uhoh, you asked for it! :D A quick code style and docs review incoming. Let's get these out of the way to help us see the good stuff underneath. :)

1. +++ b/core/modules/simpletest/drupal_web_test_case.phpundefined
   @@ -1272,31 +1272,19 @@ class DrupalWebTestCase extends DrupalTestCase {
   +  protected function prepareDatabasePrefix() {
   ...
   +  protected function changeDatabasePrefix() {
   
   @@ -1307,12 +1295,24 @@ class DrupalWebTestCase extends DrupalTestCase {
   +  protected function prepareEnvironment() {
   
   +++ b/core/modules/simpletest/tests/upgrade/upgrade.testundefined
   @@ -331,3 +331,482 @@ abstract class UpgradePathTestCase extends DrupalWebTestCase {
   +  protected function installDrupal() {
   

   These guys all need docblocks.

2. +++ b/core/modules/simpletest/drupal_web_test_case.phpundefined
   @@ -1325,30 +1325,46 @@ class DrupalWebTestCase extends DrupalTestCase {
   +   * Generates a random database prefix, runs the install scripts on the
   +   * prefixed database and enable the specified modules. After installation
   +   * many caches are flushed and the internal browser is setup so that the
   +   * page requests will run on the new prefix. A temporary files directory
   +   * is created with the same name as the database prefix.
   

   We should have a one-line summary above this paragraph that explains what the function does. I guess in this case it might be simply "Overrides FooClass::setUp()"? Usually, we don't have docblocks for the setUp methods, but I think upgrade path tests are complex enough that we should. These certainly aren't any old test case.

3. +++ b/core/modules/simpletest/tests/upgrade/upgrade.testundefined
   @@ -331,3 +331,482 @@ abstract class UpgradePathTestCase extends DrupalWebTestCase {
   + * Perform end-to-end tests of the upgrade path.
   ...
   +   * Original cURL options backup.
   ...
   +   * Specialized variable_set() that works even if the child site is not upgraded.
   ...
   +   * Perform the upgrade.
   ...
   +   * Force uninstall all modules from a test database, except those listed.
   ...
   + * Perform end-to-end tests of the upgrade path.
   
   +++ b/core/modules/simpletest/tests/upgrade/upgrade_bare.testundefined
   @@ -5,21 +5,38 @@
   +   * Test a successful upgrade.
   ...
   + * Minor Upgrade test for the bare database..
   

   These guys should all start with a third-person verb ("Performs," "Forces," etc.) Also mind the 80-character limit; let's reword one or two to get these down to 80 chars.

4. +++ b/core/modules/simpletest/tests/upgrade/upgrade.testundefined
   @@ -331,3 +331,482 @@ abstract class UpgradePathTestCase extends DrupalWebTestCase {
   +   * Executes a (list of) command(s) and debugs its result.
   

   I'd say "the" result here. (Way trivial.)

5. +++ b/core/modules/simpletest/tests/upgrade/upgrade.testundefined
   @@ -331,3 +331,482 @@ abstract class UpgradePathTestCase extends DrupalWebTestCase {
   +    if (strpos($htaccess, 'SetHandler') === FALSE) {
   +      $htaccess = 'SetHandler none
   +Options All
   +'. $htaccess;
   +      file_put_contents($htaccess_file, $htaccess);
   +    }
   

   The formatting here is kind of weird. Can we use \n instead? ( I actually have no idea if this is preferred or not, actually.)

6. +++ b/core/modules/simpletest/tests/upgrade/upgrade.testundefined
   @@ -331,3 +331,482 @@ abstract class UpgradePathTestCase extends DrupalWebTestCase {
   +  ¶
   +      // Check for errors during the update process.
   +      foreach ($this->xpath('//li[@class=:class]', array(':class' => 'failure')) as $element) {
   +        $message = strip_tags($element->asXML());
   +        $this->upgradeErrors[] = $message;
   +        if ($register_errors) {
   +          $this->fail($message);
   +        }
   +      }
   +  ¶
   +      if (!empty($this->upgradeErrors)) {
   +        // Upgrade failed, the installation might be in an inconsistent state,
   +        // don't process.
   +        return FALSE;
   +      }
   +    }
   +  ¶
   

   Trailing whitespace on the blank lines through here.

7. +++ b/core/modules/simpletest/tests/upgrade/upgrade.testundefined
   @@ -331,3 +331,482 @@ abstract class UpgradePathTestCase extends DrupalWebTestCase {
   +    // Reload module list. For modules that are enabled in the test database,
   +    // but not on the test client, we need to load the code here.
   +//    $new_modules = array_diff(module_list(TRUE), $this->loadedModules);
   +//    foreach ($new_modules as $module) {
   +//      drupal_load('module', $module);
   +//    }
   

   Hrm, commented-out code?

8. +++ b/core/modules/simpletest/tests/upgrade/upgrade.testundefined
   @@ -331,3 +331,482 @@ abstract class UpgradePathTestCase extends DrupalWebTestCase {
   +    // to install Drupal.  DrupalWTF!
   

   Probably should remove the DrupalWTF. ;)

9. +++ b/core/modules/simpletest/tests/upgrade/upgrade.testundefined
   @@ -331,3 +331,482 @@ abstract class UpgradePathTestCase extends DrupalWebTestCase {
   +   * - mixing drush and apache is mixing access control attributes
   

   Missing a period here.

10. +++ b/core/modules/simpletest/tests/upgrade/upgrade.testundefined
    @@ -331,3 +331,482 @@ abstract class UpgradePathTestCase extends DrupalWebTestCase {
    +}
    \ No newline at end of file
    

    Missing newline at the end of upgrade.test.

And, of course, there are all the pending @todo. :)

Making these changes to the patch would make a great novice task--so totally up to you whether you'd prefer to make the changes yourself or leave it to the novice queue. Tagging.

First on the test. Hmm, that is a strange behavior from the test on not removing the .git directory. Perhaps running rm -rf on the directory would be better? The other option might be to change

$this->upgrade_root = variable_get('file_public_path', conf_path() . '/files') . '/upgrade';

into

$this->upgrade_root = variable_get('file_public_path', conf_path() . '/files') . '/upgrade_' . $this->randomName();

Second, thank you @xjm. Your review is a great start. I'll wait a bit and see if anyone is willing to roll a new version of the patch with your feedback and if not, then I'll make the changes.

#46: 1364798-46.patch queued for re-testing.

I had to reroll #46 first. I applied all except #7 from @xjm feedback for #47. Thanks.

We still have TODOs.

Let's see what the test bot thinks.

Thanks @clemens.tolboom, those changes make it much cleaner and easier to read.

A few more observations based on the updated version:

1. +++ b/core/modules/simpletest/drupal_web_test_case.phpundefined
   @@ -1276,30 +1276,23 @@ class DrupalWebTestCase extends DrupalTestCase {
   -   * Generates a random database prefix, runs the install scripts on the
   -   * prefixed database and enable the specified modules. After installation
   -   * many caches are flushed and the internal browser is setup so that the
   -   * page requests will run on the new prefix. A temporary files directory
   -   * is created with the same name as the database prefix.
   

   We can keep this detailed information (if accurate), just make it a separate paragraph after the summary (after a blank line).

2. +++ b/core/modules/simpletest/drupal_web_test_case.phpundefined
   @@ -1276,30 +1276,23 @@ class DrupalWebTestCase extends DrupalTestCase {
   +   * Generates a temporary prefixed database to ensure that tests have a clean starting point.
   

   This looks a lot better. Thee summary should be one line less than 80 chars, though. (Might need to reword a bit.)

3. +++ b/core/modules/simpletest/tests/upgrade/upgrade.testundefined
   @@ -315,6 +315,346 @@ abstract class UpgradePathTestCase extends DrupalWebTestCase {
   +abstract class MajorUpgradePathNGTestCase extends DrupalWebTestCase {
   

   I have no idea what "NG" stands for here; can we spell it out or otherwise clarify?

4. +++ b/core/modules/simpletest/tests/upgrade/upgrade.testundefined
   @@ -315,6 +315,346 @@ abstract class UpgradePathTestCase extends DrupalWebTestCase {
   +  /**
   +   * Replaces DrupalWebTestCase::setUp().
   +   */
   

   This particular docblock we can omit. (The current standard is that setUp() methods don't have docblocks, with the exception certain cases like our friend elsewhere in this patch that needs extra documentation.)

5. +++ b/core/modules/simpletest/tests/upgrade/upgrade.testundefined
   @@ -315,6 +315,346 @@ abstract class UpgradePathTestCase extends DrupalWebTestCase {
   +   * Replaces the use of variable_set() for use on child site that is not upgraded.
   

   This summary is over 80 chars, so we should reword it a little to make it slightly shorter.

6. +++ b/core/modules/simpletest/tests/upgrade/upgrade.testundefined
   @@ -315,6 +315,346 @@ abstract class UpgradePathTestCase extends DrupalWebTestCase {
   +   * Executes a (list of) command(s) and debugs the result.
   

   This confused me twice now; maybe here it would be better to say:
   Executes a command or list of commands and debugs the result.

7. +++ b/core/modules/simpletest/tests/upgrade/upgrade.testundefined
   @@ -315,6 +315,346 @@ abstract class UpgradePathTestCase extends DrupalWebTestCase {
   +   * Clones a fresh Drupal 7 git checkout into upgrade directory.
   

   I'd say "into the upgrade directory."

8. +++ b/core/modules/simpletest/tests/upgrade/upgrade.testundefined
   @@ -315,6 +315,346 @@ abstract class UpgradePathTestCase extends DrupalWebTestCase {
   +   * Perform the upgrade.
   

   I think this should be:
   Performs the upgrade.

9. +++ b/core/modules/simpletest/tests/upgrade/upgrade_bare.testundefined
   @@ -1,25 +1,42 @@
   - * Upgrade test for the bare database..
   + * Upgrades tests for the bare database.
   

   I don't think it upgrades tests... rather it tests upgrades. :D Maybe:
   Runs major version upgrade tests for the bare database.

10. +++ b/core/modules/simpletest/tests/upgrade/upgrade_bare.testundefined
    @@ -1,25 +1,42 @@
    + * Runs Minor Upgrade test for the bare database.
    

    How about:
    Runs minor version upgrade tests for the bare database.

Novice-tagging again; same deal. If you want I can also ask someone to make these changes during office hours. Thanks!

@xjm thanks again.

#1 Those lines belong to ::setup ... so are still there :)

#2 Done

#3 NG stands for Next Generation ... I'm not sure but guess it will replace the NG-less version when tests are all ran successfully.

#4, #5, #6, #7, #8, #9, #10

I removed the novice. Please leave it that way as the patch will still fail probably and it deserves guru attention ;)

Sorry, was just tagging novice so a novice could do the cleanups if you didn't get to it first. Novice doesn't mean "gurus don't look at this" (they will regardless); it just means "there's something a novice can do here." Thanks!

I fixed the err of #54 which was about $htaccess(_insecure) concat.

Test fails on local with
An AJAX HTTP error occurred. HTTP Result Code: 500 Debugging information follows. Path: /batch?render=overlay&id=8&op=do StatusText: Service unavailable (with message) ResponseText: PDOException: SQLSTATE[42S22]: Column not found: 1054 Unknown column 'base.langcode' in 'field list': SELECT base.uid AS uid, base.name AS name, base.langcode AS langcode, base.pass AS pass, base.mail AS mail, base.theme AS theme, base.signature AS signature, base.signature_format AS signature_format, base.created AS created, base.access AS access, base.login AS login, base.status AS status, base.timezone AS timezone, base.preferred_langcode AS preferred_langcode, base.picture AS picture, base.init AS init, base.data AS data FROM {users} base WHERE (base.uid IN (:db_condition_placeholder_0)) ; Array ( [:db_condition_placeholder_0] => 1 ) in DrupalDefaultEntityController->load() (line 202 of /Users/clemens/Sites/drupal/d8/www/core/modules/entity/entity.controller.inc).

(sigh) path to git ... we really should fix that for local testing :(

Not sure why the image test fails.

The debug line from the Major version bare upgrade test

prepareUpgradeEnvironment:

'Skipping \'SetHandler\' sites/default/files/upgrade/.htaccess'

implies .htaccess has already a handler set. Did a cleanup failed?

The Minor version bare upgrade test definitively failed on an existing .git/index.lock.

Are we not deleting the .git somehow?

The order from http://qa.drupal.org/pifr/test/233318 suggests MinorUpgradePathNGTestCase

exception 'PDOException' with message 'SQLSTATE[42S02]: Base table or view not found: 1146 Table 'drupaltestbotmysql.simpletest296740variable' doesn't exist' in /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Database/Statement.php:58

was run before MajorUpgradePathNGTestCase

exception 'PDOException' with message 'SQLSTATE[42S22]: Column not found: 1054 Unknown column 'base.langcode' in 'field list'' in /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Database/Statement.php:58

I need to get your patch synched up with mine. I get the D8 tests to work, the D7 *kinda* works but not exactly. I can see why its not working. And when I did a manual upgrade from latest D7 to latest D8, it was a *HUGE* pain. My way was:

1. Checkout latest D7 code
2. Create D7 site
3. Log out
4. Checkout latest D8 code
5. Log in

But I was never able to log in and I'd stay stuck in the same langcode error as above. I had to set update_free_access to true and only access update.php for the upgrade to go through. So the simpletest screaming is kinda because of this behavior (which we mimic with the user_load in the setUp() process. Actually removing the user_load set of lines still allows the tests to go through. But I did not submit that version of the patch yet. But like I said, I'll try to merge our patches together sometime today.

A resolution to this seems to be to change $user = user_load(1) into $user = $this->root_user;.

I also think that having the results get stored in one directory (upgrade) doesn't work all too well if we're trying to debug what is going on (wherein both 7.x and 8.x fail, let's say). So I'm using the database prefix (so the upgrade directory is upgrade_simpletestNNNNNN). The tests seem to pass (locally) in either case so we can use whichever works best.

Setting to 'needs review'

While I am not the update system maintainer I would really love if this wouldn't get in before Denver (that's a mere two weeks from now) when I will have the time to properly review. Thanks for your patience and understanding.

#63: 1364798.63.patch queued for re-testing.

#63: 1364798.63.patch queued for re-testing.

New version of the patch goes back to using the current git tree to perform a checkout from the origin. The speeds of the test should be a very large increase (depending on the speed of your connection - local tests sped up from taking > 2 mins per upgrade test to < 30 seconds).

I'm trying to expand on the tests by trying to write code to generate content but I am running into strange issues where it keeps failing. At any rate, I'm basically running generate-d7-content.sh to generate some content for testing purposes (yes, it is not in the right place but we need a proof of concept up and running before moving things to the right areas). I don't think it is the right approach (how will we extend the content generation for tests like for configuration, multilingual initiatives, etc to test different components?) so any guidance on a better approach would be *greatly* appreciated. Attaching the new version for review regardless.

Woohoo, I got a version working that can generate content! Need more help with this but we can now have a way to test many different things. Attaching first version of this patch for review. There are 2 new tests classes for testing with 'filled' content. We may want to figure out how to enable all modules and all that fun stuff as well.

Rename $modules to $keep_modules in uninstallModulesExcept please.

Change the // TODO: check out why agent 'simpletest' is disallowed to 'simpletest' agent is disallowed for security reasons

+ // Since cache_bootstrap won't exist in a Drupal 6 site, True but this is D7->D8 so remove the try-catch please.

Remove + protected function shell_exec this wrapper. It's not necessary.

Just + $shell_exec("$git --git-dir={$this->upgrade_root}/.git --work-tree={$this->upgrade_root} checkout origin/7.x ."); should be enough, all other commands in prepareCheckout are not necessary at all. (nice trick with the origin/7.x)

+ // We allow for all possible options. this needs a comment saying "as this is a random and temporary directory, this is not a security risk". Please explore whether using a more random directory name than the db prefix is feasible, like ->randomName(16).

setupDrupal should not be necessary as file_prepare_directory does this.

+ $response = filter_xss($this->drupalPost(NULL, array(), t('Continue')), array());
+ if (!$this->assertResponse(200)) {

i think this can be collapsed into

+ $this->drupalPost(NULL, array(), t('Continue'));
+ if (!$this->assertResponse(200) || $this->assertRaw(t('No pending updates.')) {

or assertText

+ // Check if there still are pending updates. this comment is not clear enough. Smthng like "By now there should be no pending updates".

Remove commented out code in general.

+ // Log in with a clean $user. actually a root $user.

+++ b/core/modules/simpletest/tests/upgrade/upgrade.test
@@ -315,6 +315,361 @@ abstract class UpgradePathTestCase extends DrupalWebTestCase {
+  /**
+   * Clones a fresh Drupal 7 git checkout into the upgrade directory.
+   *
+   * For sanity checks we run a few environent tests to help local
+   * developers getting grip to their development environment.
+   *
+   * For example:
+   * - the location for git is system dependent.
+   * - the cleanup of the checkout directory failed a previous time.
+   * - mixing drush and apache is mixing access control attributes.
+   */
+  protected function prepareCheckout() {
+    $git = 'git';
+    // TODO: we need to fix this git location.
+    // This is a hardcode path to a git install.
+    // Use it when testing locally.
+    //$git = "/usr/local/git/bin/git";
+
+    //TODO: change commands to an OS agnostic version.
+    $this->shell_exec('pwd');
+    $this->shell_exec('env');
+    $this->shell_exec("ls -al {$this->upgrade_root}/");
+    $this->shell_exec("rm -rf {$this->upgrade_root}/");
+    $this->shell_exec("mkdir -p {$this->upgrade_root}");
+    $this->shell_exec("cp -R .git {$this->upgrade_root}/.git");
+    $this->shell_exec("ls -al {$this->upgrade_root}/");
+    $this->shell_exec("chmod -R 0775 {$this->upgrade_root}");
+    $this->shell_exec("ls -al {$this->upgrade_root}/");
+    $this->shell_exec("$git --git-dir={$this->upgrade_root}/.git --work-tree={$this->upgrade_root} checkout origin/7.x .");
+    // $this->shell_exec("$git clone --branch 7.x http://git.drupal.org/project/drupal.git {$this->upgrade_root}");
+  }

I disagree with "we only need the git checkout as we had quite some trouble finding out why the prepareCheckout failed. Especially locally. See the methods documentation.

I do agree we could reduce the shell_exec commands a little. And we should document the remaining commands. Ie the cp -R .git is very useful for a local developer testing the upgrade as it runs way faster.

By changing the upgrade_root to a random location we loose control over the proper functioning of prepareCheckout too.

I have no idea what #72 tries to say but #71 clearly says that function is going away:

  protected function prepareCheckout() {
    shell_exec("git --git-dir={$this->upgrade_root}/.git --work-tree={$this->upgrade_root} checkout origin/7.x .");
  }

the rest is debug leftovers.

@chx I don't get it ... we had trouble in running the test so in the future people will run into trouble running their tests whether on local or d.o.

The protected method shell_exec logs nicely what was executed with what result.

Reducing the debug info into an oneliner is imho bad practice :(

+++ b/core/modules/simpletest/tests/upgrade/upgrade.test
@@ -315,6 +315,361 @@ abstract class UpgradePathTestCase extends DrupalWebTestCase {
+  protected function shell_exec($commands) {
+    if (!is_array($commands)) {
+      $commands = array($commands);
+    }
+    foreach ($commands as $command) {
+      // @see http://php.net/manual/en/language.operators.execution.php
+      $result = shell_exec("$command 2>&1");
+      debug($result, $command);
+    }
+  }

This is doing nice debug logging does it?

#70: 1364798.70.patch queued for re-testing.

EDIT: I'm going to try and work on the patch this weekend but wanted to see if the patch still applies.

Ok, I've looked through the file and based off the discussion between @chx and @clemens.tolboom, I feel that for now, keeping the shell_exec command is a good idea if atleast for the initial debugging. I've tried to correct the various portions that @chx pointed out and added in a couple of things:

• Use checkRequirements to see that we have the git command and the git directory to do the things we want.
• Path to git is now configurable using variable_get or by setting in settings.php
• Removed a bunch of the things we had listed during the directory setup process.

@chx, I did not remove 2 of the chmod commands as one is dealing with ensuring the .git directory that comes in gets the permissions set up correctly (we could likely remove that one) but the one in tearDown has to stay - after site install, settings.php gets locked down from being modified/deleted so we need to set the permissions to that file (and any others that were locked down to readonly) can get deleted.

Attaching patch and interdiff.

Hmm, I'm not quite sure why the tests failed on the check if git exists (afaik, it does on the qa servers or the prior tests would not have passed). I tried testing on a second computer and I am getting a different set of exceptions (Undefined index: module). Not sure why they are happening yet and going to try and debug the issue.

This patch is to get rid of the notices I received (and to correct a few things like having generateContent() command run on minor upgrades) - the tests seem to go through correctly at this stage (locally, anyways - I'm not sure if the error above will continue as before..)

I'm hoping this version of the test passes since I'm explicitly removing the checkRequirements portion (which will help in knowing if the testbots are running git).

Needs review.

ack...wrong patch!

Do you really need to copy the whole git dir? http://privatepaste.com/9263cf3ba5 I tried it without -- but note the dot after the 7.x -- I could only get this to work with changing to the directory in question. But then it worked.

I tried to run the copy without the git directory and got it to work (my command was $this->shell_exec("{$this->git_command} --git-dir=./.git --work-tree={$this->upgrade_root} checkout origin/8.x ."); (or 7.x if I was D7) but what happens is if I run git status on the current drupal root, it and what is in the git tree do not match with each other. So if that is not an issue, I am willing to give it a try.

I'd still like to address being able to check that the server has git before it tries to do anything so if anyone has some cool ideas on it, they are most welcome :)

What's faster, cd dir/to/test; git do_magic_for_me; cd ../../..;git checkout 8.x; now run the test or copying 100MB around?

@chx, I won't argue that copying the .git directory isn't faster. But working with the initial directory causes issues like this to come up (I want to checkout 7.x into a directory called blah, let's say):

amodi@localhost ~/Sites/d8: mkdir blah
amodi@localhost ~/Sites/d8: cd blah/
amodi@localhost ~/Sites/d8/blah: git --git-dir=../.git checkout origin/7.x
D	profiles/minimal/minimal.profile
D	profiles/minimal/translations/README.txt
D	profiles/standard/standard.profile
D	profiles/standard/translations/README.txt
D	profiles/testing/modules/drupal_system_listing_compatible_test/drupal_system_listing_compatible_test.module
D	profiles/testing/modules/drupal_system_listing_compatible_test/drupal_system_listing_compatible_test.test
D	profiles/testing/modules/drupal_system_listing_incompatible_test/drupal_system_listing_incompatible_test.info
D	profiles/testing/modules/drupal_system_listing_incompatible_test/drupal_system_listing_incompatible_test.module
D	sites/all/README.txt
D	sites/all/modules/README.txt
D	sites/all/themes/README.txt
D	sites/example.sites.php
Note: checking out 'origin/7.x'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by performing another checkout.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -b with the checkout command again. Example:

  git checkout -b new_branch_name

HEAD is now at dfa820d... Issue #1537370 by Liam Morland: Improve documentation of return value for file_load()
amodi@localhost ~/Sites/d8/blah: cd ..
amodi@localhost ~/Sites/d8: git checkout 8.x
error: Your local changes to the following files would be overwritten by checkout:
	.htaccess
	README.txt
	index.php
	profiles/minimal/minimal.info
	profiles/minimal/minimal.install
	profiles/standard/standard.info
	profiles/standard/standard.install
	profiles/testing/modules/drupal_system_listing_compatible_test/drupal_system_listing_compatible_test.info
	profiles/testing/testing.info
	profiles/testing/testing.install
	profiles/testing/testing.profile
	robots.txt
	sites/default/default.settings.php
	web.config
Please, commit your changes or stash them before you can switch branches.
error: The following untracked working tree files would be overwritten by checkout:
	core/CHANGELOG.txt
	core/COPYRIGHT.txt
	core/INSTALL.mysql.txt
	core/INSTALL.pgsql.txt
	core/INSTALL.sqlite.txt
	core/INSTALL.txt
	core/LICENSE.txt
	core/MAINTAINERS.txt
...
Aborting

Which is finally resolved with a

amodi@localhost ~/Sites/d8: rm -rf core/ && git checkout .

Doing a local git clone turned out to be surprisingly faster although that might not make sense if folks are committing directly to their local 7.x/8.x branch (or whatever else in the future).

yes, I explored the --git-dir option in the first patches already. The git checkout updates the .git information to state that 7.x has been checked out, although it has been checked out into a completely different location. Thus, any subsequent git operation on the original/actual D8 checkout fails and actually breaks horribly - you're not able to recover from the HEAD pointer mismatch without massaging the filesystem manually.

Truth is that we just need an "export" of the 7.x branch and the .git dir is not actually needed. However, there does not seem to be a git command that would allow us to do just that.

Ok, how about using git archive? You can read about it at http://linux.die.net/man/1/git-archive

I was trying to figure out git checkout-index but I didn't get the impression it would do what we wanted (check out a specific branch without the .git directory)

New patch removes the cp -R commands but the whole thing is becoming even less OS agnostic :(

This is awesome. Nice detective work, man. And pipe exists on windows and cygwin has gnu tar. I will try to find time to properly review, I hope others will review, too.

I've extracted the DrupalWebTestCase::setUp() changes from this patch into #1541958: Split setUp() into specific sub-methods

While I like the progress here so far, I'm a little confused by all the variations of the UpgradeNG base classes and implementations - especially the minor update tests - and, even more so, because those variations seem to entirely duplicate the UpgradeNG root class (while still extending it) instead of only doing the necessary overrides. In short, I had a very hard time to review this patch and make sense of it, and would like to scale this issue/patch back down to the major upgrade path. We can do the other variations in separate follow-up issues.

Re-roll without the DrupalWebTestCase::setUp() changes.

+++ b/core/modules/system/tests/upgrade/upgrade.testundefined
@@ -335,3 +335,474 @@ abstract class UpgradePathTestCase extends DrupalWebTestCase {
+  /**
+   * Replaces DrupalWebTestCase::setUp().

The standard is "Overrides ..." I think

+++ b/core/modules/system/tests/upgrade/upgrade.testundefined
@@ -335,3 +335,474 @@ abstract class UpgradePathTestCase extends DrupalWebTestCase {
+      debug($result, $command);

I assume that debug is just there for debugging purpose and can be removed now that we have something that works?

+++ b/core/modules/system/tests/upgrade/upgrade.testundefined
@@ -335,3 +335,474 @@ abstract class UpgradePathTestCase extends DrupalWebTestCase {
+
+  protected function setupDrupal() {
+  }
+
+  protected function generateContent() {

This should contain some docblocks to explain that test classes are supposed to implement them.

Also, according to http://api.drupal.org/api/drupal/core%21modules%21entity%21entity.contro..., this should have the {} on the same line.

+++ b/core/modules/system/tests/upgrade/upgrade.testundefined
@@ -335,3 +335,474 @@ abstract class UpgradePathTestCase extends DrupalWebTestCase {
+    // Check for any pending updates.
+    if (strpos($response, t('No pending updates.')) === FALSE) {
+      // Go!
+      $this->drupalPost(NULL, array(), t('Apply pending updates'));
+      if (!$this->assertText('Updates were attempted')) {
+        return FALSE;

Haven't read everything, is there a reason for not using assertNoText()?

+++ b/core/modules/system/tests/upgrade/upgrade.testundefined
@@ -335,3 +335,474 @@ abstract class UpgradePathTestCase extends DrupalWebTestCase {
+  /**
+   * Force uninstall all modules from a test database, except those listed.
+   *
+   * @param $modules
+   *   The list of modules to keep installed. Required core modules will
+   *   always be kept.
+   */
+  protected function uninstallModulesExcept(array $keep_modules) {
+    $required_modules = array('block', 'dblog', 'filter', 'node', 'system', 'update', 'user');
+
+    $modules = array_merge($required_modules, $keep_modules);
+
+    db_delete('system')
+      ->condition('type', 'module')
+      ->condition('name', $modules, 'NOT IN')
+      ->execute();

This function seems to be unused?

+++ b/core/modules/system/tests/upgrade/upgrade.testundefined
@@ -335,3 +335,474 @@ abstract class UpgradePathTestCase extends DrupalWebTestCase {
+ * Performs end-to-end tests of the upgrade path.
+ */
+abstract class MinorUpgradePathNGTestCase extends MajorUpgradePathNGTestCase {
+
+  protected function setUp() {
+    global $user;

Yep, I confirmed that this is an exact duplicate of the major setUp() method except some old comments? Should be possible to just delete this function.

+++ b/core/modules/system/tests/upgrade/upgrade.testundefined
@@ -335,3 +335,474 @@ abstract class UpgradePathTestCase extends DrupalWebTestCase {
+  protected function prepareCheckout() {
+    $this->git_command = variable_get('simpletest_git_path', 'git');
+    //TODO: change commands to an OS agnostic version.
+    $this->shell_exec("chmod -R 0775 {$this->upgrade_root}");
+    $this->shell_exec("{$this->git_command} archive origin/8.x | tar -x -C {$this->upgrade_root}");

I guess we could move the branch name to a property then this function could be the same for major and minor as well?

+++ b/core/modules/system/tests/upgrade/upgrade.testundefined
@@ -335,3 +335,474 @@ abstract class UpgradePathTestCase extends DrupalWebTestCase {
+  /**
+   * Performs the upgrade.
+   *
+   * @param $register_errors
+   *   Register the errors during the upgrade process as failures.
+   * @return
+   *   TRUE if the upgrade succeeded, FALSE otherwise.
+   */
+  protected function performUpgrade($register_errors = TRUE) {
+    return parent::performUpgrade($register_errors);

Uh, looks unecessary as well?

In general, this looks amazing. One thing to figure out I guess would be how to support contrib module upgrade tests? Yes, there are actually contrib modules with upgrade tests, e.g. Simplenews.

We'd probably need to provide an easy way to do that git archive thingy for a given path/module as well that would put it in sites/all/modules of the previous site?*

* Yes, yes, follow-up for sure!

Updated issue summary.

Updated issue summary.

Updated issue summary.

I've removed all the "minor upgrade" stuff and performed a massive clean-up of code and comments.

However, also added plenty of @todos - we're not remotely done with this ;)

I've also rewritten the issue summary.

Feel free to call me nuts, but I actually have a crazy idea for making even more simple:

1. Add upgrade_prepare.module with this .info file:
   

   name = Upgrade Preparation
   description = Prepares the site for upgrade path testing.
   package = Core
   core = 7.x
   hidden = TRUE
   

2. cp -R drupal_get_path('module', 'upgrade_prepare') $this->upgrade_root . /modules/
3. cp __FILE__ $this->upgrade_root . /modules/upgrade_prepare/
4. $this->drupalGet('upgrade-prepare/' . __CLASS__);

5. function upgrade_prepare_menu() {
     $items['upgrade-prepare'] = array(
       'page callback' => 'upgrade_prepare_page',
     );
     return $items;
   }
   
   function upgrade_prepare_page($class) {
     foreach (file_scan_directory(dirname(__FILE__), '/\.(test|php|inc)$/') as $file) {
       include_once $file->uri;
     }
     // Insert configuration and content using native APIs. :)
     $class::prepareUpdate();
   }
   

As mentioned in IRC, the simpletest check in install.core.inc originally comes from http://drupal.org/node/258200 and prevents getting around the "database already installed check" and install a new drupal site. So that makes sense and working around it like this is also ok, because it really is a new installation in that case.

Updated issue summary.

Is this issue still valid now that migrate is in core?

Thankfully not — all upgrade path tests have been removed from D8 core. :)