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

CommentFileSizeAuthor
#72 2554151-72.patch16.4 KBplach
#72 2554151-72.interdiff.txt676 bytesplach
#65 interdiff-62-64.txt1009 bytesstefan.r
#65 2554151-64.patch16.48 KBstefan.r
#62 2554151-62.patch16.48 KBstefan.r
#62 interdiff-60-62.txt2.7 KBstefan.r
#61 interdiff.txt663 bytesGábor Hojtsy
#61 2554151-60.patch15.2 KBGábor Hojtsy
#59 interdiff.txt2.63 KBGábor Hojtsy
#59 2554151-59.patch15.17 KBGábor Hojtsy
#54 interdiff.txt760 bytesGábor Hojtsy
#54 2554151-54.patch15.02 KBGábor Hojtsy
#49 2554151-49.patch14.96 KBGábor Hojtsy
#46 2554151-46.patch14.67 KBstefan.r
#42 2554151-41.patch12.76 KBstefan.r
#40 fields (after).png34.38 KBstefan.r
#40 after upgrade.png44.18 KBstefan.r
#40 before (beta12).png97.64 KBstefan.r
#39 2554151-39.patch12.74 KBstefan.r
#36 beta12-filled.sql_.gz529.41 KBstefan.r
#36 filled-dump-after-update.sql_.gz849.5 KBstefan.r
#36 2554151-reroll.patch1.56 KBstefan.r
#34 2554151.patch1.5 KBstefan.r
#24 2554151.patch598.28 KBdawehner
#13 interdiff.txt413.35 KBGábor Hojtsy
#13 2554151-13.patch531.3 KBGábor Hojtsy
#12 interdiff.txt408.01 KBGábor Hojtsy
#12 2554151-12.patch525.95 KBGábor Hojtsy
#11 interdiff.txt526.1 KBGábor Hojtsy
#11 2554151-11.patch525.95 KBGábor Hojtsy
#6 2554151.patch1.35 KBcsg
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy created an issue. See original summary.

catch’s picture

Issue tags: +revisit before release candidate

Tagging 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.

alexpott’s picture

Priority: Major » Critical

Making 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.

Gábor Hojtsy’s picture

Issue tags: -revisit before release candidate

In case this is critical, it is by definition revisit before RC :D

csg’s picture

Assigned: Unassigned » csg
csg’s picture

Assigned: csg » Unassigned
Status: Active » Needs review
FileSize
1.35 KB

I 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.

stefan.r’s picture

We 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.

Status: Needs review » Needs work

The last submitted patch, 6: 2554151.patch, failed testing.

Gábor Hojtsy’s picture

Yeah 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.

Gábor Hojtsy’s picture

@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:

Parse error: syntax error, unexpected '' (T_ENCAPSED_AND_WHITESPACE), expecting identifier (T_STRING) or variable (T_VARIABLE) or number (T_NUM_STRING) in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/system/tests/fixtures/update/drupal-8.filled.standard.php.gz on line 59555
FATAL Drupal\system\Tests\Update\UpdatePathTest: test runner returned a non-zero error code (255).

So the dump is problematic... That line is the source line from:

values(array(
  'lid' => '7120',
  'source' => "<strong>Reminder: don't forget to set the $settings[&#039;update_free_access&#039;] value in your settings.php file back to FALSE.</strong>",
  'context' => '',
  'version' => 'none',
))
Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
525.95 KB
526.1 KB

Fixing that line to

'source' => '<strong>Reminder: don\'t forget to set the <code>$settings[\'update_free_access\'] value in your settings.php file back to FALSE.',

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.

Gábor Hojtsy’s picture

Haha, there is one more place for a syntax error in the translation for that string of course. Fixing that should work now.

Gábor Hojtsy’s picture

And fixing the rest of the tests, which were not even testing with this data, haha.

jhedstrom’s picture

  1. +++ b/core/modules/system/src/Tests/Update/UpdatePathTest.php
    @@ -0,0 +1,49 @@
    +    $this->databaseDumpFiles = [__DIR__ . '/../../../tests/fixtures/update/drupal-8.filled.standard.php.gz'];
    +    parent::setUp();
    

    This should be flipped around as the others or it will load the bare db.

  2. +++ b/core/modules/system/src/Tests/Update/UpdatePathTest.php
    @@ -0,0 +1,49 @@
    +    $this->runUpdates();
    

    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.

The last submitted patch, 11: 2554151-11.patch, failed testing.

The last submitted patch, 12: 2554151-12.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 13: 2554151-13.patch, failed testing.

stefan.r’s picture

@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:

@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

stefan.r’s picture

stefan.r’s picture

Ouch, 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?

stefan.r’s picture

@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.

catch’s picture

