Update test database dump should be based on beta 12 and contain content [#2551341]

Problem/Motivation

Opening this as critical because:
- it's blocking other criticals
- every critical issue that deals with it has commit conflicts with each other
- false positives in the upgrade path tests.

The current database dump for updates was taken from pre-beta 12, and now has some modifications to make it suitable for tests that have been added.

We have false-passes in update tests because the database dump has no nodes/taxonomy terms etc.

Additionally issues like #2540416: Move update.php back to a front controller are getting blocked on inconsistent data in the database dump.

Proposed resolution

Since we know that the update path from beta 12 either works, or doesn't work in a way that is documented in existing critical issues, we should target that for the database dump, and ensure there's some data to actually get upgraded - at least nodes, users, taxonomy terms, comments, forums, contact forms, custom block, blocks with visibility conditions.

Remaining tasks

Review patch.

Steps taken for bare dump:
- Installed beta12 with standard profile and site name "Site-Install"
- Created a dump with core/scripts/dump-database-d8-mysql.php | gzip > core/modules/system/tests/fixtures/update/drupal-8.bare.standard.php.gz

Steps taken for filled dump:
- Installed beta12 with standard profile
- Enabled all modules
- Enabled the Stark theme
- Added a language (Spanish)
- Added a test date format
- Checked all boxes in admin/config/regional/language/detection
- 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
- Translated an interface string ("Full comment") at admin/config/regional/translate
- Added a test image style at admin/config/media/image-styles/manage/test_image_style
- Added a test responsive image style at admin/config/media/responsive-image-style/test
- Added a shortcut at admin/config/user-interface/shortcut
- Added a test action at admin/config/system/actions
- Added a test feed at aggregator/sources/1/configure
- Added a field at /admin/config/services/aggregator/fields
- Added a test text format at admin/config/content/formats/manage/test_text_format
- Added a banned IP at admin/config/people/ban
- Added a comment type at admin/structure/comment
- Added a form mode at admin/structure/display-modes/form
- Added a view mode at admin/structure/display-modes/view
- Enabled translation for users at admin/config/people/accounts
- Added a new user at user/3, translated at es/user/3
- Added a test field at admin/config/people/accounts/fields
- Added a new translatable content type at admin/structure/types/manage/test_content_type
- Added new fields at admin/structure/types/manage/test_content_type/fields
- Added a vocabulary at admin/structure/taxonomy/manage/test_vocabulary/overview, including terms and translated terms.
- Added a menu at/admin/structure/menu/manage/test-menu, including a menu link and a translation
- Added a view at admin/structure/views/view/test_view
- Added a forum at admin/structure/forum
- Create a new article, with several revisions and a menu link
- Added a comment at node/8
- Added a new forum topic at node/3
- Added a book page at node/2
- Added a new node for the custom test content type at node/8 including an image and a file
- Added a translation at es/node/8
- Added a contact form at /admin/structure/contact, including a translation
- Added a block with visibility conditions at admin/structure/block, including a translation
- Set site name to "Site-Install"
- Created a dump with core/scripts/dump-database-d8-mysql.php | gzip > core/modules/system/tests/fixtures/update/drupal-8.filled.standard.php.gz

User interface changes

N/A

API changes

N/A

Data model changes

N/A

Comment	File	Size	Author
#23	2551341-23.patch	812.95 KB	stefan.r
#21	drupal-8.bare_.standard.php_.gz	110.15 KB	stefan.r
#21	drupal-8.filled.standard.php_.gz	549.79 KB	stefan.r
#19	2551341-18.patch	779.48 KB	stefan.r
#15	2551341-15.patch	662.86 KB	stefan.r
#15	changes.txt	2.19 KB	stefan.r

Comments

Comment #1

14 August 2015 at 09:54

catch created an issue. See original summary.

Comment #2

catch

he/him

English

commented 14 August 2015 at 10:04

Issue summary:

View changes

Comment #3

alexpott

he/they

English

🇪🇺🌍

commented 14 August 2015 at 10:05

Issue summary:

View changes

Comment #4

catch

he/him

English

commented 14 August 2015 at 10:13

Issue summary:

View changes

Comment #5

webchick

she/they

English

Vancouver 🇨🇦

commented 14 August 2015 at 14:28

Issue tags:

+Actionable D8 critical

Nice. This is something that lots of people could do.

Comment #6

cilefen commented 14 August 2015 at 15:46

I just spent a few minutes searching but I cannot find where this dump lives in the repo.

Comment #7

jibran

he/him

Urdu

Toronto, Canada

commented 14 August 2015 at 15:50

It is at core/modules/system/tests/fixtures/update/drupal-8.bare.standard.php.gz

Comment #8

mpdonadio

he/him

English

Philadelphia/PA/USA (UTC-5)

commented 14 August 2015 at 15:52

It's core/modules/system/tests/fixtures/update/drupal-8.bare.standard.php.gz

@see \Drupal\system\Tests\Update\UpdatePathTestBase() and the tests that extend it.

Comment #9

jhedstrom

English

Portland, OR

commented 14 August 2015 at 16:00

Issue summary:

View changes

Adding details on the dump script to the IS. We might want to rename the bare to filled once there is content.

Comment #10

jibran

he/him

Urdu

Toronto, Canada

commented 14 August 2015 at 16:08

As per our discussion in critical issues meeting we are keeping the bare as well.

Comment #11

cilefen commented 14 August 2015 at 18:45

Issue summary:

View changes

Comment #12

dawehner

German

commented 15 August 2015 at 15:05

So I think we need two steps:

a) Update the bare one to beta 12
b) Create a new one based upon beta 12 with content.

