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.
Comment | File | Size | Author |
---|---|---|---|
#62 | 1182296.patch | 1.78 KB | BTMash |
#52 | 1182296_52.patch | 353.61 KB | BTMash |
#45 | 1182296_45.patch | 352.23 KB | BTMash |
#42 | test_errors.png | 22.71 KB | xjm |
#40 | 1182296_39-interdiff.txt | 6.42 KB | BTMash |
Comments
Comment #1
catchThe 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.
Comment #2
catchDoh, 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.
Comment #3
Damien Tournoud CreditAttribution: Damien Tournoud commentedSee http://drupal.org/sandbox/damz/1174672 for (1).
Comment #4
catch#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.
Comment #5
jcisio CreditAttribution: jcisio commentedsubscribe
Comment #6
catchOptimistically un-postponing this now that #1182290: Add boilerplate upgrade path tests is RTBC.
Comment #7
matason CreditAttribution: matason commentedInitial stab at a patch, not sure if I've created it correctly though (includes binary files).
Comment #8
BTMash CreditAttribution: BTMash commentedI 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.
Comment #9
matason CreditAttribution: matason commentedI'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.
Comment #10
matason CreditAttribution: matason commentedI'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!
Comment #11
BTMash CreditAttribution: BTMash commentedI 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.
Comment #12
BTMash CreditAttribution: BTMash commentedThinking 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 :)
Comment #13
penyaskitoSubscribing.
Comment #14
BTMash CreditAttribution: BTMash commentedTalking 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).
Comment #15
BTMash CreditAttribution: BTMash commented#10: 1182296_10.patch queued for re-testing.
Comment #17
BTMash CreditAttribution: BTMash commentedI 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...
Comment #18
BTMash CreditAttribution: BTMash commentedRerolled to remove whitespace issues.
Comment #20
BTMash CreditAttribution: BTMash commentedWow...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.
Comment #22
BTMash CreditAttribution: BTMash commentedI 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.
Comment #23
BTMash CreditAttribution: BTMash commentedAfter 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.
Comment #24
BTMash CreditAttribution: BTMash commentedack...setting to needs review.
Comment #26
BTMash CreditAttribution: BTMash commentedOk, this was due to an event where there are no images. Revised patch.
Comment #27
BTMash CreditAttribution: BTMash commentedmarked as 'needs testing'
Comment #28
BTMash CreditAttribution: BTMash commentedargh...sandbox['total'] is not an array.
Comment #29
BTMash CreditAttribution: BTMash commented#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.
Comment #30
webchickI 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!!
Comment #31
BTMash CreditAttribution: BTMash commentedIt 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.
Comment #32
catchThe method header looks generic, but then we have D7-specific code in there. Could we make it one way or the other?
Exceeds 80 chars (and elsewhere).
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.
Comment #33
BTMash CreditAttribution: BTMash commentedThanks 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.Comment #34
BTMash CreditAttribution: BTMash commentedMarking 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 :)
Comment #35
catchIt's a D7 patch so it ought to be fine I think.
Comment #36
BTMash CreditAttribution: BTMash commented#33: 1182296_33.patch queued for re-testing. Completely forgot that its only D8 patches that have changed :)
Comment #37
xjmThis patch looks really good overall. Minor observations follow. Reference: http://drupal.org/node/1354
This optional parameter should be documented in the docblock. E.g.:
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!)
We should not include these wrapping corrections in this patch, since these lines are not otherwise being changed.
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.
Comment #38
BTMash CreditAttribution: BTMash commentedThank 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.Comment #39
xjm#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.
Comment #40
BTMash CreditAttribution: BTMash commentedThank 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 :)
Comment #41
BTMash CreditAttribution: BTMash commented#40: 1182296_39.patch queued for re-testing.
Comment #42
xjmAlright, when I apply this patch locally and try to run
BASIC STANDARD + ALL PROFILE UPDATE PATH
, it fatals right away onsetUp()
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/> ";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&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:
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. :)
Comment #43
xjmSo, followup: When testing this patch, apply it with
git apply
, notpatch -p1
. :)Comment #44
xjmNitpicks inc:
Per our coding standards,
elseif
should be one word.This should probably read "Constructs an UpgradePathTestCase object."
This should say "Overrides" rather than "Override of."
Should be "Tests" rather than "Test."
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.
Missing periods.
"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. :)
Comment #45
BTMash CreditAttribution: BTMash commented@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:
Should instead change into:
So it does not run the rest of the tests (since the main upgrade itself failed).
Comment #46
xjmAdded note about using
git apply
to the issue summary.Comment #47
catchI 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.
Comment #48
chx CreditAttribution: chx commentedComment followup at #1344312: Comment in upgrade test setup is out of place
Comment #49
webchickExcellent! 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.
Comment #50
webchickSorry, I really hate to mark such a patch 'needs work' for comments, but...
I tried for quite awhile to figure out what this is trying to test, and failed. I asked in IRC and catch said:
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.
Comment #51
BTMash CreditAttribution: BTMash commentedArgh...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.
Comment #52
BTMash CreditAttribution: BTMash commentedOk, 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.
Comment #53
chx CreditAttribution: chx commentedAddressed webchick's concern.
Comment #54
xjmBe sure to use
git apply
rather thanpatch
! :)Comment #55
webchickAll 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:
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!
Comment #56
BTMash CreditAttribution: BTMash commentedThis is great news :) I've commented in on that issue in being to look into it soon.
Comment #57
BTMash CreditAttribution: BTMash commentedI'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).
Comment #59
Dave ReidI'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:
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.
Comment #60
BTMash CreditAttribution: BTMash commented@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.
Comment #61
scor CreditAttribution: scor commentedrunning --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:
There are also 1104 fails for Taxonomy upgrade path (UpgradePathTaxonomyTestCase) on the same machines
Comment #62
BTMash CreditAttribution: BTMash commentedI 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.
Comment #63
scor CreditAttribution: scor commentedI 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.
Comment #64
BTMash CreditAttribution: BTMash commentedI 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.: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).
Comment #65
clemens.tolboomWhere 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?
Comment #66
catch@clemens.tolboom: sun's working on removing the db dumps altogether at #1364798: Impossible to generate meaningful diffs of upgrade db dumps.
Comment #67
catchComment #68
xjmTagging.
Comment #69
catch#62: 1182296.patch queued for re-testing.
Comment #70
catch#62 looks RTBC.
@BTMash, do we need a follow-up to look at #64?
Comment #71
BTMash CreditAttribution: BTMash commented@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.
Comment #72
BTMash CreditAttribution: BTMash commented#62: 1182296.patch queued for re-testing.
Comment #73
pillarsdotnet CreditAttribution: pillarsdotnet commentedDuplicate issue? #1496396: All 7.x tests postponed because Drupal core upgrade tests are failing.
Comment #74
rfayCurrent fail today is *not* in any way random.
Comment #75
jhodgdonThe 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.
Comment #76
pillarsdotnet CreditAttribution: pillarsdotnet commentedConfirmed that 7.x HEAD is now passing tests.
Comment #77
BTMash CreditAttribution: BTMash commentedPosting a followup for #64 at #1504994: Upgrade tests for taxonomy are failing
Comment #78.0
(not verified) CreditAttribution: commentedUpdated issue summary.