Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
#2551341: Update test database dump should be based on beta 12 and contain content added sufficient content in the database dump, however we only added implicit tests to cover the existence of the data. We should add functional tests after the upgrade has run that the content/config entities are accessible and have the right data.
Proposed resolution
Write the tests.
Remaining tasks
Review/improve the tests.
Expand test coverage if needed.
User interface changes
N/A
API changes
N/A
Data model changes
N/A
Comment | File | Size | Author |
---|---|---|---|
#72 | 2554151-72.patch | 16.4 KB | plach |
#72 | 2554151-72.interdiff.txt | 676 bytes | plach |
#65 | interdiff-62-64.txt | 1009 bytes | stefan.r |
#65 | 2554151-64.patch | 16.48 KB | stefan.r |
#62 | 2554151-62.patch | 16.48 KB | stefan.r |
Comments
Comment #2
catchTagging this to review how things are going before we tag a release candidate. We've had several silent failures in the upgrade path which may or may not have been found just by adding data to the update test dumps. @alexpott's concern in the other issue was that just having some data in the dump could lull us into a false sense of security. If there are any number in the 'may not' category, we may want to make catch-all tests for this critical after all.
Comment #3
alexpottMaking critical as per #2551341-35: Update test database dump should be based on beta 12 and contain content. Without tests that issue is not really done but having it committed helps other patches - so this issue takes over the critical mantle.
Comment #4
Gábor HojtsyIn case this is critical, it is by definition revisit before RC :D
Comment #5
csg CreditAttribution: csg at Cheppers for Acquia commentedComment #6
csg CreditAttribution: csg at Cheppers for Acquia commentedI wrote two tests, one for the site name and another one for the article node for a quick start. Please let me know if this approach is what you had in mind.
Comment #7
stefan.r CreditAttribution: stefan.r commentedWe already have a test for the site name so that won't be needed, but this seems to be in the right direction with the test for the node title. So this just needs more tests asserting the content (which includes contact forms, nodes, field content, comments, menu links, and anything listed in the IS of the link issues) is still sane.
Comment #9
Gábor HojtsyYeah the question is how far we should go with testing things, there are like several dozen objects on the db :)
BTW I reviewed the test live and did suggest @csg to include the site name test. I do see now that we have UpdatePathTestBaseFilledTest which is testing it in the filled DB via UpdatePathTestBaseTest.
Comment #10
Gábor Hojtsy@stefan.r: what's more interesting is how does the dump have a syntax error now? We don't change the dump and it is already used for testing all kinds of things... The patch fails with:
So the dump is problematic... That line is the source line from:
Comment #11
Gábor HojtsyFixing that line to
'source' => '<strong>Reminder: don\'t forget to set the <code>$settings[\'update_free_access\']
value in yoursettings.php
file back toFALSE
.',Of course its in a gz, so you cannot review it. I promise I did not change anything else in the gz.
Also expanding the node tests and removing the site name test as per above.
Comment #12
Gábor HojtsyHaha, there is one more place for a syntax error in the translation for that string of course. Fixing that should work now.
Comment #13
Gábor HojtsyAnd fixing the rest of the tests, which were not even testing with this data, haha.
Comment #14
jhedstromThis should be flipped around as the others or it will load the bare db.
If we really want to be thorough, I think we should test some raw data from the db dump prior to running updates. Not super-necessary, but a pattern that has been followed in head2head for the various update hook tests.
Comment #18
stefan.r CreditAttribution: stefan.r commented@Gabor that doesn't make sense, the filled dump already did have some of its own tests in the critical. Note how an earlier iteration was red (with that same error): https://www.drupal.org/node/2551341#comment-10226689
Comment #19
stefan.r CreditAttribution: stefan.r commentedComment #20
stefan.r CreditAttribution: stefan.r commentedOuch, just read #2551341-41: Update test database dump should be based on beta 12 and contain content. So the filled dump was never tested... That's ugly :(
Can't we find a way to fix the syntax error from happening while the gz contents get included in the first place without having to fix the dump? Otherwise we may run into this same problem again when changing/adding a dump?
Comment #21
stefan.r CreditAttribution: stefan.r commented@jhedstrom re #15, flipping around will still not fix it, the dump is loaded in the parent setUp method so it's too late by then. We'll rather need to test the parent tests.
Comment #22
catchShall we just stop gzipping the dumps? I think we're past the point where the size of the core tarball is our biggest problem - if we remove one js file for authenticated users in the standard profile that'll save terabytes more bandwidth over the years.
We should probably spin off a separate issue to fix the dump itself, otherwise this issue still blocks #2542748: Automatic entity updates can fail when there is existing content, leaving the site's schema in an unpredictable state which was the point of splitting the tests out in the first place. It'd be great to see how many of those fails from #13 the entity updates issue fixes.
Comment #23
stefan.r CreditAttribution: stefan.r commentedcreated #2555183: Fix the filled update tests, they are broken
Comment #24
dawehnerLet's try a combined patch
Comment #25
Wim LeersThis issue was discussed by Alex Pott, catch, dawehner and plach on the EU criticals call. The conclusion was that we should just do a smoke test: check for the presence of everything in the dump, e.g. for all nodes, go to the full node page, and check that the node title is present.
Comment #27
dawehnerFirst follow up: #2555259: Add config schema for image_convert image effect
Comment #28
dawehnerSecond one: #2555275: Fix missing schema for URI widget
Comment #29
Gábor HojtsyIf those "followups" are required for this critical to pass in the first place, not sure we can do them after this one to follow this patch up. Why not include here? We have at least implicit coverage for them then. (We did the same for migrate exposed config schema issues).
Comment #30
catchSo stefan_r posted #2555183: Fix the filled update tests, they are broken. I think I agree with an issue to fix the tests we thought we had, then another issue to add the tests we don't have yet.
However #2555183: Fix the filled update tests, they are broken is required to fix this one, so either we need to bump that to critical and postpone this on that, or swap the titles around.
I bumped the two follow-ups to critical for now - we might want to do them in separate issues just to discuss one thing at a time, but they will indeed block this one.
Comment #31
catchPostponing on #2555183: Fix the filled update tests, they are broken.
Comment #32
dawehner.
Comment #33
webchick#2555183: Fix the filled update tests, they are broken is in. Onward! :D
Comment #34
stefan.r CreditAttribution: stefan.r commentedReroll of the test from #13, which will need to be updated to check for all the content/config listed in #2551341: Update test database dump should be based on beta 12 and contain content
Comment #36
stefan.r CreditAttribution: stefan.r commentedHere's another attempt of a reroll of the test @csg wrote.
Also including SQL dumps of the beta12 filled database before and after running update.php in HEAD in case anyone wants to work further on this.
Comment #38
Berdir$node->label() or $node->getTitle() or $node->title->value.
Comment #39
stefan.r CreditAttribution: stefan.r commentedAdding some web tests.
Comment #40
stefan.r CreditAttribution: stefan.r commentedOne test fail related to the upgrade path.
Before (beta12):
After updating to HEAD (disregard the missing druplicon, it's not in the dump):
Here, field #12 disappeared on the node view (where the labels of some other fields also are styled differently), but it's still in the edit form, with another value (it's an entity reference to a user).
Comment #42
stefan.r CreditAttribution: stefan.r commentedComment #44
stefan.r CreditAttribution: stefan.r commentedUpdated remaining tasks in IS. We need to fix the test failure mentioned in #40 and perhaps it would be nice to get some of the admin paths by their routes instead of hard-coding the paths in case they change?
Maybe we could also attempt an uninstall of all modules as a sanity check. I may be wrong but for if I remember correctly the Simpletest module doesn't let itself uninstall after running update.php as it is missing a table.
Comment #45
stefan.r CreditAttribution: stefan.r commentedActually the bug in #40 may not be an upgrade issue. It's in WebTestBase where user 1 is called admin, whereas in the dump it's called drupal (with email drupal@example.com).
Comment #46
stefan.r CreditAttribution: stefan.r commentedIt was an entity reference to the admin user so it was access-restricted
Comment #48
stefan.r CreditAttribution: stefan.r commented#46 will need a reroll
Comment #49
Gábor HojtsyRerolled.
Comment #50
catchOpened an issue for the user/1 @todo: #2560237: UpdatePathTestBase saves the root user before updates have run.
Comment #51
stefan.r CreditAttribution: stefan.r commented@catch its not really a todo, it's already in this patch and a ver straightforward change. Given that, do we really need a separate issue? I'm fine to take it out of this patch and put it in there if we think it's better?
Comment #52
Gábor HojtsyWell its a @todo that predates this patch at least :)
Comment #53
catch@stefan_r moving it to the new method is fine here.
This was just a handy critical upgrade path issue to spam that link to...
The only thing we could possibly do here, is update the @todo to point to the issue now.
Comment #54
Gábor HojtsyAdded link to todo. Also hopefully better explanation on the problem not just what to do with it :)
Comment #55
Gábor HojtsyReviewed the tests (it is only similar to a couple lines now to what @csg and I initially posted). They look fine as smoke tests. I think its best to test then with the display and forms as they are now rather than just loading objects with the API because the display and forms also make assumptions about them, so they are better smoke tests as to whether the data is in the right format than just loading the data.
LOL :P
The uninstall idea is interesting, we should indeed explore it especially if we expect it will uncover bugs with the upgrade path. Setting to needs work for that.
Comment #56
plachI checked the list of items posted at #2551341: Update test database dump should be based on beta 12 and contain content and it seems the following were not explicitly checked:
Can we explicitly check the fields page?
Also the following items were only partially checked (mainly translation was left out):
Comment #57
stefan.r CreditAttribution: stefan.r commented@plach it seems like a good idea to add tests for all those as well. I belive the comment is already checked for, but just missing a code comment explaining where this happens:
Comment #58
plachOh, yeah, missed that. Quick code review:
We should pass language as an option, not hard-coding it in the path.
Can we add an empty line to separate this from the other code block?
If I'm not mistaken this line should be moved after the comment below.
As above, we should pass language as an option.
Comment #59
Gábor HojtsyFixing #58 and that the method comment and test method name were outdated. Did not address #56/#57 yet.
Comment #60
stefan.r CreditAttribution: stefan.r commentedI'll post a patch for #56/#57 in a minute.
Just need an uninstall test now, i.e. going to admin/modules/uninstall, checking everything that's checkable and keep submitting until everything is unselected. In manual testing this broke for me :)
This was not technically true, checked in the dump and it was the slogan not the site name. So let's update that in the parent issue.
Comment #61
Gábor HojtsyLooking at #56.
1. Node/8 comments got their code comment now.
2. Site name is not translated in the dump, only site slogan. That is already tested for. From the dump:
3. The translated content view is tested already
4. Do we know what user field is that, I have a hard time grepping the db dump :/
(did not yet continue looking at the further ones).
Comment #62
stefan.r CreditAttribution: stefan.r commentedComment #63
Gábor Hojtsy@stefan.r: updates in #62 look great!
Comment #64
Gábor HojtsyComment #65
stefan.r CreditAttribution: stefan.r commentedComment #67
catchWe have an overridden menu link in the database dump, but no test coverage for it yet.
See #2341575-52: [meta] Provide a beta to beta/rc upgrade path and #2548009: Overridden menu links lose parent, expanded and enabled (are disabled) status on cache clear.
Comment #68
stefan.r CreditAttribution: stefan.r commented#67 that was not in this dump :)
As to the uninstall idea mentioned in #55, we'd need to delete all content for that first, which would be an interesting second smoke test:
Then we want to purge everything (field_purge_batch()?)
And then we have a couple of rounds of going to the uninstall screen and resolving dependencies. The biggest issue in my manual testing was getting rid of all the content/traces of every field before it lets itself be uninstalled, and uninstalling dblog along with other stuff also seemed problematic as some of it calls watchdog during uninstall.
Also uninstallling everything may be a bit optimistic, even manually it's really not that easy to install everything and then uninstall everything, so maybe let's aim for uninstalling 90% of modules?
Also the dump script doesn't dump the
simpletest_test_id
table for some reason, we could create a separate issue to add that manually into our dump, as Simpletest won't let itself uninstall without that table existing.I am done with this for today, in case anyone wants to give this a go!
Comment #69
catchI think we should move the uninstall to a separate issue.
It's a good idea, but I'm not immediately seeing how we'd regress uninstall after an update that wouldn't also affect new installs, so feels less important than the smoke testing on content here.
Comment #70
plach+1 for #69, I was slightly worried about the implications of the uninstallation work too.
Comment #71
plachIn light of #69, if we remove this line we should be ready to go. The bugs I found in #2341575-52: [meta] Provide a beta to beta/rc upgrade path now have their own issues.
Comment #72
plachThe interdiff is ridiculously trivial, so it's should be fine for me to RTBC this. Let's see whether committers agree :)
Comment #73
Gábor HojtsyYeah agreed looks good :)
Comment #74
catchThis looks great. We can add more assertions and/or content when new things come up. Also it's a relief that this didn't uncover even more bugs than we already have.
Committed/pushed to 8.0.x, thanks!