NOTE TO REVIEWERS AND COMMITTERS: When applying this patch, use git apply, not patch -p1.

Drupal 7 supports two main upgrade paths:

1. From 6.22 to 7.x (probably the test database could be updated to 6.22 from whatever it currently is).

2. From 7.0 (actually the first release candidate) to 7.x

Currently only the first of these is tested. Since it is quite possible for the database state to be inconsistent between a site upgraded from 6.0 to 7.0, and a new 7.0 install, we should test both of these going forwards.

What needs to happen:

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 from 7.0.

3. Add very basic tests that load the 7.0 databases then run updates. I don't think it's necessary to add any specific assertions about particular updates here - but we may want to sort it out so that it's possible to share assertions between the 6.x-7.x and 7.0-7.x tests.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

The first item on the @todo is exactly the same as the first item from #1182290: Add boilerplate upgrade path tests, whichever happens first can use the same code from the other.

catch’s picture

Status: Active » Postponed

Doh, which means that issue has to happen first, but it's impossible to run 7.0-7.x tests in 8.x so marking this postponed rather than duplicate.

Damien Tournoud’s picture

catch’s picture

Priority: Major » Critical

#1170362: Install profile is disabled for lots of different reasons and core doesn't allow for that could really do with this, bumping to critical since this is now blocking other bugs being fixed.

The infrastructure still needs to be added in #1182290: Add boilerplate upgrade path tests and backported though, so not un-postponing.

jcisio’s picture

subscribe

catch’s picture

Status: Postponed » Active

Optimistically un-postponing this now that #1182290: Add boilerplate upgrade path tests is RTBC.

matason’s picture

Status: Active » Needs review
FileSize
182.74 KB

Initial stab at a patch, not sure if I've created it correctly though (includes binary files).

BTMash’s picture

FileSize
6.09 KB

I haven't added the diff of the content from matason (he said he will work on the binary files) so I am adding a boilerplate of the tests that should be occurring. This will fail since the gen. dumps are not in but the code could use a readthrough.

matason’s picture

Status: Needs review » Needs work

I've just realised I somehow missed changes to /modules/simpletest/upgrade/upgrade.test in the patch on #7 - will have a look through and submit another patch shortly.

matason’s picture

Status: Needs work » Needs review
FileSize
195.87 KB

I've managed to get all the files into one patch, I am setting it 'needs review' to check whether it passes tests but I want to tackle the duplication of code in prepareSession() method I've introduced so will set it to 'needs work' after testing... overall I don't feel comfortable with this patch - it feels like I am trying to squeeze the update tests into the upgrade test and trying to extend a class that does mostly what is needed (but for the session stuff) - will give it some more thought, comments appreciated!

BTMash’s picture

I haven't had a proper chance to go through the code (this is partially a subscribe so I remember to look through the patch more thoroughly) but one thing I have noticed is the other profile dumps (minimal, minimal filled, standard+all, standard+all filled) and their tests have not yet been created (which is also understandable since the patch is just going to balloon up). Most of the patch looks sound though if we're requiring gzip for the tests, then the setup for the tests will also need to get updated to catch the requirement and stop the tests from running.

BTMash’s picture

Status: Needs review » Needs work

Thinking through and seeing the code some more, what I **think** should happen is that we have another area for update testing (or another folder - update) which could have its own update.test (which could be a subclass of upgrade tests) but could be built out more effectively that way (and it would be easier to follow which tests are upgrade tests and which are update tests. Once the way the tests function is worked out, then we'll have to add in the gzipped versions of the dumps (oy...). For the session, I think we can use either the uid = 1 or user with access to update/upgrade site to run the updates. So I've provided what the bare tests would look like, you have the dump and shell scripts. So we just need to write out the update.test file which will do a chunk of this (I believe the upgrade tests from D8 will help us out on this one since the way they currently function is quite similar). Then, running the tests should be ok :)

