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.
Comment | File | Size | Author |
---|---|---|---|
#49 | 1182290_49.patch | 190.02 KB | BTMash |
#48 | 1182290_48.patch | 190.03 KB | BTMash |
#47 | 1182290_47.patch | 191.4 KB | BTMash |
#44 | 1182290_44.patch | 177.9 KB | matason |
#44 | 1182290_44_failing.patch | 178.4 KB | matason |
Comments
Comment #1
catchCrossposting 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.
Comment #2
marcingy CreditAttribution: marcingy commentedneed this for http://drupal.org/node/1161486 so will happily start the process
Comment #3
catchBumping 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.
Comment #4
marcingy CreditAttribution: marcingy commentedUnassigning as I'm just not finding time to work on this
Comment #5
catchI'm uploading a patch that shows the current problem. With any test coverage at all there would be a fatal error.
Comment #6
catchFollowing 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
Comment #7
franzsubscribing
Comment #8
catchBack to active
Comment #9
mdm CreditAttribution: mdm commentedI'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?
Comment #10
catch#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!
Comment #11
mdm CreditAttribution: mdm commentedThis 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...
Comment #12
plach@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.
Comment #13
Lars Toomre CreditAttribution: Lars Toomre commentedIssue #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.
Comment #14
jcisio CreditAttribution: jcisio commentedsubscribe
Comment #15
chx CreditAttribution: chx commentedjcisio and I are on this.
Comment #16
chx CreditAttribution: chx commentedPlease review the generate and the dump scripts. I copied the object fix from ctools into the drupal_var_export function.
Comment #18
BTMash CreditAttribution: BTMash commentedThis 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 :)
Comment #19
BTMash CreditAttribution: BTMash commented#16: 1182290.patch queued for re-testing.
Comment #20
chx CreditAttribution: chx commentedYeah, the uid - 1 stuff is completely unnecessary now, users.uid is not a serial now.
Comment #21
BTMash CreditAttribution: BTMash commentedOk, 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)
Comment #22
jcisio CreditAttribution: jcisio commentedYes I think we need some tests for data consistency after upgrade.
Comment #23
podaroksubscribe
Comment #24
BTMash CreditAttribution: BTMash commentedThis 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 :)
Comment #26
tstoecklerEDIT: This was a crosspost with the patch above, I have no idea, how much of the below is still valid.
ran -> run
Drupal 7 -> Drupal 8 (I think?)
"...cache, session, and watchdog..." (missing commas)
This seems duplicate?
Should we determine the minor Drupal version or is 7.0 ok? What about possible future Schema updates in 7.x?
d6 -> d7
Not too firm with Heredoc, but doesn't this insert a blank line in the middle of the PHPDoc?
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.
Drupal 6 -> Drupal 7
(also below)
See above.
Shouldn't that include taxonomy.module then?
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.
Comment #27
BTMash CreditAttribution: BTMash commentedI 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.
Comment #28
BTMash CreditAttribution: BTMash commentedShould have marked as 'needs review'
Comment #29
chx CreditAttribution: chx commentedThis one has the tests. Not much of a test. We probably want to nuke them later. But this issue is about boilerplate.
Comment #30
chx CreditAttribution: chx commentedThis one has the tests. Not much of a test. We probably want to nuke them later. But this issue is about boilerplate.
Comment #31
chx CreditAttribution: chx commentedmatason uploaded this for me 'cos with my slow connection and the huge patch I was timing out.
Comment #32
BTMash CreditAttribution: BTMash commentedSorry 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.
Comment #33
matason CreditAttribution: matason commentedUploaded new patch, combination of patches from #5 and #31 as requested by @chx
Comment #34
chx CreditAttribution: chx commentedTo clarify: #32 is probably RTBC, #33 is to show tests are working.
Comment #35
aspilicious CreditAttribution: aspilicious commentedI'm confused...
Why is #33 not failing?
The bananas() function doesn't get loaded or the test would fail... Or not?
Comment #36
BTMash CreditAttribution: BTMash commentedI 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.
Comment #37
chx CreditAttribution: chx commentedSo, 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.
Comment #38
BTMash CreditAttribution: BTMash commentedThe 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.Comment #39
BTMash CreditAttribution: BTMash commentedI 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).
Comment #40
chx CreditAttribution: chx commentedOK 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.
Comment #41
chx CreditAttribution: chx commentedThis 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.
Comment #42
catchLet's never go without upgrade path testing ever, ever again. Great work btmash and chx!
Comment #43
tstoecklerTrailing whitespace.
New patches attached.
I literally edited the patch files, so I'm leaving at RTBC.
Comment #44
matason CreditAttribution: matason commentedI 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.
Comment #46
BTMash CreditAttribution: BTMash commentedMarking 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.
Comment #47
BTMash CreditAttribution: BTMash commentedThis 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.
Comment #48
BTMash CreditAttribution: BTMash commentedI somehow had the .install file changes in there which shouldn't have been the case. Updated patch without .install changes.
Comment #49
BTMash CreditAttribution: BTMash commentedPatch with formatting suggestions by chx.
Comment #50
chx CreditAttribution: chx commentedWhile 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.
Comment #51
webchickYoinking 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.
Comment #52
catchQuite 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.
Comment #53
catchComment #54
Dries CreditAttribution: Dries commentedCommitted 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.
Comment #55
chx CreditAttribution: chx commentedNo, #1182296: Add tests for 7.0->7.x upgrade path (random test failures) is the D7 issue.
Comment #56
chx CreditAttribution: chx commentedAlso: YAY YAY YAY YAY YAY
Comment #57
plachMoving back to D8.