Drupal 8 doesn't have any schema changes, but there is a placeholder system_update_8000() that was added when the 6.x-7.x updates were removed, that should be enough to add boilerplate tests.

What we need:

1. Update scripts/dump-database-d6.sh and scripts/generate-d6-content.sh to Drupal 7.

2. Create minimal, standard and standard+all optional modules database dumps using 7.0.

3. Add very basic tests that load the databases, run the tests, and maybe do something stupid like visit the front page and makes sure it's still there.

I just ran a clean 7.2 to 8.x upgrade, and amazingly that currently works, so those tests should pass.

Since #760014: Stop supporting direct updates from Drupal < 6.17 we only support upgrades from the latest stable release, so ideally we'd keep the 7.x database somewhat up-to-date (the absolute best case would be whenever a 7.x point release comes out, but some of these may not contain any updates).

This needs to be in place before any real updates get committed to 8.x.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

Crossposting with #1182296: Add tests for 7.0->7.x upgrade path (random test failures) - the database dump and content creation scripts are exactly the same as what's needed there, so code for those should be the same.

Since we'll be backporting those scripts, marked #1182290: Add boilerplate upgrade path tests postponed on this issue.

marcingy’s picture

Assigned: Unassigned » marcingy

need this for http://drupal.org/node/1161486 so will happily start the process

catch’s picture

Priority: Major » Critical

Bumping this to critical.

#1097100: Remove all 7xxx update functions and tests (D6 to D7 upgrade path) removed the D6 database + tests alongside the update functions. Had those been left in, then it would've been possible to test new update functions in Drupal 8 using those tests (not perfect but doable).

As it is, the tests were removed without any viable replacement, so the first patch to D8 that introduces an update hook is going to either be untested, or be postponed on this issue. The upgrade path from 6.x-7.x was properly broken for 2 years (and counting), so let's not do that again.

marcingy’s picture

Assigned: marcingy » Unassigned

Unassigning as I'm just not finding time to work on this

catch’s picture

Status: Active » Needs review
FileSize
450 bytes

I'm uploading a patch that shows the current problem. With any test coverage at all there would be a fatal error.

catch’s picture

Following discussion with webchick in irc I marked the following patches postponed on this to make the current situation clear. Those patches all have relatively straightforward update hooks, but the code will have zero test coverage whatsoever (could be calling non-existent functions etc. for all we know) without the basic set of tests that should be added here.

#1188430: Rip out textgroup support from locale module
#918808: Standardize block cache as a drupal_render() #cache
#646312: Remove $vocabulary->module

franz’s picture

subscribing

catch’s picture

Status: Needs review » Active

Back to active

mdm’s picture

I'm going to take a crack at this if nobody objects; I've already made some progress on requirement #1 (the two script updates).

Question about requirement #2: what you're visualizing is the user should be able to pass some command line option to the db dump script to specify core modules/optional core modules/all modules?

Q about requirement #3: What sort of testing are we talking about? Should system_update_8000 run some kind of unit tests, or should it just be a small set of ad hoc tests defined in system.install, such as the ones already there for testing php versions, options, &c?

catch’s picture

#2 - so far we have just been generating data manually (i.e. enabling modules/running devel etc.) then dumping it. The D6-D7 upgrade tests have a couple of 'base' databases that individual tests then add to.

To get something going, would be fine to start with a 'standard' Drupal 7 install and just dump that straight away - we can improve on that once there's something to work with.

On #3 - if you look at the Drupal 7 tests in modules/simpletests/tests/upgrade - you'll see tests that load a database dump then run upgrade, then I think most of them hit a couple of pages to ensure the site is still functioning. That's all we need to get started.

Thanks for working on this!

mdm’s picture

FileSize
9.32 KB

This patch is pretty hackish, but it gets both scripts working for me on D7. More or less looking to confirm I'm on the right track here...

plach’s picture

@catch:

I ain't sure it's feasible, but what about finding the way to launch the complete test suite performing an upgrade instead of a plain install for each test? I guess this would be much more relevant that a bunch of dedicated tests.

Lars Toomre’s picture

Issue #1015916: Image field "title" allows more data than database column size. has an update hook as well that is dependent on this issue being resolved first.

jcisio’s picture

subscribe

chx’s picture

Assigned: Unassigned » chx

jcisio and I are on this.

chx’s picture

Status: Active » Needs review
FileSize
16.73 KB

Please review the generate and the dump scripts. I copied the object fix from ctools into the drupal_var_export function.

Status: Needs review » Needs work

The last submitted patch, 1182290.patch, failed testing.

BTMash’s picture

Status: Needs work » Needs review
FileSize
13.88 KB

This is a (very) minor cleanup of the work done by chx (removed the .save file that had been added in). I'm still trying to understand it fully. The contents of utility.inc look sound, though the dump-database-d7.sh seemed to not have the update users set uid = uid - 1 portion in there which I am guessing in unnecessary?. I will write back more thoughts regarding the remainder though once I get a chance. Partly a subscribe so I remember to get back to this as well :)

BTMash’s picture

#16: 1182290.patch queued for re-testing.

chx’s picture