penyaskito’s picture

Subscribing.

BTMash’s picture

Talking with chx on irc, we concluded that I was far overthinking the whole thing and what @matason hasshould be expanded to instead have tests for a minimal install and a standard + all install (so both cases are covered).

BTMash’s picture

Status: Needs work » Needs review
Issue tags: -D7 upgrade path

#10: 1182296_10.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +D7 upgrade path

The last submitted patch, 1182296_10.patch, failed testing.

BTMash’s picture

Status: Needs work » Needs review
FileSize
353.4 KB

I finally got around to making out the dumps. The generate-d7 script has been updated to allow for creating content on a minimal profile. Other than that, the funstionality is basically being rerolled. Hopefully its all been done correctly...

BTMash’s picture

FileSize
353.39 KB

Rerolled to remove whitespace issues.

Status: Needs review » Needs work

The last submitted patch, 1182296_18.patch, failed testing.

BTMash’s picture

Status: Needs work » Needs review
FileSize
353.38 KB

Wow...that failed horribly (all due to where I was expecting the site to go). I should have named my site properly. Hopefully this works out a bit better.

Status: Needs review » Needs work

The last submitted patch, 1182296_20.patch, failed testing.

BTMash’s picture

I was trying to figure out why the tests were failing and running through a manual update from 7.0 -> 7.8, it seems to be broken in that I cannot update my (core) image fields from the standard install (so something is broken in core and the simpletest catches it, yay?!). I am unsure of how to fix that bug however so any help on that portion would be greatly appreciated.

BTMash’s picture

FileSize
354.45 KB

After some digging around and with help from @sun, @yched, and @mbrandon, we figured out that the issue is coming from the commit at #1129642: Populate HTML image tags with dimension attributes (like D6 imagefield) without re-introducing I/O. I have added a patch in there and am rolling a version of this patch with that patch. Once that one is committed, this one will need to be rerolled but this should *hopefully* show that the test fails without that patch and passes when it is in there.

BTMash’s picture

Status: Needs work » Needs review

ack...setting to needs review.

Status: Needs review » Needs work

The last submitted patch, 1182296_23.patch, failed testing.

BTMash’s picture

FileSize
354.81 KB

Ok, this was due to an event where there are no images. Revised patch.

BTMash’s picture

Status: Needs work » Needs review

marked as 'needs testing'

BTMash’s picture

FileSize
354.8 KB

argh...sandbox['total'] is not an array.

BTMash’s picture

#20: 1182296_20.patch queued for re-testing. Now that #1129642: Populate HTML image tags with dimension attributes (like D6 imagefield) without re-introducing I/O has been committed, the original version of the test should pass.

webchick’s picture

I just committed #1129642: Populate HTML image tags with dimension attributes (like D6 imagefield) without re-introducing I/O. Not sure if that helps or hurts this issue. :) Thank you SO much for working on it though!!

BTMash’s picture

It should help this issue in that the test patch no longer needs to get rolled with the other issue's patch for verifying that the test works :) So I'm retesting the patch in post #20 to see if there are any issues.

catch’s picture

