Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
We rely on chance to ensure that drupalCreateRole and drupalCreateUser do not fail if called multiple times in the same test. This is a bad idea.
Comment | File | Size | Author |
---|---|---|---|
#41 | 38-41-interdiff.txt | 1.64 KB | alexpott |
#41 | 2020209.randomuniques.41.patch | 13.69 KB | alexpott |
#38 | 37-38-interdiff.txt | 2.61 KB | alexpott |
#38 | 2020209.randomuniques.38.patch | 13.59 KB | alexpott |
#37 | 36-37-interdiff.txt | 766 bytes | alexpott |
Comments
Comment #1
alexpottAnd here's a patch...
Comment #2
alexpottBlah my .htaccess file crept in :(
Comment #3
Berdir:)
Same description for both?
Missing type
Comment #4
alexpottThanks!
Comment #5
YesCT CreditAttribution: YesCT commentedshould this be
isset($strings... ?
copy and paste typo?
Comment #6
alexpottThanks @YesCT and @chx pointed out no need for these functions to be static too...
Comment #7
YesCT CreditAttribution: YesCT commentedsome more copy and paste fix.
also,
https://drupal.org/node/1354#classes
Comment #9
alexpott#7: 2020209.randomuniques.7.patch queued for re-testing.
Comment #10
alexpottYet another c&p error from me :( spotted by @YesCT
Comment #11
Gábor HojtsyLooks great. I think it would have been equally fine to have this feature in the existing methods (as a base feature), but additional methods sound equally fine. This highlights the specifics of the behaviour definitely.
Comment #12
alexpottI agree with #11...
Comment #13
alexpott... as a result now is the time to make randomName and randomString non static...
Comment #14
Gábor HojtsyThe *Unique methods don't exist anymore.
Comment #15
alexpottDoh!
Comment #16
Gábor HojtsyLooks good, thanks for putting it into the original implementation I think it will not hurt to have this unique at all times. I don't think people expect randomname to return sometimes the same values as before randomly :)
Comment #17
effulgentsia CreditAttribution: effulgentsia commentedThis looks great to me as well. RTBC+1. @alexpott: since that's now two people who've reviewed it, I think it's okay for you to commit it, despite it being your patch.
Comment #18
alexpottCommitted 880d568 and pushed to 8.x. Thanks!
Comment #19
tstoecklerCan someone explain how the $this->randomStrings and $this->randomNames are being set? It's not obvious to me from the patch.
Comment #20
Gábor HojtsyWups, looks like it does not remember the old strings in fact.... They will always be an empty array....
Comment #21
Gábor HojtsyComment #22
Gábor HojtsyLooks better now? :)
Comment #23
Berdir\Drupal\Tests\UnitTestCase has it's own implementation of randomName() that we should fix too.
Comment #24
alexpottSo I've refactored randomName, randomString and randomObject into a new random component and added tests for the uniqueness of random names and strings. Additionally I have added a fail safe to ensure we don't end up in an endless loop.
I have ran the new phpUnit test over 1000 times locally to ensure this test does not introduce new random fails.
Comment #25
xjmLooks great other than some stupid docs nitpicks.
Defines a.... https://drupal.org/node/1354#classes
Let's keep these lines in
TestBase
.generation.
s/less/fewer/ :)
Comment #26
alexpottThanks @xjm
Comment #27
BerdirThis doesn't need to be inside the loop, it's static.
Comment #28
alexpottGood point...
Comment #29
alexpottOk missed the assertion messages :)
Comment #31
Berdir#29: 2020209.randomuniques.29.patch queued for re-testing.
Comment #32
BerdirWe don't actually document anywhere that it *does* loop and does ensure unique random strings per request.
I think we should a) do that and b) provide a possibility to disable the uniqueness check, especially because there's no way to reset it and we explicitly want to make this available to non-test code.
Maybe that should even be the default and only the test methods that call out to this should enforce uniqueness?
$max also doesn't change :)
Comment #33
alexpottThanks for the review @berdir...
Comment #34
alexpottSo lets prove we have the problem...
Comment #35
dawehneryou will never know that maybe something changes stuff here and it potentially break. Let's ensure the size of the object in a test.
Comment #36
alexpott@dawehner here is a test for you...
I test 0 and 1 because different amounts of code will fire in Random::object depending on this value. So we have complete execution path coverage.
Based on the result of #34 marking this as major as random testbot failures greatly impact our velocity.
Comment #37
alexpottComment #38
alexpottImproving docs...
Comment #39
ParisLiakos CreditAttribution: ParisLiakos commentednitpick alert:
need visibility
Comment #40
Berdirnitpick alert^2.
Should start with (optional), maybe something like "If uniqueness of returned strings should be enforced, defaults to TRUE" ?
Comment #41
alexpottThanks for the reviews :)
Comment #42
ParisLiakos CreditAttribution: ParisLiakos commentedthanks!
Comment #43
catchThat looks better. Committed/pushed to 8.x, thanks!
Comment #44
BerdirMore random fun, here is the random problem that @alexpott and I discovered in Dublin: #2032565: Test failures due to special character (combinations) in random strings.