Yeah, the uid - 1 stuff is completely unnecessary now, users.uid is not a serial now.

BTMash’s picture

Ok, I see that these tests pass what is currently in. But when I try the banana patch from @catch along with the patch from @chx, the tests still pass (when it should have failed?). Not sure if I'm doing something wrong or if there is some additional code that is necessary.

EDIT: Talked with catch on irc. Would need to write out tests to test out the upgrade path (making some more sense now)

jcisio’s picture

Yes I think we need some tests for data consistency after upgrade.

podarok’s picture

subscribe

BTMash’s picture

This patch needs a thorough review (and revisions to upgrade.test). I am attaching the dumps from a clean 'standard' profile install of Drupal (bare for a clean, filled for one with generated content). Sorry in advance for the large diff :)

Status: Needs review » Needs work

The last submitted patch, boilerplate_upgrade_tests_1182290_24.patch, failed testing.

tstoeckler’s picture

EDIT: This was a crosspost with the patch above, I have no idea, how much of the below is still valid.

+++ b/scripts/dump-database-d7.sh
@@ -0,0 +1,91 @@
+ * The output of this script is a PHP script that can be ran inside Drupal 7

ran -> run
Drupal 7 -> Drupal 8 (I think?)

+++ b/scripts/dump-database-d7.sh
@@ -0,0 +1,91 @@
+ * and recreates the Drupal 7 database as dumped. Transient data from cache
+ * session and watchdog tables are not recorded.

"...cache, session, and watchdog..." (missing commas)

+++ b/scripts/dump-database-d7.sh
@@ -0,0 +1,91 @@
+$_SERVER['PHP_SELF']        = '/index.php';
...
+$_SERVER['PHP_SELF']        = $_SERVER['REQUEST_URI'] = '/';

This seems duplicate?

+++ b/scripts/dump-database-d7.sh
@@ -0,0 +1,91 @@
+ * Filled installation of Drupal 7.0, for test purposes.

Should we determine the minor Drupal version or is 7.0 ok? What about possible future Schema updates in 7.x?

+++ b/scripts/dump-database-d7.sh
@@ -0,0 +1,91 @@
+ * This file was generated by the dump-database-d6.sh tool, from an

d6 -> d7

+++ b/scripts/dump-database-d7.sh
@@ -0,0 +1,91 @@
+ * tool. It has the following modules installed:
+
+ENDOFHEADER;

Not too firm with Heredoc, but doesn't this insert a blank line in the middle of the PHPDoc?

+++ b/scripts/dump-database-d7.sh
@@ -0,0 +1,91 @@
+  $output .= " *  - $module\n";

Super minor: I think our standard is to indent lists in PHPDoc so that the "- " lines up with the text above. That would mean there's one space to many here.

+++ b/scripts/generate-d7-content.sh
@@ -0,0 +1,309 @@
+ * Generate content for a Drupal 6 database to test the upgrade process.

Drupal 6 -> Drupal 7
(also below)

+++ b/scripts/generate-d7-content.sh
@@ -0,0 +1,309 @@
+$_SERVER['PHP_SELF']        = '/index.php';
...
+$_SERVER['PHP_SELF']        = $_SERVER['REQUEST_URI'] = '/';

See above.

+++ b/scripts/generate-d7-content.sh
@@ -0,0 +1,309 @@
+$modules_to_enable          = array('path', 'poll');
...
+// Create vocabularies and terms

Shouldn't that include taxonomy.module then?

+++ b/scripts/generate-d7-content.sh
@@ -0,0 +1,309 @@
+var_dump($node);

Leftover debug.

I have yet to try the patch, which I will hopefully do soon.

