Revealed via #1373142: Use the Testing profile. Speed up testbot by 50%

TaxonomyTermTestCase::testNodeTermCreationAndDeletion() produces random test failures.

The test asserts the JSON response of the autocomplete page callback.

It creates 3 terms:

    $terms = array(
      $this->randomName(),
      $this->randomName() . ', ' . $this->randomName(),
      $this->randomName(),
    );
    $edit[$instance['field_name'] . "[$langcode]"] = drupal_implode_tags($terms);
    $this->drupalPost('node/add/article', $edit, t('Preview'));

The terms are then loaded via:

    // Get the created terms.
    list($term1, $term2, $term3) = array_values(taxonomy_term_load_multiple(FALSE));

My suspicion is that the random term names are re-ordered/sorted alphabetically somewhere in the autocomplete functionality.

Attached patch does not fix the bug, merely verbosely exposes the actual values for debugging.

Related issues

#1000736: Term reference autocomplete widget having problems handling terms with commas

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

sun’s picture

Status: Needs review » Needs work

So @beejeebus seems to have found a workaround/solution in #1373142-36: Use the Testing profile. Speed up testbot by 50%, essentially:

+++ b/core/modules/taxonomy/taxonomy.test
@@ -601,17 +614,20 @@ class TaxonomyTermTestCase extends TaxonomyWebTestCase {
     // Get the created terms.
-    list($term1, $term2, $term3) = array_values(taxonomy_term_load_multiple(FALSE));
+    $terms = taxonomy_term_load_multiple(FALSE);
+    $term1 = $terms[1];
+    $term2 = $terms[2];
+    $term3 = $terms[3];

While that resolves the random test failure, I'm still not sure why the terms are not loaded in the order they are saved.

xjm’s picture

There's this issue that went in recently: #479368: D7: Create RFC compliant HTML safe JSON

sun’s picture

The JSON encoding is correct though. It's really just the order of loaded terms that were previously saved via tagging/autocomplete.

xjm’s picture

Yeah, was just clarifying since catch referenced it in the other issue. I was mistaken about the other autocomplete patches going in; I forgot that one got reverted. Nonetheless, there's these open issues about the autocomplete:

And these about term ordering:

I bet something in that second list is related to the issue here. Haven't looked closely yet.

xjm’s picture

So I haven't been able to reproduce the random failures from this issue using today's 7.x-dev build and the standard profile. I'm not sure why the bug would not show up then... however, maybe we should just be loading the terms by name anyway, since we have the term names? The test does not seem particularly robust as it is.

xjm’s picture

Status: Needs work » Needs review
FileSize
698 bytes

How about something like this?

sun’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, that looks most robust to me.

After all, it's not the purpose nor in the scope of that test case to verify the order of saved/loaded terms, so this fix is appropriate.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, no-magic-term-order.patch, failed testing.

xjm’s picture

Right, need a reset() on there.

xjm’s picture

Status: Needs work » Needs review
FileSize
3.77 KB

I think this is a bit cleaner. I also actually bothered to run it locally this time. ;)

sun’s picture

I guess I can beat that with still less, but more smart magic. :)

Anonymous’s picture

magic battle - sun vs xjm - fight! ;-)

i guess i prefer #12. simple patch, so i'll RTBC if it comes back green.

Anonymous’s picture

Status: Needs review » Reviewed & tested by the community
catch’s picture

Priority: Normal » Major
Status: Reviewed & tested by the community » Needs review

Hmm, I think I prefer #11, 'cos we only have four calls to extract() in all of core, and it'd be a shame to add another one even in a test. Back to CNR for a bit.

core/includes/theme.inc:  extract($variables, EXTR_SKIP);               // Extract the variables to a local namespace
core/modules/dashboard/dashboard.module:  extract($variables);
core/modules/dashboard/dashboard.module:  extract($variables);
core/modules/dashboard/dashboard.module:  extract($variables);
core/modules/dashboard/dashboard.module:  extract($variables);
sun’s picture

Alright, I hope you won't count the number of $$var throughout core :P ;)

If you will, then let's go with #11 :)

catch’s picture

Oh I completely missed the $$var.

Can't decide which is worse now ;)

sun’s picture

To clarify: There are three approaches to choose from now ;)

#11 (objects in array), #12 (extract()), and #16 ($$var)

xjm’s picture

$$var--, ewww.

I've used extract() in my own code I am fine with it personally. #11 is nice in that it is self-documenting, but it's also more typing. ;) So either #11 or #12 is fine by me. Leaning toward #12.

Bikeshed? Us? Never!

sun’s picture

Status: Needs review » Reviewed & tested by the community

@catch or @Dries, can you just simply pick one? Let's move on. :)

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Yep, went for #11. Committed/pushed to 8.x, will need a re-roll for 7.x.

xjm’s picture

Status: Patch (to be ported) » Needs review
FileSize
3.75 KB

Dette er rerollen.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Testbot queue hiccup. Re-queued #22 manually. Guess it'll come back green, so RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Yay for fewer random test failures! The code gets a bit more obfuscated now, but the trade-off seems worth it.

Committed and pushed to 7.x. Thanks!

Status: Fixed » Closed (fixed)

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

cweagans’s picture

Issue tags: +Needs backport to D7
sun’s picture

Issue tags: +Random test failure
sun’s picture

Issue summary: View changes

Updated issue summary.