Random test failures in SearchCommentTest

Well if we want to see what's going on then not using variable method calls would be good while at it.

Committed to 8.x.

Moving to 7.x.

Doesn't sound like that patch was intended to be the final word here, was it? Just a cleanup patch to improve the assertion messages and help see what was going on...

Also not sure these random test failures exist in Drupal 7 at all (although they could).

We just had this issue on our first core patch and was a confusing experience, not saying it is a major issue but should definitely be fixed ASAP
the issue http://drupal.org/node/1662956 failed testing first time, second time passed.

I believe this issue just came up on #1145080: SelectQuery::countQuery() incompatible with UNION queries. I took a screenshot of the fail messages from the test bot before retesting incase they would be helpful.

I tried to look into this, but I have no clue how to figure out what's going on here. Can't see anything wrong either in the tests nor in the code, nothing that could cause this. It looks like the comment is not indexed, despite he should. Something weird going on with roles maybe?

Note that the line numbers of the screenshot don't match anymore.

Got it!

The offending line of code is in comment_node_update_index()... specifically the assumption here that rid is numeric and anonymous will be 0 and authenticated 1

    // Prevent indexing of comments if there are any roles that can search but
    // not view comments.
    $index_comments = TRUE;
    foreach ($perms['search content'] as $rid) {
      if (!isset($perms['access comments'][$rid]) && ($rid <= DRUPAL_AUTHENTICATED_RID || !isset($perms['access comments'][DRUPAL_AUTHENTICATED_RID]))) {
        $index_comments = FALSE;
        break;
      }
    }

Obviously with the new machine name roles and the fact this test creates a role with a random rid is responsible for this test's random behaviour!

Very very awesome. So if it created a role name which started with aa or something then it failed, not many strings are below 'authenticated' so that's why it was so rare.

I'm happy to commit this to stop the madness (on my way down to the sprint shortly), but doesn't this also mean we need test coverage for comment_node_update_index()? There's no reason we should have been getting random test failures; the code there is simply wrong, no?

This is the test for comment_node_update_index :D

Oh, ok. :)

Committed and pushed to 8.x. Rock!