I'm setting this to needs work, because of the above, but assuming it works, it's probably wise to commit it nonetheless, as this is presumably the patch that could make the Drupal 8.0 release suck a lot less than the Drupal 7.0 release in terms of the upgrade path situation (I don't (!!!!!) mean that in a general sense!) and the above is really only minor clean-up.

BTMash’s picture

I have updated the patch by correcting some of the pieces as recommended by @tstoeckler. I have also left the upgrade.test file alone (that will require changes as mentioned above - I also need to figure out where to reference the bare / filled dumps). I **think** the patch should now apply correctly at the very least (I get 2 warnings on EOF lines but things seem to come through otherwise.

BTMash’s picture

Status: Needs work » Needs review

Should have marked as 'needs review'

chx’s picture

Issue tags: -D8 upgrade path

This one has the tests. Not much of a test. We probably want to nuke them later. But this issue is about boilerplate.

chx’s picture

This one has the tests. Not much of a test. We probably want to nuke them later. But this issue is about boilerplate.

chx’s picture

FileSize
2 MB

matason uploaded this for me 'cos with my slow connection and the huge patch I was timing out.

BTMash’s picture

Sorry about the lack of tests on my side. The tests make sense. Its missing the utility.inc and generate scripts so attached patch should have all the pieces.

matason’s picture

FileSize
2 MB

Uploaded new patch, combination of patches from #5 and #31 as requested by @chx

chx’s picture

To clarify: #32 is probably RTBC, #33 is to show tests are working.

aspilicious’s picture

Status: Needs work » Needs review

I'm confused...

Why is #33 not failing?

I'm uploading a patch that shows the current problem. With any test coverage at all there would be a fatal error.

The bananas() function doesn't get loaded or the test would fail... Or not?

BTMash’s picture

Status: Needs review » Needs work

I looked through the UI and apparently, the tests are not running. Well, the tests aren't showing up in the UI so there isn't a way for me to select them for testing. I'm unsure of what we might be missing. Trying to narrow it down now.

chx’s picture

So, here's a failing and and a passing patch. The failing patch shows that things fail (doh!) which didnt happen in #33 because the .info wasnt edited.

BTMash’s picture

Status: Needs review » Needs work

The tests now show up (yay!) but the passing test fails from the get-go from upgrade.test on line 114 with: Call to undefined function update_get_d6_session_name(). I don't think that whole bit is necessary since D7/D8 sessions are currently handled in the same way(?). That session portion needs to get reworked so it'll work correctly.

BTMash’s picture

Status: Needs work » Needs review
FileSize
2.01 MB
2.01 MB

I think I have it resolved now (this was done by creating a user that has the ability to adminster updates/upgrades and removing the code that pertained to requiring user 1). Also adding 2 versions of the patch (one that fails and one that doesn't).

chx’s picture

OK I see i will write a better fix -- adding a user into a to-be-upgraded D7 site with a D8 user routine is not going to cut it.

chx’s picture

FileSize
177.9 KB
178.41 KB

This is new. I have added gzip support and also it actually works :) Removed the ssid column and the D6 - session name function call where the function itself was already nuked somewhere. I hope the bot uses git apply 'cos for sure these wont apply with patch -p1.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Let's never go without upgrade path testing ever, ever again. Great work btmash and chx!

tstoeckler’s picture

FileSize
177.9 KB
178.4 KB
+++ b/modules/simpletest/simpletest.install
@@ -38,6 +39,14 @@ function simpletest_requirements($phase) {
+    'value' => $has_zlib ? $t('Enabled') : $t('Not found'),    ¶

Trailing whitespace.

New patches attached.

I literally edited the patch files, so I'm leaving at RTBC.

matason’s picture

FileSize
178.4 KB
177.9 KB

I noticed a couple of small issues with the patches in #43 whilst working on another patch...

Changed:

$requirements['hash']['severity'] = REQUIREMENT_ERROR;

to

$requirements['zlib']['severity'] = REQUIREMENT_ERROR;

and

$requirements['zlib']['description'] = $t('The testing framework could not be installed because the PHP zlib extension is disabled.', array('@hzlib_url' => 'http://php.net/manual/en/book.zlib.php'));

to

$requirements['zlib']['description'] = $t('The testing framework could not be installed because the PHP zlib extension is disabled.', array('@zlib_url' => 'http://php.net/manual/en/book.zlib.php'));

I edited the patch files directly just as @tstoeckler did so left RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1182290_44_failing.patch, failed testing.

BTMash’s picture

Status: Needs work » Reviewed & tested by the community

Marking back to rtbc since the expected patches pass/fail :)

@matason, I guess the question would be should the entire simpletest module require the zlib extension? I've updated your issue #1271060: Allow tests to declare 'requirements', skip tests where requirements are not met so that particular tests can have requirements.

BTMash’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
191.4 KB

This now has a check if zlib extension is enabled (and provides failed tests if it does not) as a result of #1271060: Allow tests to declare 'requirements', skip tests where requirements are not met. Do we need to continue with attaching a failed version of the patch (for banana()) since that aspect remains unchanged.

BTMash’s picture

FileSize
190.03 KB

I somehow had the .install file changes in there which shouldn't have been the case. Updated patch without .install changes.

BTMash’s picture

FileSize
190.02 KB

Patch with formatting suggestions by chx.

chx’s picture

Status: Needs review » Reviewed & tested by the community

While calling parent::setUp just to get the database prefixes set up is certainly an overkill, it doesnt matter much because these will only fire when you dont have zlib.

webchick’s picture

Assigned: chx » Dries

Yoinking the assigned field from chx to put this on Dries's plate.

Dries, this patch is critical and required for us to commit any D8 patches that deal with update hooks; without it, we're flying completely blind with no safety net.

catch’s picture

Issue tags: +Needs backport to D7

Quite a few bits of this are necessary for 7-7 upgrade path testing as well, so adding tag. We may want to bundle that all in with #1182296: Add tests for 7.0->7.x upgrade path (random test failures) but not sure.

catch’s picture

Dries’s picture

Version: 8.x-dev » 7.x-dev
Assigned: Dries » Unassigned
Priority: Critical » Normal
Status: Reviewed & tested by the community » Needs work

Committed to 8.x.

Do we want to backport this in any way to 7.x? Moving to 7.x for discussion. Also un-assigning this as it is no longer blocked on me.

chx’s picture

Status: Needs work » Closed (fixed)
chx’s picture

Also: YAY YAY YAY YAY YAY

plach’s picture

Version: 7.x-dev » 8.x-dev

Moving back to D8.