Comment #13

jhedstrom

English

Portland, OR

commented 15 August 2015 at 19:25

The content based dump could be put on top of the bare dump if #2544972: Add options to limit tables, exclude data, or only export data to DbDumpCommand were used (people generating the beta12 dumps would need to manually apply that patch).

Comment #14

jhedstrom

English

Portland, OR

commented 15 August 2015 at 19:29

Issue summary:

View changes

Comment #15

stefan.r commented 16 August 2015 at 13:46

Status:

Active

» Needs review

Status	File	Size
new	changes.txt	2.19 KB
new	2551341-15.patch	662.86 KB

Here's a beta12 dump with some content

Comment #16

stefan.r commented 16 August 2015 at 14:01

Will post a patch with both bare and filled in a bit...

Comment #17

16 August 2015 at 14:09

Status:

Needs review

» Needs work

The last submitted patch, 15: 2551341-15.patch, failed testing.

Comment #18

dawehner

German

commented 16 August 2015 at 15:39

Here's a beta12 dump with some content

Do you mind describing in the IS what is part of the dump? Did you enabled all the modules, did you configured a multilingual site, did you configured some real views etc.

Comment #19

stefan.r commented 16 August 2015 at 15:47

Status:

Needs work

» Needs review

Status	File	Size
new	2551341-18.patch	779.48 KB

Just for the record, the test failures said Parse error: syntax error, unexpected '' (T_ENCAPSED_AND_WHITESPACE), expecting identifier (T_STRING) or variable (T_VARIABLE) or number (T_NUM_STRING) in core/modules/system/tests/fixtures/update/drupal-8.bare.standard.php.gz

Maybe I forgot to do --binary when doing a git diff?

Here's another try with both bare and filled, with current tests updated to use both dumps.

Comment #20

stefan.r commented 16 August 2015 at 15:51

Issue summary:

View changes

Comment #21

stefan.r commented 16 August 2015 at 16:01

Status	File	Size
new	drupal-8.filled.standard.php_.gz	549.79 KB
new	drupal-8.bare_.standard.php_.gz	110.15 KB

And the actual raw dumps, in case there is a problem with the patch again

Comment #22

16 August 2015 at 16:14

Status:

Needs review

» Needs work

The last submitted patch, 19: 2551341-18.patch, failed testing.

Comment #23

stefan.r commented 16 August 2015 at 22:31

Status:

Needs work

» Needs review

Status	File	Size
new	2551341-23.patch	812.95 KB

Updated the dumps to have the site name "Site-Install" as expected by the tests.

Comment #24

stefan.r commented 16 August 2015 at 22:33

Issue summary:

View changes

Comment #25

dawehner

German

commented 17 August 2015 at 08:02

This looks like a quite decent list. Should we have some test code which ensures that these things continue to sort of work?

Comment #26

stefan.r commented 17 August 2015 at 08:16

Yes that could be nice, would it be in scope for this issue?

Comment #27

