Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Stevel’s picture

FileSize
591.05 KB

And here's an initial patch. I'm sure it will fail, but sadly that's because of genuine bugs in the upgrade path.

Status: Needs review » Needs work

The last submitted patch, 848368-poll-test.patch, failed testing.

Stevel’s picture

Summary of the currently failing tests:

Message Group Filename Line Function Status
Undefined variable: field Notice taxonomy.install 410 taxonomy_update_7005()
Undefined index: poll Notice node.install 549 node_update_7006()
Undefined index: poll Notice node.install 549 node_update_7006()
Undefined index: poll Notice node.install 549 node_update_7006()
Undefined index: poll Notice node.install 549 node_update_7006()
Undefined index: poll Notice node.install 549 node_update_7006()
Undefined index: poll Notice node.install 549 node_update_7006()
Undefined index: poll Notice node.install 549 node_update_7006()
Undefined index: poll Notice node.install 549 node_update_7006()
Undefined index: poll Notice node.install 549 node_update_7006()
Undefined index: poll Notice node.install 549 node_update_7006()
Attempt to create an instance of field comment_body on bundle comment_node_poll that already has an instance of that field. FieldException field.crud.inc 674 field_create_instance()
HTTP response expected 200, actual 500 Browser upgrade.test 231 UpgradePathTestCase->performUpgrade()
The upgrade was completed successfully. Other poll.test 32 PollUpgradePathTestCase->testPollUpgrade()

The first exception (undefined variable field) is being resolved at #706842: Improve comments for the taxonomy upgrade path. The last two failures are caused by the preceding FieldException.
The field_create_instance function is probably called from node_update_7006 indirectly, so all other exceptions seem related; Looking into that.

Stevel’s picture

Status: Needs work » Needs review
FileSize
600.93 KB

Updated version of the tests. Using drupal_install_modules doesn't rebuild {node_types} causing various warnings and errors noted above. New solution is to enable the modules manually before running the content generation script.

Another strange issue is that the test only works when the poll.module is actually enabled on the site running the test. Otherwise (for a reason unknown to me) the "Results" tab isn't displayed on poll nodes, only "View" and "Edit".

Expecting one failure now ("undefined variable: field").

Status: Needs review » Needs work

The last submitted patch, 848368-poll-test-2.patch, failed testing.

Stevel’s picture

There are more tests failing. Strange thing is, it doesn't happen when poll.module is enabled on the site running the tests.

First debugging yields the following results:
- module_list includes poll.module
- module_implements('menu') doesn't return poll.module, causing the poll menu items not being included in {menu_router}.

Further investigation needs to point out why this happens.

Stevel’s picture

Status: Needs work » Needs review
FileSize
608.24 KB

Lets see what this gives on the testbot...

Major thanks to chx for helping and guiding me to find why these apparently strange things were happening.

Status: Needs review » Needs work

The last submitted patch, 848368-poll-test-3.patch, failed testing.

Stevel’s picture

Status: Needs work » Needs review
FileSize
608.21 KB

Leave module_list(TRUE) in performUpgrade, so the tests know about newly enabled/disabled modules.

Status: Needs review » Needs work

The last submitted patch, 848368-poll-test-4.patch, failed testing.

Stevel’s picture

Status: Needs work » Needs review

#7: 848368-poll-test-3.patch queued for re-testing.

Stevel’s picture

#9: 848368-poll-test-4.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 848368-poll-test-4.patch, failed testing.

Stevel’s picture

FileSize
608.37 KB

Another try, now we explicitly load modules that are enabled in the test database, but not on the testing client.

Stevel’s picture

Status: Needs work » Needs review

Needs review again

Status: Needs review » Needs work

The last submitted patch, 848368-load-new-modules.patch, failed testing.

Damien Tournoud’s picture

Woo, great job. Now let's just merge this one with my patch for taxonomy_update_7005() and call it a go.

Stevel’s picture

Status: Needs work » Needs review

#14: 848368-load-new-modules.patch queued for re-testing.

Not there yet. Testbot complains about an error in locale.test now, but not sure how that can be related...

Damien Tournoud’s picture

FileSize
609.41 KB

Actually, for the sake of isolating test runs, I suggest we do something like that.

Stevel’s picture

Status: Needs review » Reviewed & tested by the community

RTBC based on #17 and my review of Damien's modifications

Stevel’s picture

Title: Test updating for poll.module » [beta blocker blocker] Test updating for poll.module
Priority: Normal » Critical
Issue tags: +D7 upgrade path

This patch makes some changes to the upgrade testing framework as well as add a more generic testing database, both of which are needed for #706842: Improve comments for the taxonomy upgrade path, so marking it accordingly.

int’s picture

Issue tags: +beta blocker
philbar’s picture

This module does not appear on the D7 beta blocker section of the community initiative page. Should it be?

Stevel’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
615.25 KB

Made some corrections to the generation script, which now also enables needed modules and runs cron after installing modules (but before adding the content).
Modifies the dump script to include a header containing the loaded modules as well.

chx’s picture

Status: Needs review » Reviewed & tested by the community

Very nice work. Given that the generation script was patch ping-pong between Stevel and me of course I like it :p but then this is an absolute first someone wrote a poll generation script, even devel generate does not do that. Stevel is awesome.

Dries’s picture

Very cool. It does increase the size of Drupal by 0.6MB though -- are we cool with doing that?

chx’s picture

Yes we have discussed this -- we can change the packaging script to make the default tarball exclude this directory completely and have another with this. Also in a followup patch we can experiment with gzipping it down to almost nothign but for now we want to see diffs.

webchick’s picture

For reference, the place this was discussed was #706842-22: Improve comments for the taxonomy upgrade path and below, a sister issue to this one.

chx’s picture

So what happened to this issue this week?

chx’s picture

Anyone around?

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Nothing happened with this patch last week (that is OK) but I committed it just now after another review. Thanks to Stevel and Damien Tournoud for driving this home.

chx’s picture

Issues like this cause me to slip down the contributor top list. :( oh well.

webchick’s picture

Sorry; this patch has been on my "to review and commit list" since July 16, but real life has not been so kind to my patch committing schedule. Thanks, Dries, for picking it up!

Mmmm. I love the smell of fresh beta, and this patch gets us a huge chunk of the way there.

Status: Fixed » Closed (fixed)
Issue tags: -D7 upgrade path, -beta blocker

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