Shall 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.

stefan.r’s picture

dawehner’s picture

Status: Needs work » Needs review
FileSize
598.28 KB

Let's try a combined patch

Wim Leers’s picture

Yeah the question is how far we should go with testing things, there are like several dozen objects on the db :)

This 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.

Status: Needs review » Needs work

The last submitted patch, 24: 2554151.patch, failed testing.

dawehner’s picture

dawehner’s picture

Gábor Hojtsy’s picture

If 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).

catch’s picture

So 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.

catch’s picture

Status: Needs work » Postponed
dawehner’s picture

Title: Test content/configuration in update database dump » [pp-1] Test content/configuration in update database dump

.

webchick’s picture

Title: [pp-1] Test content/configuration in update database dump » Test content/configuration in update database dump
Status: Postponed » Needs work
stefan.r’s picture

Status: Needs work » Needs review
FileSize
1.5 KB

Reroll 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

Status: Needs review » Needs work

The last submitted patch, 34: 2554151.patch, failed testing.

stefan.r’s picture

Status: Needs work » Needs review
FileSize
1.56 KB
849.5 KB
529.41 KB

Here'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.

Status: Needs review » Needs work

The last submitted patch, 36: 2554151-reroll.patch, failed testing.

Berdir’s picture

+++ b/core/modules/system/src/Tests/Update/UpdatePathTestBaseFilledTest.php
@@ -21,4 +23,31 @@ protected function setDatabaseDumpFiles() {
+      $this->assertEqual($node->title, $title);

$node->label() or $node->getTitle() or $node->title->value.

stefan.r’s picture

Status: Needs work » Needs review
FileSize
12.74 KB

Adding some web tests.

stefan.r’s picture

Issue summary: View changes
FileSize
97.64 KB
44.18 KB
34.38 KB

One 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).

Status: Needs review » Needs work

The last submitted patch, 39: 2554151-39.patch, failed testing.

stefan.r’s picture

Status: Needs work » Needs review
FileSize
12.76 KB

Status: Needs review » Needs work

The last submitted patch, 42: 2554151-41.patch, failed testing.

stefan.r’s picture

Issue summary: View changes

Updated 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.

stefan.r’s picture

Actually 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).

stefan.r’s picture

Status: Needs work » Needs review
FileSize
14.67 KB

It was an entity reference to the admin user so it was access-restricted

Status: Needs review » Needs work

The last submitted patch, 46: 2554151-46.patch, failed testing.

stefan.r’s picture

Issue summary: View changes
Issue tags: +Needs reroll

#46 will need a reroll

Gábor Hojtsy’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
14.96 KB

Rerolled.

catch’s picture

stefan.r’s picture

@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?

Gábor Hojtsy’s picture

Well its a @todo that predates this patch at least :)

catch’s picture

@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.

Gábor Hojtsy’s picture

Added link to todo. Also hopefully better explanation on the problem not just what to do with it :)

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Reviewed 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.