+++ b/modules/simpletest/tests/upgrade/upgrade.testundefined
@@ -28,9 +28,56 @@ abstract class UpgradePathTestCase extends DrupalWebTestCase {
+  /**
+   * Prepare the appropriate session for the release of Drupal being upgraded.
+   */
+  protected function prepareSession() {

The method header looks generic, but then we have D7-specific code in there. Could we make it one way or the other?

+++ b/modules/simpletest/tests/upgrade/upgrade.testundefined
@@ -28,9 +28,56 @@ abstract class UpgradePathTestCase extends DrupalWebTestCase {
+    // We are going to set a missing zlib requirement property for usage during

Exceeds 80 chars (and elsewhere).

+++ b/modules/simpletest/tests/upgrade/upgrade.testundefined
@@ -92,7 +139,11 @@ abstract class UpgradePathTestCase extends DrupalWebTestCase {
     // Load the database from the portable PHP dump.

Should 'can be' be 'may be'. This could also be on the same line as the previous comment I think.

Actual code looks fine to me, and I'd love to see these get in sooner rather than later so we can actually use them.

-23 days to next Drupal core point release.

BTMash’s picture

FileSize
353.56 KB

Thanks for the feedback. I've attempted to make the changes you asked for above. For the session, I am now calling it prepareD7Session() and I created a new abstract class UpdatePathTestCase which provides that function (and the update path tests are subclasses of that) since the prepareD7Session differs between the Upgrade test cases and the Update test cases.

BTMash’s picture

Status: Needs review » Needs work

Marking it back to 'needs work' since it needs to get rerolled for core. I should be able to tackle this on Monday but would be great if someone else is able to reroll it before then :)

catch’s picture

Status: Needs work » Needs review

It's a D7 patch so it ought to be fine I think.

BTMash’s picture

#33: 1182296_33.patch queued for re-testing. Completely forgot that its only D8 patches that have changed :)

xjm’s picture

This patch looks really good overall. Minor observations follow. Reference: http://drupal.org/node/1354

  1. +++ b/modules/simpletest/tests/upgrade/upgrade.testundefined
    @@ -28,9 +28,56 @@ abstract class UpgradePathTestCase extends DrupalWebTestCase {
    +  function __construct($test_id = NULL) {
    

    This optional parameter should be documented in the docblock. E.g.:

    * @param $test_id
    *   (optional) The ID of the test (to do what with?), or NULL if (what?).  Defaults to NULL.
    
  2. +++ b/modules/simpletest/tests/upgrade/upgrade.testundefined
    @@ -28,9 +28,56 @@ abstract class UpgradePathTestCase extends DrupalWebTestCase {
    +   * Constructor for UpgradePathTestCase.
    ...
    +   * Prepare the appropriate session for the release of Drupal being upgraded.
    
    @@ -319,6 +378,26 @@ abstract class UpgradePathTestCase extends DrupalWebTestCase {
    + * Perform end-to-end point test of the release update path.
    

    These are just a couple examples; there are more throughout the file. In general the verb in the one-line summaries needs to be third person, e.g., "Tests the blahblah blah." They should also be a single line that's 80 chars or less. We're cleaning this up in #1310084: [meta] API documentation cleanup sprint, so we should make sure to follow the standard for these new docblocks. See http://drupal.org/node/1354#functions. Be sure to see the specific examples for constructors, implementations, overridden methods, etc. at http://drupal.org/node/1354#classes. (Of course, don't change any doxygen that's not already a part of your patch!)

  3. +++ b/modules/simpletest/tests/upgrade/upgrade.testundefined
    @@ -42,7 +89,8 @@ abstract class UpgradePathTestCase extends DrupalWebTestCase {
    -    // Generate a temporary prefixed database to ensure that tests have a clean starting point.
    +    // Generate a temporary prefixed database to ensure that tests have a
    +    // clean starting point.
    
    @@ -88,11 +136,16 @@ abstract class UpgradePathTestCase extends DrupalWebTestCase {
    -    // Reset all statics and variables to perform tests in a clean environment.
    +    // Reset all statics and variables to perform tests in a clean
    +    // environment.
    
    @@ -184,13 +229,14 @@ abstract class UpgradePathTestCase extends DrupalWebTestCase {
    -   * Specialized variable_set() that works even if the child site is not upgraded.
    +   * Specialized variable_set() that works even if the child site is not
    +   * upgraded.
    
    +++ b/modules/simpletest/tests/upgrade/upgrade.testundefined
    @@ -184,13 +229,14 @@ abstract class UpgradePathTestCase extends DrupalWebTestCase {
    -   *   The value to set. This can be any PHP data type; these functions take care
    -   *   of serialization as necessary.
    +   *   The value to set. This can be any PHP data type; these functions take
    +   *   care of serialization as necessary.
    

    We should not include these wrapping corrections in this patch, since these lines are not otherwise being changed.

  4. Some parts of the patch are a bit light on inline comments, but maybe they're not needed.

I'd suggest uploading an interdiff along with any revised patch, so that previous reviewers like catch can see exactly the changes you've made.

BTMash’s picture

FileSize
352.21 KB
6.33 KB

Thank you for the review, @xjm. I did not change the part regarding Contructor for UpgradePathTestCase since that seems to be the way it is defined through the codebase (and I don't recall seeing that being in third person. As you recommended, I've created an interdiff file (albeit in diff format) so the difference between the two patches can be seen.

xjm’s picture

#38: That's the way it is frequently in the codebase, but it's not the correct standard unfortunately. ;) See: http://drupal.org/node/1354#classes -- look for the example of the constructor there.

We are cleaning this up in the documentation sprint currently, so it would be good to have added code consistent with that. See: #1310084: [meta] API documentation cleanup sprint.

If you like/don't mind, I can roll a patch addressing those things.

BTMash’s picture

FileSize
352.2 KB
6.42 KB

Thank you for pointing that out (I had not seen that change; should have read the document more carefully). I need to learn and make sure I do this going forward. So I've re-rolled a patch (and interdiff). Hopefully, it all tests out :)

BTMash’s picture

#40: 1182296_39.patch queued for re-testing.

xjm’s picture

FileSize
22.71 KB

Alright, when I apply this patch locally and try to run BASIC STANDARD + ALL PROFILE UPDATE PATH, it fatals right away on setUp() with error:

An AJAX HTTP error occurred. HTTP Result Code: 500 Debugging information follows. Path: /d7git/batch?render=overlay&id=4&op=do StatusText: Internal Server Error ResponseText: Uncaught exception thrown in shutdown function.PDOException: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'd7.simpletest237142batch' doesn't exist: UPDATE {batch} SET batch=:db_update_placeholder_0 WHERE (bid = :db_condition_placeholder_0) ; Array ( [:db_update_placeholder_0] => a:13:{s:4:"sets";a:1:{i:0;a:14:{s:7:"sandbox";a:1:{s:3:"max";i:1;}s:7:"results";a:0:{}s:7:"success";b:0;s:5:"start";d:1321483451.0901629924774169921875;s:7:"elapsed";i:0;s:5:"title";s:13:"Running tests";s:8:"finished";s:26:"_simpletest_batch_finished";s:16:"progress_message";s:0:"";s:3:"css";a:1:{i:0;s:33:"modules/simpletest/simpletest.css";}s:12:"init_message";s:106:"Processing test 1 of 1 - <em class="placeholder">Basic standard + all profile update path</em>.<br/>&nbsp;";s:13:"error_message";s:22:"An error has occurred.";s:5:"total";i:1;s:5:"count";i:1;s:5:"queue";a:2:{s:4:"name";s:16:"drupal_batch:4:0";s:5:"class";s:10:"BatchQueue";}}}s:16:"has_form_submits";b:0;s:10:"form_state";a:3:{s:10:"programmed";b:0;s:7:"rebuild";b:0;s:8:"redirect";s:42:"admin/config/development/testing/results/3";}s:11:"progressive";b:1;s:11:"current_set";i:0;s:3:"url";s:5:"batch";s:11:"url_options";a:1:{s:5:"query";a:1:{s:6:"render";s:7:"overlay";}}s:10:"source_url";s:32:"admin/config/development/testing";s:8:"redirect";N;s:5:"theme";s:5:"seven";s:17:"redirect_callback";s:11:"drupal_goto";s:2:"id";s:1:"4";s:13:"error_message";s:81:"Please continue to <a href="/d7git/batch?id=4&amp;op=finished">the error page</a>";} [:db_condition_placeholder_0] => 4 ) in _batch_shutdown() (line 531 of /Applications/MAMP/htdocs/d7git/includes/batch.inc).

The next screen:

test_errors.png

I confirmed that zlib is enabled. I'm still assuming this is some configuration problem with my environment, though, since testbot runs fine, but I told @BTMash I'd post it in the issue. :)

xjm’s picture

So, followup: When testing this patch, apply it with git apply, not patch -p1. :)

xjm’s picture

Nitpicks inc:

+++ b/includes/utility.incundefined
@@ -46,6 +46,13 @@ function drupal_var_export($var, $prefix = '') {
+  else if (is_object($var) && get_class($var) === 'stdClass') {

Per our coding standards, elseif should be one word.

+++ b/modules/simpletest/tests/upgrade/upgrade.testundefined
@@ -28,9 +28,60 @@ abstract class UpgradePathTestCase extends DrupalWebTestCase {
+   * Constructs UpgradePathTestCase.

This should probably read "Constructs an UpgradePathTestCase object."

+++ b/modules/simpletest/tests/upgrade/upgrade.testundefined
@@ -319,6 +379,26 @@ abstract class UpgradePathTestCase extends DrupalWebTestCase {
+   * Override of UpgradePathTestCase::prepareD7Session().

This should say "Overrides" rather than "Override of."

+++ b/modules/simpletest/tests/upgrade/upgrade.testundefined
@@ -408,3 +488,309 @@ class BasicUpgradePath extends UpgradePathTestCase {
+   * Test a successful point release update.
...
+   * Test a successful point release update.
...
+   * Test a successful point release update.
...
+   * Test a successful point release update.

Should be "Tests" rather than "Test."

+++ b/scripts/dump-database-d7.shundefined
@@ -0,0 +1,90 @@
+/**
+ * Dump a Drupal 7 database into a Drupal 7 PHP script to test the upgrade
+ * process.
+ *
+ * Run this script at the root of an existing Drupal 7 installation.
+ *
+ * The output of this script is a PHP script that can be run inside Drupal 7
+ * and recreates the Drupal 7 database as dumped. Transient data from cache,
+ * session, and watchdog tables are not recorded.
+ */

I'm thinking this should be an @file? Also, for the summary, we can probably say "Dumps a Drupal 7 databse into a PHP script to test the upgrade process." That way it fits on one line and has the right verb tense.

+++ b/scripts/generate-d7-content.shundefined
@@ -0,0 +1,317 @@
+// Enable requested modules
...
+// Run cron after installing
...
+// Create six users
...
+// Create vocabularies and terms
...
+  // Create poll content
...
+    // Add some votes

Missing periods.

+++ b/scripts/generate-d7-content.shundefined
@@ -0,0 +1,317 @@
+    // Vocabularies without hierarcy get one term, single parent vocabularies get
+    // one parent and one child term. Multiple parent vocabularies get three
+    // terms: t0, t1, t2 where t0 is a parent of both t1 and t2.

"hierarchy" is misspelled here. Also, the word "get" is tiptoeing over the 80 char mark. And if you want to get REALLY nitpicky, the comma should be a semicolon. :)

I also did a bunch of manual testing, running the tests locally with both a clean version of 7.x and various fun bogus update hooks. The tests nicely catch various bad update hooks that no previously existing tests did--stuff like duplicating an update hook and giving it a new number, dropping the node table, etc. Plus, with multiple install profiles and generated database content, the tests are a much-needed foundation for any further test coverage we may need to add in the future.

@BTMash: Amazing work. :)

BTMash’s picture

FileSize
352.23 KB

@xjm Thank you for your reviews. Attaching a new patch which tries to resolve what you have found :) One thing I found is after an upgrade fails, the code still runs through logging in the user/whatnot and run the remainder of the tests. So I'm wondering if the following line:

$this->assertTrue($this->performUpgrade(), t('The upgrade was completed successfully.'));

Should instead change into:

if (!$this->assertTrue($this->performUpgrade(), t('The upgrade was completed successfully.'))) {
  return FALSE;
}

So it does not run the rest of the tests (since the main upgrade itself failed).

xjm’s picture

Added note about using git apply to the issue summary.

catch’s picture

Status: Needs review » Reviewed & tested by the community

I looked through this again and it all looks completely find.

chx looked through again and found one issue with a comment, but this is massively urgent to get in so we're going to open a follow-up for that rather than hold it up on a minor comment improvement. Marking RTBC.

chx’s picture

webchick’s picture

Excellent! Great to see this RTBC. Been seeing some weird bug reports about 7.9 which hopefully this can mitigate in the future.

I spent about 9 hours today with various travel-related dramas, so I don't trust myself to commit this tonight, but I'll take another look by the weekend.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Sorry, I really hate to mark such a patch 'needs work' for comments, but...

+++ b/scripts/generate-d7-content.sh
@@ -0,0 +1,317 @@
+$uid = 6;
+$node_type = 'broken';

I tried for quite awhile to figure out what this is trying to test, and failed. I asked in IRC and catch said:

webchick: it can't possibly be right though because those variables are ignored, then a new $node is created straight after.
i.e. those two lines could just be deleted from the patch.

I'm not sure if that's true or not, it seems to be $node_type vs. $node->type. However, the very end of this file is probably worth a look, and in any case we really need at least a comment to explain what the heck this part is testing.

13 days to next Drupal core point release.

BTMash’s picture

Argh...this would mean the dumps need to be recreated. Basically, $node->type is supposed to have the value of 'broken' so we are also testing for upgrades on content with an unknown bundle.

BTMash’s picture

Status: Needs work » Needs review
FileSize
353.61 KB

Ok, the dumps were rebuilt (probably for the better, they are consistent with the other dumps in using admin as the username / etc). Attaching revised patch. Local testing passes and hopefully testbot agrees.

chx’s picture

Status: Needs review » Reviewed & tested by the community

Addressed webchick's concern.

xjm’s picture

Be sure to use git apply rather than patch! :)

webchick’s picture

Status: Reviewed & tested by the community » Fixed

All right, FINALLY getting around to committing this! Very sorry. :\ Thank you SO much for all of your excellent work on this important issue!!

I was actually asking for a comment above that hunk, but re-rolling everything sounds good too. :) I added:

// Test that upgrade works even on a bundle whose parent module was disabled.

Hopefully that's right.

The only other hang-up I have really is that this patch moved the D7 and D8 upgrade path tests pretty far away from each other. There's all kinds of cosmetic clean-up in this patch (variable names, wrapper fucntions, etc.) that are not part of the D8 version of these tests. This would normally require kicking back to D8 for bringing that in-line, but this is critical enough that I decided to commit to D7 anyway. However, opened up #1352000: Forward-port upgrade test clean-ups from 7.x to 8.x as a follow-up, and it'd be nice to get that taken care of quickly.

Committed and pushed to 7.x. Thanks!

BTMash’s picture

This is great news :) I've commented in on that issue in being to look into it soon.

BTMash’s picture

I've found one more issue (this is in regards to #50 and after trying to generate programmatic dumps (based off #1333898: Support unstable2unstable upgrades? [policy, no patch]), I ran into a could pf PDO Exceptions. New dumps need to get generated...again (sigh) but should be faster. Will post this in a followup issue (and work on it after #1352000: Forward-port upgrade test clean-ups from 7.x to 8.x - might actually be a good followup in that issue for 7.x dump cleanups anywho).

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Dave Reid’s picture

Status: Closed (fixed) » Needs work

I'm wondering if anyone has actually tested these locally because these currently fail hard locally for multiple people. Mysteriously the test bot is not able to reproduce.

When I run any of the basic tests, I have a session_name() mismatch that causes the 'inner' Drupal install to think that the admin is not logged in.

Inside update_access_allowed() I added:
debug($_COOKIE);
debug(session_name());

When I run tests this is the output I get:

array (
  'SESSd73b71b47129dead6b79ab4a14df61d3' => 'TwnkXAPeePExrGgBit6rYBYRju4pbUU1F7qrRJT8t9g',
)

'SESS764ac9d6c93b04e279734d9b6fb0ee50'

Because session_name() does not return the same key as present in $_COOKIE, my user 1 doesn't get "logged in" when running the tests and causes the failures.

BTMash’s picture

@Dave Reid, I just did a git pull to get the latest code and tested it out locally - I wasn't able to get any issue. But at the same time, @matason and I were the ones that wrote the initial patch (and it worked at the time) so I'm not yet sure what could be happening.

scor’s picture

running --class BasicMinimalUpdatePath,FilledMinimalUpdatePath,BasicStandardUpdatePath,FilledStandardUpdatePath manually via the cli on a testbot and on localhost running MAMP, I get 1 fail for each of these test cases. They all look like this:

    User admin successfully logged in.
Fail      Other      upgrade.test       628 BasicMinimalUpdatePath->testBasicMi

There are also 1104 fails for Taxonomy upgrade path (UpgradePathTaxonomyTestCase) on the same machines

    Term term 1 of vocabulary 1 (j=0) (field-name-taxonomyextra) is not
    displayed on node 1
Fail      Other      upgrade.taxonomy.  157 UpgradePathTaxonomyTestCase->testTa
BTMash’s picture

Status: Needs work » Needs review
FileSize
1.78 KB

I ran though this and thanks to @rfay and @scor, I was able to replicate the issue. It is stemming from the fact that the tests were looking for 'drupal' as the site name while the actual site name was 'Drupal'. I'm not sure why some of the test bots caught it and others did not. However, this is wildly different from the issue @Dave Reid is having. I'm posting an initial patch that fixes that portion of the issue. I am seeing issues in 2 other things but they are unrelated from the patch that came in here.

scor’s picture

I can confirm this patch solves the bug on BasicMinimalUpdatePath, FilledMinimalUpdatePath, BasicStandardUpdatePath, and FilledStandardUpdatePath on both machines. The patch is only a case fixing. The testbot is a fresh debian that rfay built today PHP 5.3.3-7+squeeze6 with Suhosin-Patch (cli) (built: Jan 31 2012 10:56:51). My localhost is MAMP with PHP 5.3.6 (cli) (built: May 25 2011 20:42:15). I'm not sure why the testbot didn't run into this bug before.

I don't think this fixes the bug Dave found in #59 though.

BTMash’s picture

I was trying to figure out what is going on with the UpgradePathTaxonomyTestCase and why it is giving the issues. Thankfully the verbose message of the page + the tests gives a much clearer picture. The gist of it is that the prior xpath related code that was in there to check on the term name is no longer working (the classes are not there, it is simply a link). The easy solution in this is to check for $term->name since the term name is in the format 'term #term_nid of vocabulary #term_vid (j=SOMETHING)' and so the prior issue is no longer there. I am not attaching it as a patch as I believe it is unrelated to what is happening in here (will be trying to hunt down if there is an issue for this or maybe someone else might be able to say otherwise that it makes sense to include with this patch) but will paste it in.:

diff --git a/modules/simpletest/tests/upgrade/upgrade.taxonomy.test b/modules/simpletest/tests/upgrade/upgrade.taxonomy.test
index 37de087..67c4256 100644
--- a/modules/simpletest/tests/upgrade/upgrade.taxonomy.test
+++ b/modules/simpletest/tests/upgrade/upgrade.taxonomy.test
@@ -140,21 +140,12 @@ class UpgradePathTaxonomyTestCase extends UpgradePathTestCase {
           '%nid' => $nid,
         );
 
-        // Use link rather than term name because migrated term names can be
-        // substrings of other term names. e.g. "term 1 of vocabulary 2" is
-        // found when "term 1 of vocabulary 20" is output.
         $term_path = url('taxonomy/term/' . $term->tid);
         if (!$should_be_displayed) {
-          // Look for any link with the term path.
-          $links = $this->xpath('//a[@href=:term_path]', array(':term_path' => $term_path));
-          $this->assertFalse($links, t('Term %name (@field) is not displayed on node %nid', $args));
+          $this->assertNoText($term->name);
         }
         else {
-          // Look for a link with the term path inside the correct field.
-          // We search for "SPACE + class + SPACE" to avoid matching a substring
-          // of the class.
-          $links = $this->xpath('//div[contains(concat(" ", normalize-space(@class), " "), :field_class)]//a[@href=:term_path]', array(':field_class' => ' ' . $field_class . ' ', ':term_path' => $term_path));
-          $this->assertTrue($links, t('Term %name (@field) is displayed on node %nid', $args));
+          $this->assertText($term->name);
         }
       }

Additionally...it may be the case that this patch is not what should be fixed but something else in the taxonomy module that is causing this issue to occur to begin with (I'm not entirely sure and getting some help on it would be much appreciated).

clemens.tolboom’s picture

Where is the motivation to gz all dump files? This is kinda not working for ie #1410096: Convert comment language code schema to langcode and #1357918: Missing update for language_default in language langcode update which are D8MI issues.

Both issues had 'cannot apply' for patch reviewers and uploading the full blob db dump does not help much in diffing too.

Can we please go back to plain db dumps?

[Edit] I guess there is motivation for this. Maybe we could add these to this issue summary or a better place?

catch’s picture

@clemens.tolboom: sun's working on removing the db dumps altogether at #1364798: Impossible to generate meaningful diffs of upgrade db dumps.

catch’s picture

Title: Add tests for 7.0->7.x upgrade path » Add tests for 7.0->7.x upgrade path (random test failures)
Category: task » bug
xjm’s picture

Tagging.

catch’s picture

#62: 1182296.patch queued for re-testing.

catch’s picture

Status: Needs review » Reviewed & tested by the community

#62 looks RTBC.

@BTMash, do we need a follow-up to look at #64?

BTMash’s picture

@catch, sorry for my delayed response. If the tests are passing (and in this scenario, since the change is quite minimal), I don't think we need to.

However, @clemens.tolboom brings up a good point that the gzipped dump files are not effective in seeing the changes that go into the dumps and they are, for most, impossible to debug. I know I had talked with @chx when we had started creating the boilerplate tests from 7.x to 8.x that the patch files were quite large (1.5+ megs for a standard+all option core modules dump with a couple of fields was the size of my initial patches). And gzip had reduced the size by a ton but its obviously brought along other problems. It might be wise to revisit this in another issue and perhaps just have the dumps in non-zipped format. @Dave Reid had an issue as well but I haven't heard back from him on whether he continues to have this issue. @xjm had run into a similar issue with the session which we resolved at http://drupal.org/node/1280792#comment-5566078. So if anyone else wants to chime in, I can look at it all and update the issue summary as best I can.

BTMash’s picture

#62: 1182296.patch queued for re-testing.

pillarsdotnet’s picture

rfay’s picture

Current fail today is *not* in any way random.

jhodgdon’s picture

Status: Reviewed & tested by the community » Fixed

The lack of having committed the patch in #62 is apparently currently breaking 7.x, due to I'm not sure what other commit exposed this problem... But I've been asked by timplunkett to commit it and it does appear to be directly relate to the broken 7.x branch, so I went ahead and committed the patch.

pillarsdotnet’s picture

Confirmed that 7.x HEAD is now passing tests.

BTMash’s picture

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.