Problem/Motivation
The new test there can fail randomly.
We create two random strings like this:
for ($i = 0; $i < 2; $i++) {
$string = $this->randomMachineName();
$fixtures[] = array(
'original' => $string,
'uppercase' => Unicode::strtoupper($string),
'lowercase' => Unicode::strtolower($string),
);
}
And then set the field value to $fixtures[0]['uppercase'] . $fixtures[1]['lowercase']
.
And in the last assertion, we do this:
$result = \Drupal::entityQuery('entity_test_mulrev')->condition(
'field_cs', Unicode::strtolower(Unicode::substr($fixtures[0]['uppercase'] . $fixtures[1]['lowercase'], 4, 8)), 'CONTAINS'
)->execute();
$this->assertIdentical(count($result), 0, 'Case sensitive, exact match.');
If the last 4 characters of the first string and the first 4 of the second are the same as the second string, then CONTAINS will match on only the second string, we however expect no matches.
I was wrong (this could happen, but it never did for me in 40 million tries), there is a much more likely random fail: If the last 4 characters of the first string are all numbers, then upper/lowercase doesn't change the string and it matches anyway.
Also, the assertion message of exact match and lowercase are wrong, should be other way round.
Proposed resolution
Append 'a' to the random string, so that we are sure that it never ends with 4 numbers.
Remaining tasks
User interface changes
API changes
Suggested commit message
Issue #2397297 by Berdir, alexpott: EntityQueryTest::testCaseSensitivity() can fail randomly
Comment | File | Size | Author |
---|---|---|---|
#5 | 2397297.5.patch | 914 bytes | Berdir |
#2 | 2397297.2.patch | 807 bytes | alexpott |
Comments
Comment #1
BerdirComment #2
alexpottHere's a possible fix that I've run 100 times without fail (though that is not enough to really prove the fix).
The patch has not fixed:
Comment #3
BerdirAh, I thought we used $i explicitly for $fixtures, we don't, that's what I meant.
Yes, the fix makes sense to me.
The only way this can happen is if the last 12 of the total 16 characters are repeated in 3 groups of 4 characters (I have no idea this could ever happen, whoever had that patch tested should play lottery), so the comment could be a bit more clear.
If we can clarify this a bit then I think this is RTBC.
Comment #4
BerdirThis just failed again in #2397691: Random fail in Drupal\taxonomy\Tests\TermTest::testNodeTermCreationAndDeletion().
Which means that there is either something else or this is more likely to happen than we thought.
Comment #5
BerdirPlaying with this, made a script to test my theory and executed it 4 times in parallel, 4x 10'000'000 tries. No match.
I can not believe that we had two fails here already and I had none in 40 million tries.
So I modified it a bit to do a strpos() instead of explicitly looking for a match on the second try, which is the same as the test is doing. And voila!
(I changed it to only run 1 million times, more than enough)
The pattern is easy enough to spot: numbers!
If the last 4 characters of the first string are all numbers, then strtoupper() doesn't change a thing and it matches.
So the correct fix is to append a character, then this can not happen. Note that it is not possible that the first or all characters are numbers, as the first character is guaranteed to be non-numeric.
The script:
Comment #6
BerdirComment #7
BerdirComment #8
swentel CreditAttribution: swentel commentedNice detective work! Solution looks sane to me.
- edit - maybe we should use Random::word() one day? But I don't want to spoil the RTBC for that.
Comment #9
webchickQuestion. Should we not make this change to randomMachineName() instead? It seems like this same condition could occur anywhere that this method is used.
Comment #10
BerdirI don't think that would make sense. It already protects against getting a number as first character, because that would be a problem for many use cases.
This scenario isn't a problem in any of our existing test (and unlikely to be in all upcoming ones), only in this very specific and rather special test. There's nothing wrong with a field name like something1234, for example.
Comment #11
alexpottCommitted 640f528 and pushed to 8.0.x. Thanks!