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:

<?php
    $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:

<?php
   
// 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

Files: 
CommentFileSizeAuthor
#22 d7-tax-test-from-comment-11.patch3.75 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 37,387 pass(es).
[ View ]
#16 drupal8.taxonomy-term-test-random-failure.16.patch2.57 KBsun
PASSED: [[SimpleTest]]: [MySQL] 34,241 pass(es).
[ View ]
#12 drupal8.taxonomy-term-test-random-failure.12.patch1.12 KBsun
PASSED: [[SimpleTest]]: [MySQL] 34,231 pass(es).
[ View ]
#11 even-less-magic.patch3.77 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 34,243 pass(es).
[ View ]
#7 no-magic-term-order.patch698 bytesxjm
FAILED: [[SimpleTest]]: [MySQL] 34,219 pass(es), 8 fail(s), and 18 exception(es).
[ View ]
drupal8.taxonomy-term-test-random-failure.0.patch3.34 KBsun
PASSED: [[SimpleTest]]: [MySQL] 34,197 pass(es).
[ View ]

Comments

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.

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

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

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.

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.

Status:Needs work» Needs review
StatusFileSize
new698 bytes
FAILED: [[SimpleTest]]: [MySQL] 34,219 pass(es), 8 fail(s), and 18 exception(es).
[ View ]

How about something like this?

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.

Right, need a reset() on there.

Status:Needs work» Needs review
StatusFileSize
new3.77 KB
PASSED: [[SimpleTest]]: [MySQL] 34,243 pass(es).
[ View ]

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

StatusFileSize
new1.12 KB
PASSED: [[SimpleTest]]: [MySQL] 34,231 pass(es).
[ View ]

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

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

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

Status:Needs review» Reviewed & tested by the community

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);

StatusFileSize
new2.57 KB
PASSED: [[SimpleTest]]: [MySQL] 34,241 pass(es).
[ View ]

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

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

Oh I completely missed the $$var.

Can't decide which is worse now ;)

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

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

$$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!

Status:Needs review» Reviewed & tested by the community

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

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.

Status:Patch (to be ported)» Needs review
StatusFileSize
new3.75 KB
PASSED: [[SimpleTest]]: [MySQL] 37,387 pass(es).
[ View ]

Dette er rerollen.

Status:Needs review» Reviewed & tested by the community

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

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.

Issue tags:+Random test failure

Issue summary:View changes

Updated issue summary.