Follow-up to #2105583: Add some sane strictness to phpunit tests to catch risky tests
Problem/Motivation
Not sure how this isn't already causing problems for global state change detection but its probably a quirk of how phpunit runs. The composer class loader is set to a global in the phpunit bootstrap.php. Somehow all relevant classes seem to be loaded or no changes happen for what ever reason during the core phpunit test runs. However in [#2105593] klausi identified that in some cases it is possible for contrib to trigger autoloading that modifies the contents of the classloader's internal structures.
Proposed resolution
Don't set the class loader as a global and run the related code within a function.
API changes
Not sure its really an API change but the global loader object will be removed. The existence of the loader global was incidental to how the code was written and not a feature/api and this is testing the change should be acceptable. Additionally, fetching the class loader should use composer's API not this global for correctness and accuracy.
Comment | File | Size | Author |
---|---|---|---|
#10 | bootstrap_php_loader-2631478-10.patch | 2.96 KB | neclimdul |
#10 | bootstrap_php_loader-2631478-10,interdiff.txt | 865 bytes | neclimdul |
Comments
Comment #2
neclimdulPatch.
Comment #3
Mile23I agree with this change, but I'm pretty sure the original code was designed that way to make tests faster. Whether it's worth it to break isolation is doutbtful, AFAIC.
Comment #4
neclimdulYeah I thought about doing it differently because there are a couple alternatives but I think this is probably ok.
1) We could call the composer method directly but that only works because we hard code the autoloader-suffix which I've been told we want to remove. After that the class name is unpredictable so we can't hard code it.
2) We could store a static in the bootload.php populate method and return the classloader each time its called.
3) We force everything to exclude the loader from global watches.
1 & 2 Are going to be more expensive then accessing a super global because its a function call. I'm not sure its really any different then the require in practice.
3 is what we're trying to fix because it isn't not really a reasonable requirement for tests.
So I settled with the require change to KTB because its just the way composer is designed to be used and the least likely to cause problems at any point in the future.
Comment #5
dawehnerYeah practically everything else which happens during a test is way more costly, so let's not worry about that.
+1
Comment #6
klausiI can confirm that this fixes the risky tests in Rules, thanks a lot!
Super minor nitpicks:
".. to avoid setting the ..."
one space too much after the @var
EDIT: and there should be no parenthesis after the class name.
Comment #7
neclimdulfixed.
Comment #8
Mile23I'm calling this RTBC even though the tests are still running.
This really should be injected, but that's out of scope here.
The namespace scan happens once per class anyway on the testbot, because each test is a separate process.
Calling it RTBC but the testbot is still free to disagree.
Comment #9
catchThis isn't a sentence. Not able to think of a quick suggestion.
Comment #10
neclimdulWow... to think that made sense in my head at one point.
Touched up the following statement to clarify where the false positive is as well.
Comment #11
neclimdulComment #12
dawehnerYeah that happens to me ALL the time.
Comment #13
alexpottCommitted ea3f8b0 and pushed to 8.0.x and 8.1.x. Thanks!