UpdateTestContribCase fails on sites with contrib modules/themes in the filesystem

Reproduced and found why this fails. Just look at _update_process_fetch_task() (http://api.drupal.org/api/drupal/modules--update--update.fetch.inc/funct...), it contains an internal failure counter. If the data we got from the update server was invalid, it updates the failure counter. Once it is above UPDATE_MAX_FETCH_ATTEMPTS, it will not contact the update server again for 5 minutes.

Now the update server that is implemented for the tests mocks the modules it must know about (and drupal core), but for the rest, it just returns empty. From update_test_mock_page():

  else {
    // The test didn't specify (for example, the webroot has other modules and
    // themes installed but they're disabled by the version of the site
    // running the test. So, we default to a file we know won't exist, so at
    // least we'll get an empty page from readfile instead of a bunch of
    // Drupal page output.
    $availability_scenario = '#broken#';
  }

  $path = drupal_get_path('module', 'update_test');
  readfile("$path/$project_name.$availability_scenario.xml");

And of course that empty return is counted as a failure. So if you have a couple modules with version information (that is when they are used to contact the update server), the test is going to break, because it will not reach the mock themes that is looking for. It will consider the server is broken and will not load update info from there after the first few modules.

Here is an ugly and very quick stopgap patch. A good approach to fix this is to actually return some meaningful data for non-existent projects in update_test.module, with minimal data required. Until I figure the minimal data required there, this is a quick stopgap.

Yeah, this is a pretty interesting bug. The test specifically turns on the setting that forces Update module to check the status of disabled modules/themes (by which it really means "disabled or not installed" in the case of modules - and after reading #162788: Include modules that aren't enabled that seems to be intended). So anything sitting around your filesystem is suddenly fair game, by design - even though the testing instance never installed it.

Given that this test is such a special case, I'm not sure the "stopgap" patch is so terrible, actually (though it would be better to set the number based on the total number of modules/themes it finds in the filesystem, rather than 99999999). Though modifying the fallback behavior of update_test_mock_page() would, I suppose, still be better.

Patch really looks not that bad to me, it will be way down the list of the worst hacks added to a test. Marking CNR.

Reuploading #2, but in -p1 format.

If we're linking to this issue and expecting to finish it properly later, we should add @todo and/or @see. Either that or remove the reference and document a bit more why it's using a silly number. I'm not all that sure this is worth trying to fix properly, would likely require a module directory scan, which is only going to slow down tests even if a little bit.

I'm not entirely sure, but it's possible that this is not limited to custom modules.

My patch in #875342-18: Guzzle should pick up X-Drupal-Assertion-* HTTP headers reveals similar test errors, but there is no custom module in the testbot checkout.

#10 is obsolete - I studied update_test.module's code and fixed the unintended errors in the other issue.

Changed the comment to a @todo, removed the reference to this issue since git blame covers that.

+++ b/includes/database/mysql/schema.incundefined
@@ -131,8 +131,13 @@ class DatabaseSchema_mysql extends DatabaseSchema {
-    if (in_array($spec['mysql_type'], array('VARCHAR', 'CHAR', 'TINYTEXT', 'MEDIUMTEXT', 'LONGTEXT', 'TEXT')) && isset($spec['length'])) {
-      $sql .= '(' . $spec['length'] . ')';
+    if (in_array($spec['mysql_type'], array('VARCHAR', 'CHAR', 'TINYTEXT', 'MEDIUMTEXT', 'LONGTEXT', 'TEXT'))) {
+      if (isset($spec['length'])) {
+        $sql .= '(' . $spec['length'] . ')';
+      }
+      if (!empty($spec['binary'])) {
+        $sql .= ' BINARY';
+      }

Did you mean to include this hunk as well? I'd think not.

Powered by Dreditor.

Tagging

whoops.

Another whoops in the comment via chx in irc.

+++ b/modules/update/update.test
@@ -441,6 +441,11 @@ class UpdateTestContribCase extends UpdateTestHelper {
+    // @todo: when there are contributed modules in the site's file system, the
+    // total number off attempts made in the test may exceed the default value
+    // of update_mx_fetch_attempts. Therefore this variable is set ridiculously
+    // high to avoid test failures in those cases.

1) I'd drop the @todo. (Also FYI, no colon after @todo, and subsequent @todo lines should be indented)

2) Typo in "off".

3) Typo in "update_mx_fetch..."

4) Let's also replace "ridiculously" with "very".

19 days to next Drupal core point release.

Guess that works as a stop-gap fix.

Updated issue summary.

Summary added.

This patch is pretty ick, but in talking to catch about it on irc, the alternative is basically to count the number of modules and set that to the timeout value, which is the same thing, only longer. :P~

So, committed and pushed to 8.x and 7.x. Thanks.