+++ b/core/modules/system/src/Tests/Update/UpdatePathTestBaseFilledTest.php
@@ -22,4 +25,374 @@ protected function setDatabaseDumpFiles() {
+    // Make sure our ban still exists.
+    $this->drupalGet('admin/config/people/ban');
+    $this->assertText('8.8.8.8');

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.

plach’s picture

Issue tags: +D8 Accelerate

I 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:

  • Added a comment at node/8
  • Translated ‘Site name’ in admin/config/system/site-information/translate/es/edit
  • Translated the ’Content’ view at admin/structure/views/view/content/translate/es/edit
  • Added a test field at admin/config/people/accounts/fields, unless the following assertion concerns this, but that looks a bit fragile to me:
    +++ b/core/modules/system/src/Tests/Update/UpdatePathTestBaseFilledTest.php
    @@ -22,4 +25,374 @@ protected function setDatabaseDumpFiles() {
    +    $this->assertText('drupal');
    

    Can we explicitly check the fields page?

Also the following items were only partially checked (mainly translation was left out):

+ Added a new user at user/3
	- translated at es/user/3
+ Added a block
	- with visibility conditions at admin/structure/block
	- including a translation
+ Added a menu at/admin/structure/menu/manage/test-menu
	- and a translation (link)
+ Added a vocabulary at admin/structure/taxonomy/manage/test_vocabulary/overview
	- translated terms
stefan.r’s picture

@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:

+++ b/core/modules/system/src/Tests/Update/UpdatePathTestBaseFilledTest.php
@@ -22,4 +25,374 @@ protected function setDatabaseDumpFiles() {
+    $this->assertText('Hola');
+    $this->assertText('Hello');
plach’s picture

Oh, yeah, missed that. Quick code review:

  1. +++ b/core/modules/system/src/Tests/Update/UpdatePathTestBaseFilledTest.php
    @@ -22,4 +25,374 @@ protected function setDatabaseDumpFiles() {
    +    $this->drupalGet('es/node/8');
    

    We should pass language as an option, not hard-coding it in the path.

  2. +++ b/core/modules/system/src/Tests/Update/UpdatePathTestBaseFilledTest.php
    @@ -22,4 +25,374 @@ protected function setDatabaseDumpFiles() {
    +    // Make sure the translated slogan appears.
    

    Can we add an empty line to separate this from the other code block?

  3. +++ b/core/modules/system/src/Tests/Update/UpdatePathTestBaseFilledTest.php
    @@ -22,4 +25,374 @@ protected function setDatabaseDumpFiles() {
    +    $this->drupalGet('admin/config/user-interface/shortcut');
    

    If I'm not mistaken this line should be moved after the comment below.

  4. +++ b/core/modules/system/src/Tests/Update/UpdatePathTestBaseFilledTest.php
    @@ -22,4 +25,374 @@ protected function setDatabaseDumpFiles() {
    +    $this->drupalGet('es/node/8/edit');
    

    As above, we should pass language as an option.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
15.17 KB
2.63 KB

Fixing #58 and that the method comment and test method name were outdated. Did not address #56/#57 yet.

stefan.r’s picture

I'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 :)

Translated ‘Site name’ in admin/config/system/site-information/translate/es/edit

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.

Gábor Hojtsy’s picture

Looking 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:

->values(array(
  'collection' => 'language.es',
  'name' => 'system.site',
  'data' => 'a:1:{s:6:"slogan";s:14:"drupal Spanish";}',
))

3. The translated content view is tested already

    $this->drupalGet('admin/structure/views/view/content/translate/es/edit');
    $this->assertText('Contenido');

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).

stefan.r’s picture

Gábor Hojtsy’s picture

@stefan.r: updates in #62 look great!

Gábor Hojtsy’s picture

Failed on Line 184 of core/modules/system/src/Tests/Update/UpdatePathTestBaseFilledTest.php:
Checkbox field edit-visibility-node-type-bundles-test-content-type is checked.
stefan.r’s picture

The last submitted patch, 62: 2554151-62.patch, failed testing.

catch’s picture

stefan.r’s picture

#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:

  • admin/content
  • admin/structure/types/manage/test_content_type/delete
  • admin/structure/types/manage/book/delete
  • admin/structure/types/manage/page/delete
  • admin/structure/types/manage/article/delete
  • block/1/delete
  • block/2/delete
  • aggregator/sources/1/delete
  • taxonomy/term/1/delete
  • taxonomy/term/4/delete
  • admin/structure/taxonomy/manage/test_vocabulary/delete
  • admin/structure/taxonomy/manage/tags/delete
  • admin/structure/taxonomy/manage/tags/delete
  • es/user/3/translations/delete/es
  • admin/config/people/accounts/fields/user.user.user_picture/delete
  • admin/config/people/accounts/fields/user.user.field_test_field/delete
  • admin/config/user-interface/shortcut/manage/test/delete
  • admin/structure/contact/manage/test_contact_form/delete
  • admin/structure/contact/manage/feedback/delete
  • admin/structure/comment/manage/test_comment_type/delete
  • admin/structure/menu/manage/test-menu/delete
  • user/3/cancel
  • admin/config/user-interface/shortcut/link/1/delete
  • admin/structure/comment/manage/comment_forum/fields/comment.comment_forum.comment_body/delete
  • admin/structure/comment/manage/comment/fields/comment.comment.comment_body/delete
  • admin/config/user-interface/shortcut/link/2/delete
  • admin/structure/comment/ (delete everything)
  • - delete filter formats (not possible through UI yet)

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!

catch’s picture

I 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.

plach’s picture

+1 for #69, I was slightly worried about the implications of the uninstallation work too.

plach’s picture

+++ b/core/modules/system/src/Tests/Update/UpdatePathTestBaseFilledTest.php
@@ -22,4 +25,405 @@ protected function setDatabaseDumpFiles() {
+    // @todo attempt uninstalling all modules, make sure there are no errors.

In 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.

plach’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
676 bytes
16.4 KB

The interdiff is ridiculously trivial, so it's should be fine for me to RTBC this. Let's see whether committers agree :)

Gábor Hojtsy’s picture

Issue tags: -Needs tests

Yeah agreed looks good :)

catch’s picture

Status: Reviewed & tested by the community » Fixed

This 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!

  • catch committed be189a8 on 8.0.x
    Issue #2554151 by stefan.r, Gábor Hojtsy, plach, dawehner, csg: Test...

Status: Fixed » Closed (fixed)

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