dawehner

German

commented 17 August 2015 at 08:22

Status:

Needs review

» Reviewed & tested by the community

Well, in order to know that our update path actually works we need them, so this would feel critical as well. But sure, we could do that in another issue.

Comment #28

alexpott

he/they

English

🇪🇺🌍

commented 18 August 2015 at 10:20

Status:	Reviewed & tested by the community	» Needs work
Issue tags:		+Needs tests

I think it is in scope to have some tests of the content/config in UpdatePathTestBaseTest in the issue.

Comment #29

gábor hojtsy

he/him

Hungarian

Hungary

commented 19 August 2015 at 19:55

Tests as in? That when you import the dump, the entity storage / config storage has them?

Comment #30

catch

he/him

English

commented 19 August 2015 at 19:58

I think it'd be functional tests after the upgrade has run that the entities are accessible and have the right content etc.

For me personally, I think we could open another issue (major or critical) to add the explicit test coverage, but the implicit coverage this adds might mean other patches start failing where they'd otherwise pass, which would be worthwhile in itself.

Comment #31

gábor hojtsy

he/him

Hungarian

Hungary

commented 19 August 2015 at 20:41

Status:	Needs work	» Reviewed & tested by the community
Issue tags:	-Needs tests

Opened #2554151: Test content/configuration in update database dump for more explicit tests as a major. I agree this in itself will start discovering errors :)

Comment #32

dawehner

German

commented 19 August 2015 at 20:42

Well I see the point, the tests can be itself a major issue for itself.

Comment #33

plach

he/him

Italian

Venezia

commented 19 August 2015 at 22:36

It would be nice to see this in ASAP, so we can have more maeningful test coverage in #2542748: Automatic entity updates can fail when there is existing content, leaving the site's schema in an unpredictable state.

Comment #34

catch

he/him

English

commented 20 August 2015 at 07:19

Assigned:

Unassigned

» alexpott

Assigning to Alex to see what he thinks per #28.

Comment #35

alexpott

he/they

English

🇪🇺🌍

commented 20 August 2015 at 08:26

Status:

Reviewed & tested by the community

» Fixed

Let's get this in since it help other issues. But I think the testing follow up should be critical because we've not proven anything without the tests. Committed b728936 and pushed to 8.0.x. Thanks!

Comment #36

20 August 2015 at 08:26

alexpott committed b728936 on 8.0.x

Issue #2551341 by stefan.r: Update test database dump should be based on...

Comment #37

catch

he/him

English

commented 20 August 2015 at 09:56

Crossposted...

Put a note on #2554151: Test content/configuration in update database dump but to document @alexpott's concern from irc here. If we add the dump with no test coverage, then there's nothing guaranteeing that it actually catches the cases it should. For me #2554151: Test content/configuration in update database dump moving to critical would be plenty for that, but #33 shows why this going in as is could be a useful interim state.

Comment #38

gábor hojtsy

he/him

Hungarian

Hungary

commented 21 August 2015 at 01:06

So.... turns out none of the tests we committed here actually test what they think they test because we use this pattern:

  protected function setUp() {
    $this->databaseDumpFiles[0] = __DIR__ . '/../../../tests/fixtures/update/drupal-8.filled.standard.php.gz';
    parent::setUp();
  }

But then, the parents reset the file list entirely:

  protected function setUp() {
    $this->databaseDumpFiles = [__DIR__ . '/../../../tests/fixtures/update/drupal-8.bare.standard.php.gz'];
    parent::setUp();
  }

So basically the filled dump is never tested. Fun stuff! All the tests we added here need fixing. Let's get that into #2554151: Test content/configuration in update database dump. BTW @csg found this because he used a different pattern to set the dumpfile and it found a PHP syntax error in the dump :)

Comment #39

gábor hojtsy

he/him

Hungarian

Hungary

commented 21 August 2015 at 01:07

BTW also ENTIRELY validates those tests being critical, haha.

Comment #40

gábor hojtsy

he/him

Hungarian

Hungary

commented 21 August 2015 at 01:07

BTW also ENTIRELY validates those tests being critical, haha.

Comment #41

4 September 2015 at 01:14

Status:

Fixed

» Closed (fixed)

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

Update test database dump should be based on beta 12 and contain content

Problem/Motivation

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Comments

Child issues