Problem/Motivation
This issue is about allowing to optionally use PHPUnit ^7 to run tests in Drupal 8, while keeping support for PHPUnit ^6.5 .
- PHPUnit 6 is out of support since Feb 1, 2019. So we are already using an unsupported PHPUnit version in our testing ecosystem.
- PHPUnit has a very strict policy of releasing a new major in line with PHP releases, and supporting only current and current-1 major:
- PHPUnit 7 was released in early 2018 and will be supported until Feb 7, 2020
- PHPUnit 8 was released in Feb 2019 will be supported until Feb 5, 2021 - #3063887: Support PHPUnit 8 in Drupal 9, drop support for PHPUnit 7
- PHPUnit 9 will be released Feb 2020.
The problem to solve to support PHPUnit 7 vis-à-vis PHPUnit 6 is that it changes the signature of a number of its methods and removes some that were deprecated earlier, making it impossible to extend its classes in a way that can work seamlessly between the 2 versions. In a pratical example, PHPUnit 6 declares PHPUnit\TextUI\ResultPrinter::printResult
like
public function printResult(TestResult $result);
whereas PHPUnit 7 like
public function printResult(TestResult $result): void;
so you cannot extend for example from ResultPrinter and override a method in a unique way, because in one of the versions PHP will step in and throw a fatal error for the different type hinting.
Several classes and methods in the current test codebase are extending and overriding directly from PHPUnit classes and are subject to this issue - listeners, asserts, etc.
Proposed resolution
Create a bridge between PHPUnit and the test codebase, that making use of class_alias
loads classes and traits depending on the PHPUnit runner version. These classes/traits adhere to the runner version required type hinting, and are conveniently grouped in namespaces organized by runner version - for PHPUnit6 and PHPUnit7 here. This allows to (a) easily drop support for a PHPUnit version by removing all the classes of a PHPUnitX bridge and upping the composer constraint, and (b) add support for future PHPUnit versions by adding PHPUnit8, PHPUnit9 and so forth bridges - when time comes.
AFAICT, the concept is generally BC, as it allows to run the tests on both PHPUnit 6 and on PHPUnit 7 with a single test codebase, as latest patches show. Contrib might need to adjust some assertions, though, or tests that depend on the internals of PHPUnit.
It would not be necessary to force the composer.lock to be updated. We can just loosen the composer.json constraint to allow PHPUnit 7 to be installed, similiarly to the approach that is being taken, AFAICS, for #2976394: Allow Symfony 4.4 to be installed in Drupal 8. Then hi/lo dependencies testing will ensure running on both versions.
Remaining tasks
review
User interface changes
none
API changes
none
Data model changes
none
Release notes snippet
Drupal 8 now supports PHPUnit 6.5+ and PHPUnit 7. PHPUnit 6 will be used by default, but PHPUnit 7 is required for testing on PHP 7.4. If you are using core's root composer file, you can update to PHPUnit 7 by running composer run drupal-phpunit-upgrade
. PHPUnit 7 is required for testing on PHP 7.4.
Original report:
I know that the testing infrastructure just switched to phpunit 6.5 two month ago but in the meantime PHPUnit 7 was released.
so the deadline to switch, and some important dates
- PHPUnit 6 support ends on February 1, 2019 https://phpunit.de/
- PHPUnit 6 does not support PHP 7.3 which is planned to be released late 2018 http://php.net/supported-versions.php
- with Drupal 8.7 March 6, 2019 we can stop using PHPUnit 4 as only PHP7 will be supported by Drupal https://www.drupal.org/node/2938726
so the deadline is like ~8 months from now based on PHP 7.3 release.
can PHPUnit 7 be used from now?
- the problem might be PHP 7.0 as it is not supported by PHPUnit 7
- PHP 7.0 will be EOL 3 Dec 2018 http://php.net/supported-versions.php
so it seems 2018 Nov-Dec is the date to switch..
.. or start using 3 different versions of phpunit now:
PHP 5.5; 5.6 > PHPUnit 4 (4.8)
PHP 7.0 > PHPUnit 6 (6.5)
PHP 7.1; 7.2; 7.3 > PHPUnit 7
Comment | File | Size | Author |
---|---|---|---|
#162 | 2950132-2-162.patch | 55.3 KB | alexpott |
#159 | 2950132-2-159.patch | 55.31 KB | alexpott |
Comments
Comment #1
PasquallePasqualle created an issue. See original summary.
Comment #2
PasqualleComment #3
PasqualleComment #4
mondrakeThe big problem here is that PHPUnit 7 changes the signature of a number of methods and removes some that were deprecated earlier.
I have been trying to see the extent of the changes; patch attached that should allow running quite a lot of the test base on PHP 7.2 + PHPUnit 7, but mostly here to document the changes needed somehow.
Comment #6
mondrakeTrying to fix some fails.
Comment #8
mondrakeComment #11
alexpottI think we also need to find out if PHPUnit 6 is really incompatible with PHP 7.3 before embarking on this. Also I think we should consider going the Symfony route and not having PHPUnit in our composer.json.
Comment #12
catchComment #13
jibranComment #14
andypostQueued last patch for php 7.3 tests
Comment #15
trzcinski.t@gmail.com CreditAttribution: trzcinski.t@gmail.com commentedHi,
it was not applying for the latest 8.6.x branch - rerolled it. There was an update in the composer.json file and that caused it to fail.
Setting tests to 8.6.x for now. I will be testing that more thoroughly since I want to write tests on PHPUnit7.
Comment #16
alexpottSo in answer to #11 - we've not hit any problems what-so-ever in using PHPUnit 6 with PHP 7.3 - so the urgency to upgrade is not there atm.
I think we need to plan for the following in Drupal 9
This solves a number of problems about supporting and testing on a wide range of PHP versions.
Comment #17
mondrakePhpUnit 8 was released today, does it warrant a separate issue of its own?
Comment #18
andypostPHPUnit 8 receives bug fixes until February 5, 2021
PHPUnit 7 receives bug fixes until February 7, 2020
Comment #19
mondrakeSome work in the direction of loading conditionally traits and/or classes depending on the PHPUnit Runner version. We need to avoid loading code that uses PHP7 syntax in PHP5, or we get compiler errors. No interdiff as changes are significant. Any comment on the approach welcome.
Comment #21
mondrakeSome progress.
Comment #23
mondrakeMore.
Comment #25
mondrakeMore.
Comment #26
alexpott@mondrake - I think we need to re-evaluate whether we use PHPUnit via composer. I'm quite sure that we need to take Symfony's approach and use their simple-phpunit so that we can decouple our dependencies from our test framework. We're moving at different speeds and want to support a wider range of PHP versions.
Comment #27
mondrakeThanks @alexpott.
Is that for building the dependencies outside of Composer, or is there more to it? In previous patches I'm more focusing on getting tests pass on various PHPUnit versions that are using different signatures of the same methods, so not so much related to the building but rather to the execution.
Comment #29
alexpottYep exactly. I'm not saying this work is not valid - just trying to say that we should probably solve that first because it might influence how do things. Also not having PHP 5 will make this easier. So 8.8.x is probably the place to get the ball really rolling here. But the progress is looking good! Only 10 fails left.
There's also the issue on min max testing which feels relevant here as well.
Comment #30
mondrakeOK - then shouldn't we have a separate issue opened for 'Use Symphony simple-phpunit to include PHPUnit and dependencies in the test codebase build'?
Also - isn't #2976407: Use drupalci.yml for composer dependency min/max testing - DO NOT COMMIT worth being bumped to critical?
I understand we will have to wait for 8.8.x for this, no worries. In the meantime the closer we can get the better, I think.
Comment #31
mondrakeComment #32
mondrakeSome more test fixes. Looks like class_alias() works with Traits, too, so we could rework the rather unconventional way of loading traits conditionally I used in the patches so far.
Comment #34
mondrakeSpinned off #3031466: EntityResourceTestBase::assert406Response uses a wrong assertion and results in false positives (and fails for PHPUnit 7).
Comment #35
mondrakeSpinned off #3031539: FileFieldTestBase tests fail under PHPUnit 7.
Comment #36
mondrakeRe #32
done that.
Comment #37
mondrakeComment #39
mondrakeI'll pause for now - the remaining failures are related to tests that are testing the test classes :) Those would need some work that it's not useful to pursue now.
With this patch, you could reasonably use PHPUnit7 for 'normal' tests.
In the meantime, reviews of the two spinoff issues #3031466: EntityResourceTestBase::assert406Response uses a wrong assertion and results in false positives (and fails for PHPUnit 7) and #3031539: FileFieldTestBase tests fail under PHPUnit 7 would be very helpful as having those in 8.7.x would help later work. Actually, I think #3031466: EntityResourceTestBase::assert406Response uses a wrong assertion and results in false positives (and fails for PHPUnit 7) just surfaces an issue with the current tests themselves.
Comment #41
mondrakeNeeds to be adjusted for #3031577: \Drupal\Tests\Listeners\DeprecationListenerTrait::getSkippedDeprecations() does not work in unit tests
Comment #42
mondrakeReroll.
Comment #44
Gábor HojtsyUpdating title to be more accurate based on what is going on.
Comment #45
Pasquallehttps://phpunit.de/supported-versions.html
So, phpunit 6 is not supported any more, and even composer started to complain about it #3033761: Package phpunit/phpunit-mock-objects is abandoned
What should be the recommended step for developers now? Should we update to phpunit 7 (or 8), or should we remove phpunit from composer?
Comment #46
gillesbailleux@Pasqualle: thank you for raising the concern about composer. Using composer version 1.8.3, the following message appears when typing
composer update
on a fresh D8 installation:Comment #47
rivimeyFound this issue for the same reason as #45; it would be nice to see mention of a plan, or is the plan "wait for 8.8?"
Comment #48
BerdirNothing is broken, it's just an annoying message. The project is still there and won't be deleted, the maintainer just made it *very* clear that there will be no further fixes/changes there.
So yes, this will likey definitely have to wait until 8.8 because updating that is likely going to break a decent amount of tests in core and contrib.
Comment #49
jedgar1mx CreditAttribution: jedgar1mx commentedIt definitely seems like a low priority, but it would be nice not to see it on my logs lol
Comment #50
philosurfer CreditAttribution: philosurfer commentedclean_logs++
Comment #51
jatinkumar1989 CreditAttribution: jatinkumar1989 commentedI am also facing same issue, below is my composer. json part:
Comment #53
alexpottJust created an issue to move to simple-phpunit - #3039344: Use Symfony's simple-phpunit
Comment #54
mondrakePostponed on #3039344: Use Symfony's simple-phpunit
(and maybe on #3039287: Implement changes required to remove support for PHP 5.5 and 5.6?)
Comment #55
mondrakeIt looks like in Symfony's simple-phpunit the earliest release that will support PHPUnit 7 will be Symfony 5, as I can see it's only loaded in the master branch at the moment. Symfony 4.2 is using PHPUnit 6.5 only for PHP 7.2+.
Comment #56
alexpott@mondrake which is absolutely fine because it works.
Comment #57
mondrake@alexpott sure, just reporting here to say this issue may not be needed until Symfony 5 is supported.
Comment #58
fgmFWIW, a side-effect is that this locks usage of sebastian/phpcpd at 3.0.x : versions 4.x require php-timer:^2.0 which is not compatible with phpunit/phpunit:^6 (requiring php-timer:^1.0).
Comment #59
jibranBlocker is in.
Comment #60
jibranPlease ignore me. I was looking for #3008870: Drop support for PHPUnit 4.8 once PHP 5 is no longer supported (8.8.x)
Comment #61
NikLP CreditAttribution: NikLP commentedSo for the less savvy users out there, is the current course of action for "Package phpunit/phpunit-mock-objects is abandoned, you should avoid using it. No replacement was suggested" (using php 7.3) basically "do nothing" at the moment?
Comment #62
acb CreditAttribution: acb commentedIt feels to me that relying on a unitTesting framework that is no longer supported is not the wisest course of action. Drupal already has a reputation for lagging, and this is yet another example of it. As a general principle, we should stay current. As a matter of practicality, downloading and installing all sorts of no-longer-supported code is a headache, and prone to break.
From https://phpunit.de/getting-started/phpunit-6.html:
Please note that PHPUnit 6 is no longer supported. Also note that PHP 7.0 is no longer supported.
Comment #63
alexpott@acb it is not practical to follow PHPUnit's release schedule. PHP 7.0 will receive years more support via the Ubuntu LTS programme regardless of PHPUnit's documentation. We are not alone, Symfony 3 is still tested using PHPUnit 4 on PHP 5.3 for example. We are taking efforts to synchronise our supported PHP versions with PHP's release schedule but it is not simple.
@NikLP yes do nothing is exactly the right course of action. Unfortunately there's nothing we can do about this message whilst we are still testing using PHPUnit 4.
We are working on decoupling core from the test framework see #3039344: Use Symfony's simple-phpunit - once we are no longer dependent on PHPUnit these issues will not be so visible for users.
Comment #64
mondrake#42 no longer applied, reroll
Comment #65
mondrakeComment #66
mondrakeDeclaration of
Drupal\Tests\Listeners\SimpletestUiPrinter7::write($buffer): void
should be compatible withPHPUnit\Util\Printer::write(string $buffer): void
Comment #68
mondrakeDeclaration of
Drupal\Tests\Listeners\DrupalListener7::endTest(PHPUnit\Framework\Test $test, $time): void
must be compatible withPHPUnit\Framework\TestListener::endTest(PHPUnit\Framework\Test $test, float $time): void
Comment #69
mondrakeComment #70
mondrakeReroll after the commits of
#3053363: Remove support for PHP 5 in Drupal 8.8
#3008870: Drop support for PHPUnit 4.8 once PHP 5 is no longer supported (8.8.x)
Since
drupal-phpunit-upgrade
anddrupal-phpunit-upgrade-check
are now gone, added a container_command to drupalci.yml to require the upgrade of PHPUnit & friends.Comment #71
mondrake... and the status change ...
Comment #73
mondrakeComment #74
mondrakeThis should be green now.
1. With one major of PHPUnit released every year, I think there should be a way to allow permanently bridging different versions of PHPUnit with the test suites.
For this purpose, I suggest to create a separate namespace for the shims necessary for each version. This code must not come across PHPUnit test discovery otherwise we risk it gets autoloaded and then we incur in issues like #3034015: Class to test deprecation error for deprecated trait triggers deprecation error itself.
So here I am putting everything into
Drupal\Core\Test\PhpUnitCompatibility\PhpUnit6
(or...7
).2. This is not dependent on #3039344: Use Symfony's simple-phpunit, here we just run PHPUnit 7 regardless of how the codebase is built.
Comment #76
mondrakeFix for failing run on PHPUnit6.
Comment #77
mondrakeComment #78
mondrakeComment #79
alexpottNo longer necessary to call the parent because the names have changed.
Comment #80
mondrake#79 thanks, it's a shortcut here, please review #3031539: FileFieldTestBase tests fail under PHPUnit 7 instead. This hunk will just go away with that in.
Comment #81
mondrakeReroll after #2929133: Replace all usages of getMock()
Comment #82
mondrakeComment #83
mondrakeReroll after #3053417: Clean up PhpunitCompatibilityTrait.
Comment #84
mondrakeComment #85
mondrakeComment #86
mondrakeRerolled and adjusted after #3031539: FileFieldTestBase tests fail under PHPUnit 7.
Comment #87
mondrakeDeprecation test won't work in PHPUnit7 since assertFileExists only accepts string argument for $file, fails otherwise.
Comment #88
mondrakeComment #89
mondrakeComment #90
mondrakeComment #91
mondrakeComment #92
Wim LeersJust wanted to say thanks for this epic work! 🙏 This looks really complicated, I'm grateful that somebody is pushing this forward! 👏
Comment #93
mondrake#92 thank you!
The real fun will come when trying to support PHPUnit 8... Preview @ #3063887: Support PHPUnit 8 in Drupal 9, drop support for PHPUnit 7 :)
Comment #94
Mile23DrupalCI has a composer step, so you can do:
But what we really should do is add the
drupal-phpunit-upgrade
script back to composer.json, since that's established in everyone's minds. Even though they kind of hate it. :-)Comment #95
mondrakeJust a reroll of #89, now that #3031466: EntityResourceTestBase::assert406Response uses a wrong assertion and results in false positives (and fails for PHPUnit 7) and #3062122: phpunit: Code Coverage Fix Up have been committed.
Comment #96
mondrakeAdding an helper class/method to determine the major version of PHPUnit being run, plus done some optimization/cleanup.
@Mile23 - re. #94:
that does not seem to be documented, at least not in https://www.drupal.org/drupalorg/docs/drupal-ci/customizing-drupalci-tes...
In any case, IMHO in my view we should not be focusing on how the codebase is built in this issue, there are at least #3039344: Use Symfony's simple-phpunit and #2976407: Use drupalci.yml for composer dependency min/max testing - DO NOT COMMIT for that or other issues may be opened to cover the DrupalCI integration. Ideally, in my view, the patch here should just do the tweaks to allow running on PHPUnit 7 if someone wants to; projects testing on other CI platforms may be interested to be free in making their choice of which PHPUnit version they want to use for testing.
Comment #97
mondrakeRe-adding the PHPUnit 4 legacy listeners, removing them is scope for #3060391: Cleanup usage of PHPUnit 4.8 aliased classes.
Comment #98
mondrakeLooks like equal comparison for floats cast to string has become more strict with PHPUnit 7.
Comment #99
mondrakeneeds a reroll
Comment #100
mondrakeRerolled.
Comment #101
mondrakeComment #102
mondrakeReroll.
Comment #103
mondrakeReroll.
Comment #104
mondrakeReroll.
Comment #105
Gábor HojtsyFirst, thanks for this epic work. I hit this when debugging https://github.com/drupal8-rector/drupal8-rector/issues/20 -- updating to PHPUnit 7 would allow to include rector/rector in a Drupal project and thus modernize the drupalmoduleupgrader infrastructure to be based on rector rather than the abandoned pharborist.
IMHO it would be quite important to explain in the issue summary how PHPUnit 7 support is achieved while keeping PHPUnit 6.x support for PHP 7.0.x users. Whether any changes are considered BC breaking here, etc. I think that would greatly enhance the chances of landing this :)
Comment #106
mondrakeUpdated IS.
Comment #107
mondrakeComment #108
Gábor HojtsyThanks for doing that. The changes look good to me although this will definitely need to have a release manager / framework manager collaboration to land.
Comment #109
alexpottShould this be part of the rtbc patch? I guess so - other but the comment could be more complete and say when we expect things to be updated. So once PHP 7.0 support is dropped we'd no longer be testing on PHPUnit6. The fact is that whilst PHPUnit 6 is not official supported on PHP 7.3 and greater it is happily working for us on PHP 7.3.
I'm wary of dropping testing using PHPUnit 6 on PHP 7.3 because many people might have sites built that test using that combination at the moment. And it likely that PHP 7.3 will be supported for quite sometime. A massive positive of this patch is that it allows these sites to upgrade to a supported version of PHPUnit - but this opens the door to us breaking existing support PHPUnit 6 once we no longer test on it (i.e. the moment PHP 7.0 is dropped). Tricky.
In other frameworks this would be a class and not abstract. And it would have something like
Sometimes such a class would also be made final.
See
\Zend\Diactoros\HeaderSecurity
and
\Symfony\Component\Process\ProcessUtils
Comment #110
mondrakeThanks for the reviews.
Re #109:
1. IMO we should not commit the drupalci changes. Rather, we stick to PHPUnit 6.5 in regular testing on all PHP versions. Then, #3044155: PHP7.1 vendor max Testing issue - DO NOT COMMIT (or in the future, other hi/lo dependencies testing features) will run daily the highest dependency testing, including upgrade to PHPUnit 7.
2. Let's draft the CR once there is agreement on 1. - in that case the CR will only mention that PHPUnit ^7 can be used for testing, still with ^6.5 being the baseline testing environment.
3. Done here.
4. :) and :(
Here with two patches, one with and one without the DrupalCI changes. The one with no changes is for commit if 1. has consensus.
Comment #111
alexpott@mondrake +1 to the plan to leave this to min/max - that does mean we have no explicit test coverage but min max testing exists in the rtbc atm so I'm not sure what else we can do. It's certainly the least disruptive. If we do this then the CR only needs to point out the PHPUNit 7 is supported and will be installed if you run composer update and if you project has a requirement on PHPUnit 6 then you need to fix that in your own root composer.
Comment #112
mondrakeDrafted CR https://www.drupal.org/node/3078763
Comment #114
mondrakeOuch, #110 failure hurts. Drupal\Tests\ComposerIntegrationTest fails because it compares the composer hash of the current composer.lock file vs its rebuilt one from the current composer.json file. Since in the patch without the drupalci changes we change the composer.json file to allow PHPUnit ^7 without adjusting the lock file, the test fails. I think we can only change the composer.lock hash based on the results of the test... Then I think with min/max testing and composer update happening before the actual run of the tests things should work fine. AFAICS any future patch trying to loosen composer.json constraints will suffer from this...
Comment #115
mondrakeassertSame requires ($expected, $actual)
it was viceversa in the test
Comment #116
mondrakeSorry, I forgot to remove the drupalci part.
Comment #117
mondrakeComment #118
mondrake@Gábor Hojtsy re #105: with the patch in #116 it is possible to install
drupal8-rector/drupal8-rector
in the codebase build - see https://github.com/drupal8-rector/drupal8-rector/issues/20#issuecomment-...Comment #119
amitgoyal CreditAttribution: amitgoyal commentedThanks @mondrake and @gábor-hojtsy, with the patch in #116, we were able to install drupal8-rector/drupal8-rector.
Comment #120
Gábor HojtsyThanks for the change record and for the fixes! Let's get this in then!
Comment #121
alexpottSo this essentially means that we've broken this functionality when PHPUnit7 is in play. At least the error will be straight forward and simple to fix but it does mean that tests that are passing (with deprecations) are not passing when you swap to PHPUnit7. At the very least this needs to go into the issue summary and change record. And it probably needs further discussion.
Let's add this to the HtmlOutputPrinterTrait as
::SimpletestUiWrite()
- the trait is already used by the parent classes.This feels wrong place to do this. This only exists for the \Drupal\Tests\Core\Test\TestSuiteBaseTest() - we can fix this by doing a conditional in the main in that test and not pollute core with this test only code. Ie. do something like
Yes it is ugly but all the ugliness is in the test and doesn't spill over into core.
We should file an issue to explore deprecating this behaviour.
This looks really odd. So to do the comparison to a float we have to change a string to an int but and string comparison to int below works just fine. Also changing the order in just this one place whilst leaving the rest of the file alone is confusing. I think it is better to have the wrong order and fix the argument order in another issue.
I guess that as pointed out in #98 float comparisons have changed. A couple of thoughts are:
This change is out-of-scope and hopefully unnecessary.
Comment #122
alexpottThere's already an issue about assertTrue and assertFalse - #2742585: Deprecate dangerous assertTrue/False() compatibility overrides in PHPUnit Kernel, Functional and FunctionalJavascript tests
Comment #123
mondrakeThanks for the review @alexpott. On it.
Comment #124
mondrakere #121:
1. Leaving open for further discussion. I do not see ways around that ATM. Not changed the IS or CR yet.
2. I am not sure I understand - open. Can you please elaborate?
3. I am afraid we can't do that as long as we support PHP 7.0 - typehinting function return values to
void
was introduced in 7.1.4. Added @todos to the referred issue.
5. Reverted the order of arguments, and returned at using
assertEqual
. Do I understand right the proposal would be to modifyTestCompatibilityTrait::assertEquals
to cast both expected and actual values to float if either one or the other are float (viais_float
)? Should also aTestCompatibilityTrait::assertNotEquals
method be added to matchassertNotEqual
?6. Reverted. Agree it's out-of-scope here. Anyway, IMO the status in HEAD is wrong -
$lock['content-hash']
is the expected value as read from the composer.json file, and as such it should be the first argument in anassertSame
.Comment #125
alexpottre #124.3 well then we should put the stubs in a fixtures directory with different names for the PHPUnit versions. You're right my solution doesn't work but there are other solutions that don't result with this ending up in the main
Drupal\Core\Test\PhpUnitCompatibility
namespace.re #124.2 I mean add
to core/tests/Drupal/Tests/Listeners/HtmlOutputPrinterTrait.php
and then in then core/lib/Drupal/Core/Test/PhpUnitCompatibility/PhpUnit6/SimpletestUiPrinter.php (and the Phpunit7 version) change the write method to
Also I've just realised that the new compatibility classes have moved to core/lib/Drupal/Core/Test/PhpUnitCompatibility - these should not be here. They have no business being autoloadable during general runtime. They can stay in core/tests/Drupal/Tests a sub directory in there should be fine.
Comment #126
mondrake#121.1 - still open for discussion, I can update IS and draft CR if there is consensus we stay as is
#121.2 - Got it, thanks. Done here.
#121.3 - IMO these thingos are neither core nor test code. The suggestion to move these to a fixture directory is a reflection of that... Also,
but the point is that in that way the thingos will be autoloaded during the test dicovery since they are in a test namespace, and if the syntax is not supported by a specific PHP version, then we will fail badly. This may be less evident now, but in earlier exercises when we were still testing with PHP 5 that was big. An idea could be to add a, say,
phpunit
subdirectory to thecore
subdirectory and move the thingos there (maybe listeners, too, in a separate issue), and somehow register this in the test bootstrap.php file as a separate namespace, so that we can still use autoloader without startingreguire_once __DIR__
dances. Something discussed about this with @Mile23 in #2905007-68: Allow run-tests.sh to report skipped/risky/incomplete PHPUnit-based tests and #69.#121.4 - OK#121.5 - Second time thinking here - IMO PHP and therefore PHPUnit are becoming stricter in type checking, and I think we should move along with that instead of trying to relax our testing. So I'd go for just mentioning in IS and CR that assertEqual for float checking need to be revised in contrib tests. That is the direction that #2742585: Deprecate dangerous assertTrue/False() compatibility overrides in PHPUnit Kernel, Functional and FunctionalJavascript tests is taking, no?
#121.6 - OK, do we need a follow-up to revert the order of the arguments in
ComposerIntegrationTest
?Comment #127
alexpott@mondrake I'm not sure that
is true. We only try to instantiate classes which end in *Test.php during test discovery as far as I know. And if this is not true then we need to explore solutions that don't pollute the runtime autoloader with these classes.
Comment #128
mondrake#127 right that happens when running tests via run-tests.sh and DrupalCI. But you might run tests with PHPUnit directly, and when it runs --testsuite(s) or --group(s), PHPUnit autoloads those classes, at least it was doing in #3034015: Class to test deprecation error for deprecated trait triggers deprecation error itself. Happy to be wrong, though ... :)
Comment #129
mondrakeBTW
fully agree here
Comment #130
mondrakeComment #131
mondrakeRe #121.3 and #125
Here I am introducing a new namespace
Drupal/Tool
from separatecore/tools
subdirectory, and moving all the compatibility code there. This way we do not polluteDrupal/Core
and as well we keep it away fromDrupal/Test
. Just an idea, naming and location can obviously change.Oustanding: #121.1, 3, 5, 6.
Comment #132
mondrakeRerolled.
Comment #133
mondrakeRerolled.
Comment #134
alexpottAs we need this for PHP 7.4 compatibility we need to go back to a way of switching between PHPUnit version. This patch adds back in the composer script to update PHPUnit and if we're on PHP 7.3 or above switches to PHPUnit 7. I choose PHP 7.3 so we can validate PHPUnit 7 before we've got a green run on PHP 7.4. See #3086374: Make Drupal 8 & 9 compatible with PHP 7.4 for more 7.4 goodness - atm we have 1 other issue but that's likely to increase once we can actually run tests on it (which this will allow us to do).
Comment #135
alexpottThis for me is still not what we want. The test code added here shouldn't be autoloadable by the regular run-time. We need to resolve this in another way.
Comment #136
alexpottHere's another way we're we don't change the autoloader in the root composer.json (which actually would break composer built drupal projects!) and only affects the test autoloader but works well with test discovery.
Comment #138
alexpottThe composer update is not running on DrupalCI properly... also broke lots :)
Comment #140
mondrakeRe. #135 can we move that to the
autoload-dev
section in composer.json? I am not sure #138 will work when running tests via PHPUnit directlyComment #141
Wim Leers#134 🤦♂️😔 — nothing we can do about PHPUnit's choices of course…
Comment #142
alexpott#138 works just fine when running PHPUnit's tests directly - core/tests/bootstrap.php is what makes the test system autoloadable for it already.
Comment #143
alexpott@mondrake your concerns about test discovery are entirely valid but core/tests/Drupal/TestTools is placed outside test discovery - so running all the unit tests with something like
$ ./vendor/bin/phpunit -v -c ./core --testsuite unit
still works a treat with PHPUnit 7 or PHPUnit 6.Comment #144
mondrake@alexpott re. #142, #143: yep, confirmed, I tested the patch with direct PHPUnit calls by --group and by --testsuite, all good. Thanks and sorry for the noise.
Comment #145
alexpottOne place we need to update the autoloader for tests so the UI works.
Comment #146
mondrakeI think this is no longer needed.
Other than that, +1 on RTBC, but will need some adjustments on the CR for #121.1, and #121.5. Obviously I cannot RTBC myself.
Re. #121.6 I think I misinterpreted the purpose of that test, and HEAD is correctly checking that the value of the content-hash as calculated from composer.json (expected) is the same as the actual one stored in composer.lock (actual). Since that was corrected in #124, all good here.
Comment #147
alexpottGood call in #146. We're probably going to need to for PHP 8.0 too so removing the entire todo seems best. Once we remove the lock file this all gets a bit easier.
Comment #148
mondrakeComment #149
mondrakeNeeds a reroll, alas
Comment #150
alexpottRe-rolled. Our lock file feels very unstable at the moment. Doing
composer update --lock
on HEAD should be a no-op but its not. See #3082866-26: composer complains "Skipped installation of bin bin/composer for package composer/composer: file not found in package".Here's a re-roll.
Comment #151
alexpottRerolled. Also this is a critical task because it is blocking PHP 7.4 compatibility.
Comment #152
larowlanNasty, but necessary evils etc - some real black magic going on here - one question -
is this not something we can do in drupalci.yml?
we can null coalesce these now right?
Comment #154
alexpottRe #152.1 - nope - we want contrib to be able to test on PHP 7.4 too.
I fixed #152.2 a bit differently - with a string cast - just in case someone decides to try to get Drupal 8.7.x to test on PHP 7.4.
Comment #155
alexpottThis is targeting 8.8.x atm
Comment #156
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedI just wanted to draw your collective attentions towards [Composer Test Scenarios](https://github.com/g1a/composer-test-scenarios). The idea is that you define new named test scenarios in your composer.json file. In this context, a "test scenario" is a collection of dependency versions that you want to test together. For example, this patch defines a new scenario consisting of upgrades to:
If we named this scenario "phpunit7", then you would be able to switch to it by running
composer scenario phpunit7
. This can be done for ad-hoc testing, or as a step in drupalci (or perhaps automatically from a Composer hook, as this patch currently does to call composer udpate).The advantage of defining a test scenario is that Composer Test Scenarios generates a separate composer.lock file for each scenario it manages. Without a lock file, e.g. as is the case with this PR, phpunit7 are always "highest" tests for the dependencies that are updated. Since we are only updating "require-dev" dependencies here, the use of a lock file is not as important as if we needed to vary the non-dev dependencies. That is usually only an issue for libraries; it seems unlikely that Drupal would ever want to define non-dev scenarios. I just thought I'd point this library out in case anyone was interested in the extra bit of organization.
Comment #157
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedHey, what happened here? I didn't refresh before posting, cross-posted with @alexpost, and it hid his two patch files? I don't even see them in the list of uploads to un-hide. They are still available from his comment, though.
Anyway, I was just waiting for his answer to @larowlan before RTBC'ing. This looks good. LMK if there's any interest in setting up test scenarios as a follow-on.
Comment #158
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedTo be clear, yes, I intended to RTBC this issue in #157.
Comment #159
alexpottChasing HEAD.
Comment #160
mondrakeComment #161
alexpottUpdated release note.
Comment #162
alexpottHere's a reroll because composer.json has changed. Note I've confirmed that the lock file is correct for 8.8.x, 8.9.x and 9.0.x
Comment #163
catchThere are some very ugly things in here, but they're not our fault and we have longer-term plans to clean them up.
This would be good to get into alpha to flesh out any issues with it earlier rather than later, we don't really have an option other than what's in the patch if we don't want to fall behind with phpunit/PHP itself.
Comment #164
alexpottUpdated issue credit and created @Gábor Hojtsy and @Pasqualle for issue management and @larowlan and @Mile23 for code review.
Comment #168
catchCommitted bcee1df and pushed to 9.0.x. Thanks!
(and cherry-picking back to 8.8.x)
Comment #169
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedHm, I upgraded to 8.8.x-dev and am running PHPUnit 6.5.14 and I get the following error when running tests:
Edit: nevermind, our project had a custom autoloader and needed the following addition:
This comment might help someone who ran into the same error message.
Comment #170
pookmish CreditAttribution: pookmish commentedI'm getting the same error as @Sam152 with a fresh composer installation and using drupal-check. Nothing custom done and it produces the same error. I can reproduce the error if i checkout the commit that was made in #168. If i checkout any commit prior to that one, I don't receive the error.
This particular issue appears to stem from `mglaman/phpstan-drupal` library. I've opened a conversation about this specific scenario at https://github.com/mglaman/phpstan-drupal/issues/87
Comment #171
xjmLet's work #169 and #170 into the CR and release note, so that we can reduce the number of bug reports following the 8.8.0 ugprade. Thanks!
Comment #172
xjmComment #173
mradcliffe#169 and #170 are committed in the primary branch of mglaman/phpstan-drupal, but not in a release version. Composer failed to resolve the dependencies when I tried to pull in phpstan-drupal's primary branch and then drupal-check because of that. In order to pull in the changes until a release is made I needed to run:
Those instructions probably should not be used in the change record. It would be preferable to only need to run an update.
Comment #174
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedAside: It is best not to use
composer global require
for packages that are not Composer plugins, as it can cause unintended problems. See Fixing the Composer Global Command.Comment #175
alexpottAdded #169 to the change record. It did not add this to the release note snippet because it is extremely technical and the PHPStan issue has been fixed already.