Problem/Motivation
We have hit problems with the security release of 8.6.7 and the bug fix release of 8.6.8. See:
- #3026386: Drush fatal error after upgrading to 8.6.6, 8.5.9, or 7.62: PHP Fatal error: Uncaught TYPO3\PharStreamWrapper\Exception
- #3026443: \Drupal\Core\Security\PharExtensionInterceptor is incompatible with GeoIP and other libraries that use phar aliases or Phar::mapPhar()
- #3031128: Update from 8.6.7 to 8.6.8 warnings - Drupal\Core\Extension\Extension has no unserializer
The technical reasons for each one are different but our tests that extends UpdatePathTestBase have not caught these problems. We need a way in which we can address the security issues caused by phar and the PHP bug behind #2701829: Extension objects should not implement \Serializable without causing these update woes.
The update programme - #3029353: First launch of the beta testing program - is one tack but this issue proposes to add a new test type.
General specifications for the new testing type:
- It will not use the branch being tested as a starting codebase
- It will not use the drupal environment to run - it will be based on Drupal's unit test case or PHP's.
- It will install the minimum version of Drupal supported determined by our policy and the branch that is being tested \Drupal::VERSION
- It will install Drupal codebase using composer
- It will install Drupal somehow
- It will log into the site as user 1
- It will then be able to do upgrades via composer / drush / tarball?
- It will then test the resulting upgrade.
Proposed resolution
- Create a new test suite. We’ll call it ‘build.’
- HTTP requests must be able to run in isolation from the system running the tests.
- Tests must be able to run in parallel under run-tests.sh.
- Each build test case will encapsulate a workspace in the file system, and potentially an HTTP server to allow for making requests against the assembled site.
- Tests will be able to execute external commands on the command line, such as git or composer, in order to manipulate the workspace.
- Tests will be able to copy the patched version of Drupal to the workspace.
- Using the provided HTTP server, tests will be able to make a request, and assert against the results.
- We’ll use Mink as a framework for assertions against content.
Remaining tasks
Remove the proof-of-concept tests from the patch before committing. Create followups as necessary to re-add them.
Determine which sorts of tests are out of scope for this framework.
Ensure we’re not re-running a large number of tests. See #44.
RM review of symfony/lock
as a new dev only dependency. https://github.com/symfony/lock/blob/master/CHANGELOG.md might be a good place to start.
User interface changes
None
API changes
New test type to use - \Drupal\BuildTests\Framework\BuildTestBase
Data model changes
None
Release notes snippet
A new test type has been added that can build a Drupal site. This will allow more realistic update testing. See https://www.drupal.org/node/3082383 for more information.
Comment | File | Size | Author |
---|---|---|---|
#242 | interdiff_241-242.txt | 1.04 KB | heddn |
#242 | 3031379-242.patch | 49.97 KB | heddn |
Comments
Comment #1
alexpottalexpott created an issue. See original summary.
Comment #3
alexpott#2984031: Create Build Tests For Composer and Drupal seems very related.
Comment #4
Mile23Comment #5
Mile23At DrupalCon 2019 sprints we brainstormed a little about this issue and similar issue #2984031: Create Build Tests For Composer and Drupal
I think the relatively narrow scope of both these issues helps us reach a conclusion.
Let's think about this issue first, since it addresses an existing gap in our test infrastructure. We'll keep in mind that the solution should be flexible enough to address #2984031: Create Build Tests For Composer and Drupal and similar build-based issues.
We can also have follow-ups to move existing unit-test-scoped builders to use the framework we make here. This would include the demo umami installer and so forth.
Comment #6
Mile23Tests should fail here. This is a proof of concept and is offered for discussion.
This patch adds a test type called
BuildTestBase
. The testsuite is 'build'.The main concepts here are:
The use case for this is as outlined in the IS: Performing an installation and then performing and update, and then performing requests.
There's some infrastructure and one test:
Drupal\BuildTests\BuildInstallTest
. It looks like this:This test fails locally like this:
This means the kernel request began to work, but the kernel happily changed the error handler without concern for phpunit-bridge's feelings.(Actually this doesn't mean that, because we never get beyond trying to load the autoloader.) You can see that I marked it@group legacy
, and I also tried to disable the bridge, but no luck.Edit: I neglected to add this testsuite to drupalci.yml so it won't run. Sigh.
Comment #7
Mile23Composer lock file suitable for PHP 7.0,
run-tests.sh --browser
now works, truncated drupalci.yml.Comment #8
Mile23Forgot interdiff...
Comment #10
Mile23An artifact from the last test:
So we use
require_once
instead.Comment #12
Mile23Rethinking so we're using PHP's test server as the HTTP server.
Currently does not work. Sharing here for review.
Comment #13
Mile23Here's a working test framework. You'll see there are plenty of @todos.
The main problem that exists now is that PHP's HTTP server isn't shut down by the time the next test is run. This would be ameliorated by using semirandom port numbers (so they can overlap for concurrency), and/or by adding a
wait()
in thetearDown()
method.Comment #15
Mile23Converted to Mink so we can do nifty assertions. We don't use UiHelperTrait because it needs a bunch of core stuff that is inappropriate for this test type.
Tests created with this framework can add all sorts of dependencies, like being able to run git. So we might consider being careful about that as we move forward.
The main TODOs here:
Comment #17
alexpotthttps://github.com/twigphp/Twig/pull/2993 does some really interesting stuff that we might be able to leverage.
Comment #18
Mile23That does look interesting.
We'd still need to set up the PHP HTTP server and use assertCommand() and friends to create the environment, but functional tests could be blackfire player scripts.
https://github.com/blackfireio/player
It has a --json output option, but no idea how that would map to run-tests.sh or junit, or other sundry concerns.
Comment #19
Mile23Discovered this while making some tests for this issue.
Comment #20
Mile23Now that I'm back from vacation...
Adds some prototype tests for existing needs. This helps us understand whether the framework meets our needs.
Still non-concurrent. We'll need to figure out a solution using a lock file to protect concurrent HTTP ports.
There's a little bit of trouble in that in order to do real black box testing of
update.php
, our mink client has to respond to meta refresh tags.UiHelperTrait
does this withcheckForMetaRefresh()
, but we can't inherit the dependencies ofUiHelperTrait
. That means we can either split up the trait or re-roll a similar solution here. The latter would involve expandingGoutteDriver::visit()
to perform refreshes, which might be a better solution that could replace that part ofUiHelperTrait
. Or not. Dunno.It's also true that as of 4.2.0, Goutte (via BrowserKit) will follow meta refresh tags for us: https://github.com/symfony/symfony/pull/27118
Anyway, the meat of the framework is in
core/tests/Drupal/Tests/BuildTests/Framework
. The rest are essentially POC tests.Comment #21
alexpott@Mile23 amazing work.
Some thoughts:
Why doesn't core pass this test? I look on the linked issue but it is far from immediately obvious to me. But any as above I'm not sure we should be using this test type for this test anyways.
Comment #22
Mile23The core dependency regression test looks like this (once you comment out the line that marks it as incomplete):
I was able to repro this on core like this:
THE ERROR HANDLER HAS CHANGED! is basically the least useful error message ever. :-)
Some hacking later and we discover that this error handler error goes away when we fix
DrupalKernelTest
: #3054649: DrupalKernelTest results in ERROR HANDLER CHANGED!There are still a bunch of other fail/error tests, so we need to address those. But
CoreDependencyRegressionTest
is more of a proof of concept in this instance anyway.Same deal... This patch only proves that the framework allows for that sort of thing, and all the tests outside of
Drupal\BuildTests\Framework
are POC, not for inclusion in the final patch. We can argue about them in other issues. :-)Sorry if I didn't make that clear.
Comment #23
Mile23Here's another one: #3054674: ThemeHandlerTest and friends break isolation for constants
Comment #24
BerdirNot quite sure how the current work here ties back to issue summary, and whether that is still accurate, but, two thoughts about the current issue summary:
* The reason we didn't saw those extension issues in our update tests is that our dumps have empty caches. That would be relatively easy to address by creating a dump that includes all cache data, would just be quite a bit of data. Maybe we can keep it out of git or use that git LFS stuff that gitlab supports? Or somehow fetch it on demand from somewhere?
* You can't install old versions of Drupal 8 on modern PHP versions, 8.4 and older doesn't install on PHP 7.2 for example.
Comment #25
Mile23The patch to review is in #20, and the stuff with fixing tests is a little off topic, yah.
I talked with @alexpott about this at DrupalCon and basically it's a black box test, where you do a build and then check expectations. This way we can perform an install -> upgrade -> see if we got what we want process.
There's a little scope creep in that I did some prototypes of other test types this build system would likely be used with, such as #2984031: Create Build Tests For Composer and Drupal and #2984068: Create a test that upgrades from one version of core to the next. and could be used for #2962157: TestSiteApplicationTest requires a database despite being a unit test
Comment #26
Mile23Another new child: #3055495: Search page migration tests are misplaced kernel tests
This patch adds a subclass of
Behat\Mink\Driver\Goutte\Client
that can follow meta redirects, with some tests.Improves port seeking for the HTTP server.
The big patch includes all the child issue patches which allow the composer min/max test to pass.
The framework_only patch does not include any of these changes, or the prototype tests.
Comment #28
Mile23Did I crack the code of concurrent ports? Let's ask the testbot.
Edit: Woops, the framework_only patch isn't only the framework.
Comment #30
Mile23The previous fail shows us this error which fails the tests which copy core:
This is a fail because we don't have permission to do anything with
sites/simpletest/dir/
. And why is that? BecauseDrupal\Tests\migrate\Kernel\process\FileCopyTest::testNonWritableDestination()
does not clean up after itself. #2793121-15: FileTestBase doesn't confirm its filesystem exists, does not clean upSo that leaves us with some options: Wait for
FileCopyTest
to get fixed or just never copysites/simpletest/
to the system under test. The latter seems completely reasonable regardless ofFileCopyTest
, so there we are.We add symfony/finder so it can make the file iterator for us, and then we copy core without
sites/files/
,sites/simpletest/
, orvendor/
.This patch also adds the ability to say
@requires externalCommand git
in the test annotation, so it can skip instead of doing a bunch of build operations only to fail for obvious reasons.Comment #32
jibranI don't understand why this has to be part of core. Why can't we setup deadicated project/jobs to run this?
Comment #33
Mile23Core maintainers seem reticent to trust a test that doesn't run under drupalci. For instance, this languishes, and I can only assume it's because Travis CI is inadmissable in court: #2876669-60: Fix dependency version requirement declarations in components
There was some small degree of work put into having different build targets within drupalci.yml. That way you could specify a different build process for RTBC or weekly regression test or whatever. Also languishes, not updated in a year, because its really only me and @mixologic: #2951375: Allow for build targets
So that leaves us with baking it into core, unless there's a better idea.
Comment #34
jibranWhy can't we setup a new project on drupal.org?
Comment #35
Mile23What sort of project?
Comment #36
alexpottBecause if a change to core breaks this we shouldn't be committing it.
Note the vital part of this is not min max testing of our components. It's the ability to do proper update testing as a user would do updates.
Comment #37
Mile23That's stubbed out in
Drupal\BuildTests\Update\UpdateTest
, which demos updating from arbitrary releases to a newer release using git, and also updating from a release tag to the current codebase (including everything patched in DrupalCI).It uses the Quickstart feature, which might not be ideal. Do you have any specs for how this update should occur?
Comment #38
jibranI totally understand both points but it doesn't mean it has to be in Drupal core codebase. We can still set up a drupal CI job(along with the test runs) for each patch in Drupal core issue queue to do the following things:
We have 7 years old issue to add these components #1451320: Evaluate Symfony2's Finder Component to simplify file handling and we also have #3029088: Replace file system component with a 3rd party library. I think adding these components here just for this patch is not right. We need a wider discussion and other things to consider like #1451320-25: Evaluate Symfony2's Finder Component to simplify file handling here.
Comment #39
heddn#3043235: [Policy, No patch] How to provide automated, in place security updates of Drupal core is an issue currently in a sandbox project. But parts will start land into drupal core over the next few releases. And in that case, we want to be able to download certain, fixed versions of Drupal and provide a mechanism to update to a newer release. We could _possibly_ work around that, but it would be ideal to run actual, in-place updates of the version of files in a project.
Comment #40
alexpott@jibran
We're not adding these components to core. We're adding them as dev requirements. This means that any project built on top of Drupal does not inherit them (well it would lovely if that was 100% true but things a very inside out in Drupal land). But the point is that we're not bringing them into core in the same way as a regular dependency which is what those issues are doing and can still do regardless of this issue.
With respect to
I don't think what being suggested is realistic. Adding matrix builds to DrupalCI is not an inconsiderable amount of work. The association has many other priorities. We have a desperate need to prove updates and do them in as real a way as possible. And as @heddn points out we other things coming in the pipeline that really really need this functionality.
If someone is willing to put the work into DrupalCI to make it work that way or has a realistic architecture that can point to the needed changes and inspire others to work on it then great but otherwise I think we should proceed here. But before someone invests the time and energy to do that I might point out that the main contributor to this issue @Mile23. @Mile23 is one of the two main current contributors to drupalci and I trust that if this was simple to implement there they might be doing just that.
Comment #41
Mile23Currently DrupalCI has
command
andcontainer_command
plugins. It's entirely possible to do all of this in either a script that is called bycontainer_command
, or as a series ofcontainer_command
directives in a drupalci.yml.So we could, conceivably, without changing DrupalCI at all, add a build phase to the current drupalci.yml which does all this stuff. It would run with every patch. It would be cumbersome but it's do-able.
The down-sides of doing that would be that if you're normalizing on using PHPUnit to run tests locally, you wouldn't be able to run these important update (or build regression) tests unless you learned how to run DrupalCI locally. We'd pretty much still have to wait for a patch run to see whether the changes broke updating.
The up-side is that someone would have to write it, and then maintain it. And I'm not sure either of those is really up-sides. Because when you write that script, you're basically re-implementing some of the build test suite I just wrote, or you're stretching the boundaries of
container_command
and DrupalCI scripting.As far as adding a build matrix or other features to DrupalCI, it has a build matrix already for environments. Just start another test for whichever PHP version or DB. It's not scriptable, but it can be set up to be automatic.
As far as whether or not to use symfony/finder or symfony/filesystem, like @alexpott says these aren't in core, per se, and guess what? If we have a build system, it can build a Drupal site that is build as
composer install --no-dev
and find out whether someone did a no-no and used symfony/finder in production code or whatever. Without it we can't, full stop.Also, we shouldn't use the library we're testing to build the SUT. That leaves bare-metal PHP and that makes the test system less maintainable.
And also also: symfony/finder, for instance, just speeds up development. That's another thing we don't have to test in this new infrastructure. However, if it's absolutely necessary that we not have rapid development on this thing, then review the patch and say, "Don't use symfony/finder." Then it will take much longer and result in twice as much code to maintain.
As far as development on the testbot, it seems to be mostly me and @mixologic, but really mostly @mixologic. So if you can clone @mixologic and get the clone on the task of telling me what's his priority, then maybe there's enough time in the day. Also I'm currently looking for work and so therefore the testbot gets low priority.
Comment #42
Mile23Reroll and updated the core min/max test after #3008870: Drop support for PHPUnit 4.8 once PHP 5 is no longer supported (8.8.x)
Dunno how I missed @heddn's earlier comment in #39. #3043235-21: [Policy, No patch] How to provide automated, in place security updates of Drupal core looks like a pretty solid endorsement.
Combined with #2984031: Create Build Tests For Composer and Drupal this means that in addition to the existing update needs, two important initiatives also need a way to do this. Both should be scoped for running in a patch evaluation context. It's much easier to snap together some Symfony components with Mink to accomplish this than to add more infrastructure to DrupalCI, and the tests which use this framework would have a better DX than a private build or a shell script.
This would give us velocity on both Automatic Updates and Composer initiatives so maybe we'll accomplish one or both before 8.8.0 is released.
Comment #44
MixologicI'd chime in here that there is also an issue of boundaries and responsibilities.
DrupalCI has four primary responsibilities:
1. Construct an environment to test in
2. Build a system to be tested
3. Execute provided tests, and capture the results
4. Provide a UI to examine the results
This may seem like we're moving some of the "build a system to be tested" into core, but it really isn't. This gives core the capability to define a category of tests to prove installing/updating/building under a variety of scenarios works as expected. The boundary here is that drupalci cannot have the burden of defining the scenarios. Those must happen in core, and be defined by core developers. I have no way of knowing what is important to test.
And as far as I understand, this test type is intended to be a specific test, like "build the site on 8.6.6, and then upgrade it to 8.6.7 using composer and check a couple things to prove that succeeded" Vs "build the site on 8.6.6, upgrade it in place to 8.6.7, and then *run the whole test suite against the newly upgraded site*". That could be a very thorough, comprehensive exercise, but Im not sure thats the scope here.
If we're talking about the latter (running the existing test suite against a variety of scenarios), then we'll need to be *very* sure about how/what/when we do those kinds of tests, because that sounds like a lot more testing, and perhaps a lot of it that is not useful except in very specific circumstances.
Comment #45
Mile23Yes, exactly.
The reason some of these demo tests run tests subsequently is to prove that they worked. They're also demos to prove the concept of a building a thing and then proving it works.
Again: The thing to review here is the
_framework
patches, not the full one.If anyone has any suggestions about how to organize this better or specific use cases that are not addressed, please offer it so we can have some forward motion.
Comment #46
Mile23This patch changes the way
copyCodebase()
works so that you can inject whatever iterator you please. By default it excludes site-specific files.Because of this change, I'm including both the framework patch and the full proof-of-concept patch. In the future it'll just be the framework.
The patch also adds some tests to the framework.
Anyone please chime in with some suggestions or next steps. Thanks.
Comment #49
Mile23Some experimentation tells me that we only need to tell the PHP HTTP server to sleep for 5 seconds by default when it starts up.
This patch:
BuildTestInterface
docblock docs.getMink()
to the interface sinceassertRequest()
tells you to look at the Mink object.Comment #50
MixologicQuestion: when we discovered that using symfony filesystem /finder could be replaced with newer versions because of a newer composer, we opted to use composer's api's for interacting with the filesystem. Is there any similar risks here?
Comment #51
Mile23I think you're referring to #2982684-177: Add a composer scaffolding plugin to core which, yah, bad. That's referring to the version baked into the composer.phar file you run at the command line.
These tests don't operate under the context of a Composer plugin, so we're good.
The only caveat I can think of is if someone runs their tests with a command like
composer run-script tests
.Comment #52
Mile23Comment #53
Mile23I must have been trying to apply the patch to 8.7.x or something when I added that tag, because it applies now.
Comment #54
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedLGTM, just a few minor issues.
@todo requires a follow-up issue (or just implement something reasonable).
I used to use
netstat -an
for this. See wait-for-webserver script. Is netstat available on drupal-ci?Needs follow-up issue
On Windows, use `where`. See ExecTrait in Drush.
Comment #55
MixologicNetcat is what we use on the db containers:
while ! netcat -vz localhost 3306; do sleep 1; done
I can make sure netcat is installed on the php containers as well if we need that. (assuming this is running *inside* a container)
Comment #56
heddnLet's first reroll this thing.
Comment #57
heddnComment #58
heddnThis picks up #54.3 and 54.4.
I'd argue for 54.1 we should remove the TODO as
sys_get_temp_dir
seems like a fine default location.#54.2 could be a follow-up me thinks. Is
netstat
available on windows?Comment #59
heddnLooking into 54.2 right now. Found https://symfony.com/doc/current/components/process.html#running-processe.... Might be able to look for the results:
Let's test it out.
Comment #60
heddnSeems we have an older symfony component. But there seems to be a comparable means. Here's a test.
Comment #62
heddnComment #63
MixologicRe #58 these will run in parallel, so we need to make sure whatever working dirs are out there share nothing with any other simultaneously running test.
Comment #64
Mile23Workspace directories per process have a hashed dir name. The idea with the todo was that if drupalci tells us an environmental variable for the temp directory, we should use that. Leaving it alone right now is probably OK, though, because I think this issue needs more consensus before we fill in too many details.
Comment #66
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedIf drupalci would just make one temp directory for every test, and configure
sys_get_temp_dir()
to return it, it would make it much easier to avoid contention between different tests.Comment #67
heddnThe php process seems to be special and know when it is attached to a tty terminal or not. But I think we have all the building blocks already. Let's try this on for size. It passes on local.
Comment #68
MixologicRe #66 -> that would work, but we should do that in run-tests.sh, not drupalci.
Comment #69
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented#68: Good idea. Here's an implementation that might suffice. Feel free to continue from #67 if this is out of scope for this issue.
Do we have a better alternative than adding simpletest_rmdir() here? I wasn't sure if it was safe / prudent to use Symfony components in run_tests.sh.
Comment #70
heddnHere's some additional test coverage and demonstrates a more typical test scenario (along my thinking) of how to use this new build type.
Oops, cross post w/ #69. I'll merge them next.
Comment #71
heddnComment #72
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedSyntax errors? Bah.
Comment #75
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedmicrotime() has a space in it, so we'll use microtime(true) instead.
Comment #77
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedMissing a space.
Comment #78
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented... and setting the status.
Comment #80
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedCode style and minor cleanup.
Comment #82
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedBuild failing with:
Did not search to find out where that's coming from.
Comment #83
heddnComment #85
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented@heddn If you want to back out my addition so that the tests pass, that would be fine. I'm not sure why making a dedicated temporary directory interfered with asset collection. Maybe I deleted the temporary directory too soon. Perhaps someone else can help with this.
Comment #86
heddnThe lates failures seem to be from my test additions. I'm limiting the testing scope in drupalci.yml since otherwise takes a really long time when we can be pretty sure its just this new test type we want to turn green.
Comment #88
heddnFewer failures. And some minor refactoring for DX.
Comment #90
heddnComment #92
heddnComment #94
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedSet
sendmail_path = "true"
in php.ini to avoid errors sending email when installing a site. Unfortunately, this setting cannot be set viaini_set()
; it has to be done in php.ini or via -d on the commandline.Comment #95
heddnI had the wonderful help of a colleague assist here.
drush uli
has an extra enter added to it that was messing up the http request and resulting in a 404. This should fix it now.It was requesting:
http://localhost:8000/user/reset/1/1567199950/bwW9hwnekXsqOTBRWlbdrILBhT9uzBxg5DC-0Uf4Kz8/login\n
Notice the newline character "\n" in the URL at the end? It isn't easily visible in xdebug.
Comment #97
heddnre #94: switching to
executeCommand
instead. It will fix the sendmail thing and let us install on php 7.2 even w/ warnings about the installer not supporting PHP 7.2.re 96: this is about the drush uli command in the tests. I was hitting my head against a specific bug on my local for a really long time as described in that comment.
And hopefully this passes green.
Comment #98
heddnComment #99
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedUse Symfony Filesystem to remove our temp directory, since run_tests.sh is already loading the autoload file, and it is therefore available.
Clean up the 'uli' output a bit. It turns out that uli does not support output formatters, so `--format=json` is a non-starter.
Comment #100
jibranHave we considered postponing this on #2982680: Add composer-ready project templates to Drupal core? Once that issue is committed then we can install multiple version of Drupal via composer to whatever folder we want using
.drupalci.yml
and simple upgrade script in the core can update the core the latest dev, run the upgrades and tests.Comment #101
Mile23I'm not sure where this was introduced, but a big ol' -1 on this for a couple of reasons:
1) The whole point of this test framework is that we can test Drupal core's install and update systems on a brand new codebase. If we let drush do that, then it's not the test we actually want. In fact, the reason we have an HTTP server in there is so we can test update.php.
2) You've added drush as a dependency of a unit test of
BuildTestBase
, which compounds the issue in #1 above.Back in #46 there were tests of installing and updating core. I took them out so potential reviewers could concentrate on the framework instead of the test cases.
It seems like folks want the framework, so I've added those PoC tests back so we can talk about what sorts of test cases this framework should and shouldn't support.
Also moves the drush install/update test to its own test class.
Comment #102
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedYes, regarding Drush, wouldn't it be better to use
Drupal\Core\Command\InstallCommand
instead ofdrush si
? Similarly, it seems it would be worthwhile to also have an updatedb command in core.As for min/max testing, I am wondering if perhaps we should hold off on this until we have a way to run the min tests in parallel. The existing test suite already takes over an hour to run; that duration could increase substantially if we reran a substantial portion of the suite in series.
Comment #103
Mile23That's the thing: The PoC tests are PoC and not the actual tests. :-)
The one that's in the patch does in fact use the
install
command. CheckQuickStartTestBase
. And again: While it would be nice to have anupdatedb
command, the raison d'etre for this test framework is to test existing update systems in core. While it's possible that making (and testing) the command would give some tertiary benefits for people who use update.php, the point of this issue is testing existing stuff.So I'll add needs followup here, so we can go on to write actual tests based on what we land on in this framework-based issue.
The min/max testing part is somewhat contentious here and in other min/max issues, like this one: #2976407: Use drupalci.yml for composer dependency min/max testing - DO NOT COMMIT So I'm setting to needs work to remove the component and dependency regression tests. According to the log, all of the build tests took 4 minutes total, which includes re-running some unit tests with different dependencies. But adding it back can be a follow-up.
Comment #104
Mile23Removes the dependency tests and their fixtures.
Next question: Should this framework know about the database under test? That is: We probably want to install a test site using MySQL instead of the SQLite that is used by default in the quickstart site. How should that work?
Comment #105
heddnI'm fine w/ removing the additional tests. I probably just got too excited. I'm going to do one final tweak to the drush build test here, then strip them out in the next comment so we can do what @Mile23 suggests in #101. I'd really rather have the framework then have us block ourselves on bikeshedding the contents of futher tests.
Comment #106
heddnActually, the PoC tests are in really, really nice shape. It seems a shame to strip them. How about some minor tweaks? I say minor, because as always, the tests written by Paul are already in very good shape.
Comment #107
heddnSome minor comment improvements. Who else needs to review this at this point to get us to RTBC?
Comment #108
heddnThis update should happen if we re-roll this again. Or if not, on commit.
Comment #111
heddnThe first failure is an easy fix.
The failure for the site application I'm able to track down to the following error. I saw this a couple times earlier in my local testing but don't know what made it go away or if I did things differently and then it went away. For now I've marked that test as a WIP. Maybe someone else has some suggestions?
Comment #112
Mile23Re: #106 Thanks. :-) We'll have these patches as starting points when we get to actual test implementations, assuming they're needed at all.
Comment #114
heddnWell, rubber ducking this... I think the fix for the errors in #111 are pretty trivial. Patch attached.
But it still fails because we set base-url in
TestSiteInstallCommand
. Then don't really use it for what we need it, which is finding the site back again in a subsequent visit. So, fixes for that are also attached.Comment #115
heddnHere's the proper interdiff.
Comment #116
heddnComment #117
LendudeReally nice that this is green!
Most important first step I think is an update to the IS, lots of good discussions in the comments, we should see some of the conclusion in the IS. It's a little hard to determine the scope of what is being done here, with people getting excited and adding all sorts of tests (this is great btw!). But should we be aiming for something a little smaller in scope and adding more test cases later? Without the IS being up to date, its a little hard to pin down if the tests are all needed or are just cool applications of the new test framework.
Just some things I saw:
This really doesn't help me understand what this does :)
The use of protected vs private seems a bit messy. I don't see any specific logic to this. In general we make everything protected (I'm not saying this is a good thing, I'm just saying..). But here some have getters and are protected and some are private and don't have a getter and vice versa.
If it's not important that chmod works, why are we doing it in the first place?
needs a doc block
Synchronously with what? We can probably just leave the comment out.
should we test for a starting '/' too?
casting to int seems redundant, is it?
Missing doc blocks for class, methods and props
'all build tests'
unrelated change
\Symfony\Component\BrowserKit\Client::$redirects
is a private property, so this won't work right?Comment #118
Mile23Thanks, @Lendude. Working on it.
Comment #119
Mile23Updated IS. No doubt it will evolve further.
#117.2:
workspaceDir
andserverProcess
are private because you should interact withgetWorkspaceDirectory()
andstandUpServer()
/stopServer()
. This is the kind of situation where the Drupal culture is wrong. :-)#117.11: The code does manage to follow meta redirects, and
DrupalMinkClientTest
is passing... Will require some more attention.Tried to address everything else.
After the test build, we should set this to NW to look at
DrupalMinkClient
per #117.11.Comment #120
Mile23::facepalm::
Comment #122
heddnI ran into issues with chmod on my local for symlinked copies of certain files. Chmod failed, but the remove worked just fine. Do we need chmod at all? Why is it there?
Comment #123
Mile23It's there because core will chmod files and then you can't delete them.
The reason it happens to the workspace directory instead of just sites/ is because we allow the test to say it has a working_dir within the workspace directory, so there could be an infinite number of sites/ directories that won't get chmodded and will thus break the teardown phase.
After I fixed my own silly fs error above, I see fails like this:
So that's
vendor/bin/composer
which is an alias, which apparently causes$fs->chmod()
to freak out.We can't say "chmod everything except vendor/" because of the working_dir concept above. vendor could be inside any working directory.
So here's the fixed version of #120 and also the fixed version without the chmod in tearDown(). Let's see what happens.
Comment #125
heddnchmod accepts an iterator. Could we filter out symlinks via https://symfony.com/doc/current/components/finder.html#custom-filtering and https://www.php.net/manual/en/splfileinfo.islink.php
Comment #126
heddnLet's see how it gets treated. Here we keep chmod but remove symlinks.
Comment #127
heddnThis should be:
Comment #128
heddnComment #129
Mile23+1 on the concept. Thanks, @heddn.
Comment #131
heddnI'm confused if this works.
Comment #133
Mile23Let's experimentally find out if we can't chmod sites/default/default.services.yml because it belongs to a different user.
Please never commit this. :-)
Comment #135
Mile23Derp. Let's see if we can't remove the config file because it's a different user.
Please never commit this either.
Edit: Yes, under drupalci it's a different user.
Comment #137
heddnThat last failure actually makes a lot of sense. We're using a console command to install. So it will run as the root user. However, for deleting, we don't really care about _file_ permissions. We want to alter directory permissions. Let's adjust the iterator to only find directories. Working on that.
Comment #138
heddnComment #140
heddnComment #142
heddnComment #143
heddnComment #144
heddnNot to self: check into why we have some PHP 7.4 failures in #142.
Comment #145
heddnComment #146
heddnThat seemed to fix things. Here's an interdiff back to 135.
Comment #147
heddn$process
is null in the case of this exception being thrown. This should be fixed.We need to use
.ht.router.php
instead of-t $full_docroot
.It was added for a reason and should be used for all those reasons. See https://www.drupal.org/node/2913019
Comment #148
heddnAlso the built in web server is single threaded. For many things, this is OK. But we should describe the limitations that this gives to a test. Things that result in a new HTTP request to the local server (ajax, http callbacks) will timeout and fail.
Comment #149
heddnFirst a re-roll.
Comment #150
heddnAnother item of note, the .ht.router.php file was only added in 8.5.0, we won't be able to test with earlier versions of Drupal and the build test. I think that is probably OK.
Comment #151
heddnWe typehint an optional null, but then still require it. I figured this out after dogfooding this patch. Let's clean this up and make it truly optional.
Comment #154
heddnComment #155
heddnComment #157
heddnComment #158
heddnYeah, back to green.
Comment #159
Ghost of Drupal PastThis is incredible. Wow. I am totally floored. It'd help Drupal adoption so much to know updates are much smoother. Yay! We don't dare to go 8.7 right now, the update issues in the core tracker scare us and this is why I am reviewing this: Smartsheet needs this.
I can't say I have read this throughly but I tried to be useful and not persnickety. Feel free to ignore it all.
Shouldn't these two be in require-dev?
I just saw FileSystem::getOsTemporaryDirectory being used...
That's odd. I'd expect /some/path?foo=bar. One, $_SERVER['REQUEST_URI'] starts with a slash so I'd expect something called $request_uri would similarly do so. Two, D8 by and large uses paths starting with a slash.
Yeah, just drop the slash from here.
Or some other process have started on it. I'd guess we only need to consider our own processes, do we need simple file based locking for ports?
Yeah but implode casts NULL to empty string same as concat does: https://3v4l.org/Z1rUG
https://github.com/symfony/process/blob/master/ExecutableFinder.php ?
We are hardwiring the expectation that MySQL has an autoincrement of 1. Of course that's OK but, hrm, hrm could we use \d+ here instead somehow?
Move the contents into a variable and use heredoc? Or is it just me who breaks out in hives seeing \'? It's 100% fine to say "that's a you problem".
Channeling my inner Gábor Hojtsy, do we want another \d+ here as well? :)
That's strange, /** is the standard doxygen opener.
Comment #160
Mile23Thanks for the review. Here's some easy responses while I'm not at the work machine:
3,4: Yah, either way. Starting with a slash is probably fine. Looking at our routing.yml files, I see we standardized on that, too.
5: We always manage the process. Ports are ‘locked’ by trying to use them and then failing, so we go on to the next one.
8: We’re only testing whether our mink client correctly extracts the meta for a refresh, not whether those URLs are valid or otherwise for their purpose. That URL looks like a typical batch process that should generate a refresh.
9,10: UpdateTest is a PoC test to show that the framework meets our needs. Only the tests in Drupal\BuildTests\Framework\Tests\ are actually testing the framework itself. The others will be scrapped before the commit, so we can bikeshed them in other issues. :-)
11: Tell it to NetBeans. :-)
Comment #161
catchRead through fairly quickly. No useful feedback apart from the nits below and to say this looks amazing.
Extremely minor but tense should be 'Makes...'.
Same tense issues here.
Should this include 'available_command' so it's obvious what the fail is?
Wow.
wow.
There's obviously no content in the freshly installed database, so we should have a follow-up to create nodes, terms, menu content links etc. and update with those.
This is really quite something and opens up so many possibilities.
Comment #162
heddnI'll work on the last few points.
Comment #163
heddnThis should cover the outstanding items from #159-161.
Note, I added a new assertion for confirming the site is a drupal site based on all the places we seem to have those 3 or 4 lines copied. And while doing that, it caused me to consider that we should also make
findAvailablePort
andinstantiateServer
part of the public interface. Reason is that to work around php's built-in server only allowing a single thread, we need an easy way for consumers to spin up new servers.Making it public lets us the following pattern:
Comment #164
heddnComment #165
Mile23Re: #163: That's why it's not part of the interface, specifically so you know you shouldn't do that. That way you write an assertion instead of starting up the server. We already have one process-isolated server per test class, and a test class shouldn't have multi-threading server needs. IOW: Don't write tests more complicated than that.
Or is there something I'm not getting?
Comment #166
Ghost of Drupal PastThis is absolutely lovely, thanks for the quick update! I still have the concern where two tests running on the same machine in parallel will try to use the same port. If they start at the same time, they will find the same port open, attempt to start a server and one of them will succeed and both of them will find the port is no longer available so they will both presume its their server and then all sort of brokenness ensues. It might be I am misunderstanding the mechanism of course (I certainly misunderstood a unit test above to be functional) or it can be we no longer run tests in parallel so these concern might be moot.
Comment #167
heddnWorking on #165/166.
Comment #168
heddnQuick reverse of api additions re: #163/#315. #166 up next.
Comment #169
Mile23Re #166: Parallel HTTP happens by assigning different ports to different tests/docroots. They operate on a round-robin query against a range of port numbers. If the port is open, that means another process has already claimed it and we can't use it.
If it's a race for querying if the port is open (and thus already claimed), then it's would also be a race to find out if the port is open and file locked. Either way, if you find an available port, someone else could grab it before you do. If we only care that the port is already opened, then we can do that more quickly, and thus run less chance of a race condition problem. Using a file lock would be a time liability.
I experimented with symfony's file lock system and it was just too complicated for basically no benefit.
Concurrency on the PHP HTTP server is an important concern because we want to design this thing to run in concurrent tests from the very beginning.
We also want to hide the implementation details from the test author, because it could be any kind of HTTP server in the future, and we're really testing 'build' -> 'process' -> 'query to verify' and not 'set up HTTP server.'
Comment #170
heddnI made these changes before I read #169, but if we decide they aren't necessary they are pretty easy to strip back out.
In defence of adding these changes, I tested concurrency on local and found out that we actually _did_ have some fragile tests. Specifically
testPortMany
when run in concurrence with really any other test is an issue. I opened three terminals, ranvendor/bin/phpunit -c core core/tests/Drupal/BuildTests/Framework/Tests/BuildTestTest.php
concurrently and had random fails 100% of the time.Comment #171
Mile23Re: #170 Then by all means let's do something else. :-)
Comment #172
Ghost of Drupal PastI must admit I do not understand #169. Here's what I think happens:
I do not think we need to lock this whole thing, it's enough to add the port to the name of the lock and then there is very little time wasted. It'd go like this:
There we go.
Comment #173
Ghost of Drupal PastThis is how I imagine it. As always: feel free to drop it, no hard feelings, I just felt that itch :)
Comment #174
clemens.tolboomComment #175
clemens.tolboomComment #176
heddnFixes in #173 seem reasonable. Additional nit fixes which I comment on below are fixed in this patch.
I noticed there's also a semaphore store too, but then later noticed that Windows doesn't support it. So for posterity's sake, commenting that's why we should use flock over semaphore. Good job.
$instance
is never used.There isn't a need to reloop, just do this in the previous loop.
Comment #177
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedGiven the experience we had with temporary folder creation in the scaffold tests, I have reservations about this port-selection technique. Once this code is committed, then every test that runs is going to compete for a port on every test run. Drupal.org runs a lot of tests, and sometimes a lot of tests start up at the same time. This code creates a critical section that will commonly cause port conflicts. Every test process will look for a free port starting at 8000 and scanning up one port at a time. If two jobs start at about the same time, then they will invariably choose the same port number if the second job asks for its port number while the Process above is launching.
I have two recommendations. To avoid having one test use the port for a web server started for a different test, we need to be able to detect errors from the `start()` method, and pick a new port if it fails. This implies that port selection should be postponed until the web server is launched (in this method).
Always starting with the same port number and always scanning up one port at a time guarantees that every job that starts at about the same time will pick the same open port.
To make the pigeon-hole-principal a bit better:
Use a range from 8100-8799. (The port numbers you use on CI do not matter, but it's good to avoid 8000, 8080 and 8888 when testing local. This matters 1/10, pick another, wider range if you prefer, the wider the better.)
Do not always start at the same place in the range. Pick a random number such that the first port checked may be anywhere between the beginning and end of the range.
If you still always go up just 1 port at a time when scanning, just the changes above will give greatly improved results. However, if a bunch of consecutive ports happen to be selected by chance, scanning by 1 each time makes it more and more likely that any given clump will grow by one. To avoid this problem, you could pick a small random prime, and skip that many port numbers each time you retry. Use a modulo to keep the trial port inside the range. Optional. Just using a larger range (way up into the 30,000s?) is probably even better in terms of impact than this improvement.
You cannot avoid the pigeonhole principal, but these techniques will increase the utility of the range available.
Comment #178
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedNevermind about #177, this was already taken care of elsewhere in the code in #173. I think the current technique should work.
Comment #179
catchCould we not just randomise the port selection, if we're unlucky exclude the port we just checked then go for another random port? Even if there's not a race condition in the current code, it might be more efficient. Right now to run at 16 concurrency we'd always have to run through up to 15 ports before finding one.
Comment #180
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedYeah, randomizing the port selection was my first suggestion. The part you quoted was my third suggestion, which reduces clumping when selecting a lot of ports in a narrow number space. As I also said, just increasing the range available for ports is probably more effective than varying the increment size. The main win is with randomizing the port selection.
Since the current patch is using a lock file to mark ports as used, rather than attempting to launch the browser as I first assumed, randomizing the initial port selection is less important. It would still be an improvement, saving us from quite a few attempts to grab a lock each run. Trying to grab an in-use lock is probably fast, but we could still make this improvement.
Comment #181
Ghost of Drupal PastI was wondering how mt_rand() will behave in two processes starting the same time and decided to not wonder, we are PHP 7.0 so we can just
random_bytes()
to find a random port. I dropped min and max and just pick a random number between 1025 and 65535.random_bytes()
is not the friendliest of API for this purpose but it could be worse. I measured hexdec(bin2hex()) vs unpack('n', )[1] and the latter is faster (it is faster than bindec even but bindec doesn't seem to work for this purpose, php is murky when it comes to binary data). I kept the locking though.Comment #183
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedGetting really close!
Teardown does not appear to release our locks; in fact, it seems that we never release any of our locks, unless we fail to acquire a port. It seems that the easiest thing to do would be to make findAvailablePort() add to a list of all acquired locks, and release them all in teardown. Otherwise, there's a chance we might run out of locks. Probably not a big chance; I presume that locks are all released when the phpunit process terminates.
If this is going to ever do a re-install, we'd also need to chmod settings.php, and maybe also copy default.settings.php over settings.php. Perhaps in this instance, a re-install is not possible, and chmoding the sites/default directory is not necessary. I think I'd favor being more conservative, and do all of the re-install steps. Maybe make a method to do this cleanup in case it's needed in other tests.
I didn't look into the test failures, but perhaps the range of ports scanned is now wider than supported by the drupalci infrastructure.
Comment #184
MixologicChecking to see if the webserver is available by determining that its port is *not* available is why the tests failed. findAvailablePort should either only pick the random number if its not passed in, or we should abstract out the fsockopen to an internal method of "is port open" etc.
This part looks suspiciously like the question asked in this stack overflow post: https://stackoverflow.com/questions/39657564/check-if-anything-is-listen...We should be checking if a resource is returned from fsockopen.
Nevermind. FALSE is the correct inversion here.
Comment #185
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedYes, overloading
findAvailablePort()
to both find a port and determine if a port is available is confusing. Let's not confuse it further by inhibiting the random port selection; instead, let's take the second alternative suggested in #184 and factor out a function just to check to see if a port is in use.Comment #186
Ghost of Drupal PastThe tests failed because of this expectation
$this->assertEquals($this->findAvailablePort(), $this->findAvailablePort());
which obviously no longer happens. And this was not just an assert, the server init routine relied on subsequent calls to findAvailablePort returning the same port and busywaited for this port to change. Even without any random that can fail:instantiateServer
returns before PHP server is ready.I removed that assert and with it the entire test -- I split findAvailablePort into findAvailablePort and checkAvailablePort and added an assert for the latter into the remaining testManyPort. I also removed the busywaiting. Interdiff is against the last passing test, previous attempt was not great.
Comment #187
Ghost of Drupal PastNote #183 is not addresed yet (and it's unlikely I will do it).
Comment #188
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedI don't think that #183.2 is necessary. I'll do #183.1 later.
Comment #189
Ghost of Drupal PastSorry, I meant #183.2 , my initial writeup got lost :( #183.1 is not necessary,
https://symfony.com/doc/current/components/lock.html
If you deem #183.2 not necessarily then we might be good to go if the test is green.
Comment #190
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedOh, cool!
I think this is, in fact, good to go. I only contributed a few lines to this very large patch, so I think it's fair for me to RTBC it. A lot of review comments from a lot of reviewers have been addressed here. Good work.
Comment #191
jibranThese tests consist of a bunch of bash commands. I'm failing to understand why these can't just be converted to bash scripts and run directly on drupalci using .drupalci.yml?
Why are we writing a whole new testing suite to achieve the task which can be completed by writing some ci commands?
Comment #192
MixologicThe bash functionality of drupalci.yml is intended to do minor setup work. It is *not* intended to be a bash testing framework or anything resembling that. Please do not place bash scripts into drupalci.yml and then rely on the exit codes of those scripts as evidence of build success or failure.
It is preferable to have these bash scripts execute within the context of phpunit, which frames them as a standard test, gives us the results in a standard way, and is runnable in environments *other than* drupalci.
Additionally, we want any developer to be able to write and execute these tests locally, and running phpunit is typical. Setting up a local drupalci, is not very typical.
That being said, I think this is still NW because of the issue summary:
Comment #193
Mile23+1 on #192, but also..
We can't use bash to make assertions like we can with Mink. So we could use bash to build out a site, but then when it came time to make a request against a web server we'd have to build that in bash, as well. Then when we needed to assert that, for instance, we got the right headers from an HTTP request, we'd have to add bash to curl or wget or something to do that. And so on.
Making a minimal build system in PHP is preferable.
Added a child issue: #3082230: [meta] Convert some tests to Build tests
And here's a patch without the proof of concept tests. :-)
Comment #194
Mile23Def needs change record, and a documentation page.
Comment #195
Ghost of Drupal PastI turned to Stackoverflow because that random_bytes line bothered me. Here's a better one. We all learn every day: I didn't know about random_int().
Comment #196
heddnI've added some WIP things for docs: https://www.drupal.org/docs/8/phpunit/build-testing and https://www.drupal.org/node/3082383
Comment #197
heddnOK, those docs are now populated and can be reviewed along with the patch here.
Comment #198
alexpottHere are some nits. First step on a larger review.
Double ;
* @see \Drupal\BuildTests\Framework\BuildTestBase
- the patch is missing as
Should be
* @see \Drupal\BuildTests\Framework\DrupalMinkClient::followMetaRefresh()
- missing a leading\
I think this might be not doing what we think it is because
$this->redirects
is a private on abstract class this extends. See https://3v4l.org/RJsbRThis was brought up before in #117 by @lendude but it does seem to have been adequately resolved beyond "it's working".
I think this is unused.
expectations
Comment #199
heddnI'll leave fixing 198.4 to @Mile23 or some else. I briefly looked at it and had some ideas, but it involved copying more of https://github.com/symfony/symfony/blob/4.4/src/Symfony/Component/Browse... into our implementation, I couldn't come up with a lot of options.
I also don't feel that having meta refresh features for the first rev is critical and could be a follow-up and we could debate how to solve it in that issue.
Comment #200
alexpottsymfony/lock is a new dependency and as such need release manager +1 and needs a dependency evaluation - see https://www.drupal.org/core/dependencies
symfony/filesystem and symfony/finder are new direct dependencies but are already included because they are amongst our dependencies dependencies.
rand() vs random_int() ... maybe we should swap to rand in ::findAvailablePort() because cryptographic security is not a concern at all here.
We shouldn't be making assertions in teardown and setup either.
I don't think this this method should be making assertions... for the same reason as below.
Making assertions in the base class means that tests which don't make any assertions will be counted as risky. I think we should avoid them. We can check for the file existing and throw an exception if it doesn't instead.
Is there any compelling reason to create this interface? It feels really odd to create an interface here. The idea of implementing interface for our different test types feels odd. It's not like they are swappable.
This has the same thing as the visit asserts. It's executing the command and then making an assertion. I'm not sure that this is good practice. I would more expact to see code like
All these methods result in the visiting a Url. The assertVisit methods are interesting because they are not *just* asserts... they are visit a url and then make assertions. This is not usual behaviour for assertions. Assertions never normally change the state of the system whereas these do. I also find all three of these living on the same interface interesting and a bit confusing.
It's a shame that PHPUnit doesn't have a more generic way of adding a new type of @requires. One thing that gives me pause here is that adding this means we are going to have to support different versions of this in #2950132: Support PHPUnit 7 optionally in Drupal 8, while keeping support for ^6.5 as undoubtedly ::checkRequirements() has got a return type. And yep it does - see https://github.com/sebastianbergmann/phpunit/blob/dfad4702af08b4d706dbc5... but what's more it has become private so our override won't work when we upgrade. :(
I think we should consider baking this functionality into BuildTestBase::setUp() and not worrying about re-usability.
I can't see where composer, git or drush is actually used by this test.
Feels odd to be changing existing tests - howe come that's necessary.
I don't understand why we have to remove existing coverage here. We're adding a new test type and sure we might want to replace these tests in the future but why are we removing them now?
Comment #201
Ghost of Drupal Past> rand() vs random_int() ... maybe we should swap to rand in ::findAvailablePort() because cryptographic security is not a concern at all here.
It shouldn't matter now -- as I mentioned above I just didn't want to figure out whether mt_rand behaves pathologically when running in parallel but now that we lock, how we pick a candidate doesn't matter. We could've stayed with the sequence. It's just faster to use a random value, any random, because there's much less to lock this way. If we change from random_int then it should be to mt_rand() because we want larger than 32767 from rand(). Also mt_rand is slightly faster than rand and significantly faster than random_int, so let's do it. I won't roll a new one with that one change, there's enough to do and it's trivial to change with the others.
Comment #202
Mile23#200.6:
I added that because if there's an interface you don't have to mark other methods as @internal. Also you have to argue about whether a given method belongs in the interface or not, so you have an extra check on bloaty bloatness.
Some of the class properties are private, as well, for similar reason: They're implementation details, and test authors should just be writing commands and assertions and not petitioning for a new convenience command.
This can all change around as needed... I'm not married to it. And hey, I got to argue it over this one time so mission accomplished.
7 & 8: In order to make an API like this:
... you have to manage the state of the process within the class. Instead of doing that, we return the process object to the caller, and they can assert whatever they want against it.
or...
This is much clearer to me, and also makes the class less complex because you're not keeping track of state.
We could remove
assertCommand()
and have the user always sayexecuteCommand()
and then make an assertion against the process object about exit code or whatever they want to. But if we do that then we lose the benefit of uniform reporting on fails, which everyone has to recreate in order to debug their tests anyway.The same pattern applies to
assertVisit()
. We do manage the state of Mink in the class, but really that's just using the Mink API.I'm more married to this than 6 above. :-)
9: Sigh. This is why we can't have nice things.
Comment #203
Mile23#200.1: Want to make this framework a component? :-) Then it and symfony/lock could be removed with —no-dev.
#200.3,4:
$fs->mkdir()
and$fs->remove()
throw an exception if there’s an error, so we just remove the assertions.#200.5: I think we don’t need to assert the existence of .ht.router.php at the framework level for a few reasons:
- Not all versions of core have this file.
- Assuming its existence is a Drupalism. Corollary: Not all HTTP requests will be against a Drupal site.
- We’re trying to test against the build and update processes for core, and if the installation is missing that file, then the bug mentioned in https://git.drupalcode.org/project/drupal/blob/8.8.x/.ht.router.php#L18 should be seen in the test. Maybe add a note on #3082230: [meta] Convert some tests to Build tests that specific test implementations need to assert the existence of .ht.router.php and/or visit a route with a ‘.’ in them to check this.
That assertion was added in #147 so how about it @heddn?
Anyway, removed the assertion from
setUp()
.#200.8: I’m not a fan of
assertDrupalVisit()
, because we might change our generator string. In fact, Drupal 9 will change it. RemovingassertDupalVisit()
here, but open to putting it back if someone has a way.The rest is the same pattern as the other
doSomething()
/assertDoSomething()
methods. What if we just take the word ‘assert’ out of the method name? So for instanceassertCommandErrorOutputContains()
becomescommandErrorOutputContains()
.Also simplified some of those assertions in
assertVisit()
andvisit()
. We get an exception if the server wasn’t able to be set up, so that should error out the test.#200.11,12: Reverted.
Still outstanding: #198.4/#117.11, #200.1, and #200.9.
Comment #204
Mile23#200.9:
Drupal\KernelTests\Core\Image\ToolkitGdTest
will need some love in this regard, and probably others. #2950132: Support PHPUnit 7 optionally in Drupal 8, while keeping support for ^6.5 doesn’t seem to cover this.It turns out we need
ExternalCommandRequirementsTrait
to be in a trait so that we can test it. Since we’re testing annotations, we can’t mock a class, and if we annotate aBuildTestBase
class, it’s discovered by the testing system. So we make it a trait and then test the trait methods on a non-test class. Let’s hear it for composition!Still outstanding: #198.4/#117.11 and #200.1.
Comment #205
Mile23Interdiff....
Comment #206
Ghost of Drupal PastAddressing 200.1 the only concern I can see https://github.com/symfony/lock/commit/a48ab1443c39e8fc6950a4edc2a16c383... is this very recent change, otherwise Lock -- at least as far as I can see -- is a very stable and problem free component, especially what little we use of it. https://github.com/symfony/lock/blob/master/CHANGELOG.md
If we added a use statement for LockFactory and did a class_exists(LockFactory) check to decide to call
new Factory
ornew LockFactory
(they are the exact same class, just renamed), we would be compatible up to and including Symfony 5.0. (use statements for nonexisting classes are fine)Comment #207
Mile23#198.4/#117.11:
In an ideal world, Drupal 8.8 would require PHP 7.1.x. I say this because it'd be lovely to just say that we require-dev symfony/browser-kit ^4.2 and then be done. :-)
But Drupal 8.8 needs to run on PHP 7.0.8, so here's
DrupalMinkClient
with reflection to access a private property in order to get meta refresh redirects from Mink.Also we check if symfony/browser-kit supports meta refreshes, and use that instead if we can. I manually tested this by having composer.json require symfony/browser-kit ~4.2 and then updating.
Still outstanding: #200.1, tho checkout #206.
Comment #208
alexpottHow come this is static and why does is not private?
If we're using privates - which I think makes heaps of sense on abstract testbase classes - why this is not private? There's a public accessor.
There's a getter... so making this private would prevent interesting side effects.
I'm not sure this needs to be a class property.
I'd be tempted to make these private too because you've got public methods to get a lock etc and what do tests extending this need to do with this?
Why is everything static in this class? Is it to indicate this trait has no side effects on $this? Also should we consider making all the methods and property private? Tests that extends BuildTestBase have no business futzing with this.
Comment #209
Mile23Thanks for the review, @alexpott.
OK. All
BuildTestBase
properties other than$destroyBuild
are now private, which is totally fine with me.A tiny bit of refactoring for
instantiateServer()
aroundstatic::$hostName
, because a test was using it.Removed the interface, moved docblocks to
BuildTestBase
.Why is everything in
ExternalCommandRequirementsTrait
static? Because of this:This means that all the methods other than
checkMethodCommandRequirements()
need to be static. And it’s a coin toss as to whethercheckMethodCommandRequirements()
should also be static. It only needs the method annotations from the concrete class.I haven’t refactored the assertions to not change state because I have a place to be in a few minutes, will get to that. Any comments welcome in the meantime.
Still outstanding: #200.1, #208.1
Update: Forgot to re-delete the interface after copying all the docblocks from it. So still outstanding: #208.2.
Comment #210
heddnre: removal of
assertDrupalVisit
we had 3 or 4 instances of those same 3 assertions. So having a helper that does it so we don't constantly do it is a nice-to-have. Especially as it does a version agnostic regex (based off earlier feedback).Removing .ht.router.php check is fine. Not having it does lead to very strange and difficult to troubleshoot test scenarios, so we want to make sure the notes/docs spell out those limitations well.
As far as assert vs doSomething, there's been a couple times I wanted to check against error response, non-error response and other things. We have this same feature w/ mink and
getSession
. I think I'm more in favor ofvisit
, followed by assertions on the last visit.Comment #211
Mile23OK, so now you say
executeCommand()
and then call the assertion helper methods. You can also say$process = executeCommand()
and then assert against$process
.Similarly, you say
$this->visit('/path', '/working/dir')
and get back a Mink object. You can assert against the Mink object, or use$this->getMink()
. There's still a caveat in that when you make a request against a different$working_dir
, you have to get a new local Mink session. There's not a good solution to this.Added back
assertDrupalVisit()
which only acts against the existing Mink object.Normalized on
$working_dir
so it's clear what's going on.Re-added the PoC test dealing with quick start, in order to demo how these work.
For commands you can say this:
For Mink assertions you can say this:
Still outstanding: #200.1
Comment #212
heddnThis is looking very nice. Paul, thanks for pushing it forward some more.
Do we need to do #206? Or is that follow-up material?
Comment #213
catchJust to say from a release management perspective adding symfony/lock as a dev dependency seems fine. Obviously if we ever want to use it outside dev we'd have to separately evaluate it.
Comment #214
heddnComment #215
Mile23OK, so here we are without the PoC tests, ready for a review.
Comment #216
Mile23Per Slack conversation w/ @alexpott... Let's try reverting
SimpletestResultsForm
. I'm pretty sure those changes are left over from the very beginning of this process when stuff was breaking, and from running under run-tests.sh to make sure everything worked.Comment #217
alexpottI ponder if we should only be doing this in concurrency > 1. One reason for this is that it makes run-tests.sh and therefore DrupalCI a bit different from running the tests via PHPUnit's runner which is a concern. Also are we sure that we're not going to hit a problem where
rand(10000,99999) . microtime(TRUE);
is not unique? Looks like a random fail waiting to happen.I wonder if we should use
\Drupal::service('file_system')->deleteRecursive($test_directory, ['\Drupal\Tests\BrowserTestBase', 'filePreDeleteCallback']);
here instead.One we're doing this already later on in this method and two the filePreDeleteCallback is obviously borne from experience.
Comment #218
alexpottThe changes been discussed in #217 came from #66 and were implemented in #69.
Given we're doing this - do we need the temp dir stuff in run-tests.sh? I would have thought this is a one or the other situation. I think I prefer the solution being inside BuildTestBase as that keeps everything runner agnostic.
Comment #219
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedRevert #66 / #69
Comment #220
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedIt would probably be more reliable to use something like:
$this->workspaceDir = $fs->tempnam(DrupalFilesystem::getOsTemporaryDirectory(), '/build_workspace_' . md5($this->getName() . microtime(TRUE)));
Is there some reason we use executeCommand here rather than assertFalse on fileExists / is_file?
Comment #221
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedUse
$fs->tempnam
to ensure that the name we pick does not already exist on disk.Avoid asserting on
exec test
when we can instead assert onfile_exists
.Comment #222
alexpottI think tempnam generates a file and not a directory and also with the patch we're making the dir before doing $fs->tempnam().
If this is the right change then we should use $this->assertFileNotExists() - when reviewing this before I wondered if this was done on purpose because it seems overly complex.
Comment #223
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedArgh I flubbed that last patch. I'll fix it up.
I think that maybe the point of the second test was only to confirm that executeCommand returns a non-zero status code when an error happens. That is, we don't care that composer.json does not exist, it's executeCommand that we're testing.
I'll clarify this as well.
Comment #225
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedHopefully un-flubbed.
Comment #226
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedCoding standards.
Comment #227
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedSince my main contribution here is backing out my previous contribution and some coding standards changes, I think it's okay for me to RTBC. Reviewed, and looks good.
Comment #228
Mile23Still needs #222.
Comment #229
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented@Mile23: I already did #222.1 by calling remove and mkdir after tempnam. I did #222.2 by changing the test. My assumption is that what we want to test here is the 'exec' wrapper function. Testing to see whether composer.json did or did not exist seems superfluous, as composer.json does not seem to be relevant to the rest of the test functions. If I've misjudged, then we should perhaps add in the assertFileNotExists test, or maybe just put the test back the way it was with a more clear comment.
Comment #230
alexpottStill not a huge fan of this. I think the current construction can allow for race conditions between the remove and the mkdir - if another process does tempnam as that point. Also
$this->getName()
only returns the test method so I would haver thought we needget_class()
to make it more unique.I propose that we do this the same way we are guaranteeing port uniqueness - we have a locking system there for us to use. So we can do something like:
And then in teardown we can do
$this->lockRelease(basename($this->workspaceDir));
when $this->destroyBuild is true.Comment #231
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedThe current implementation is a bit ironic, as the conditions for failing during the race condition is that the same code is running at exactly the same time (two processes going through tempnam, remove, mkdir at the same time), and this is guarded by the md5 of two entities that will always be the same when the same code runs at the same time. This assumes that less than one microsecond passes between the call to microtime() and when remove() deletes the temp file. If this never happens, then there will never be a conflict, but let's pretend this will always happen (both calls in the same microsecond), since we cannot prove that it will never happen.
I don't really like the lock solution here; even though it is code that we have available, it is more complicated than necessary. With a slight adjustment to the code, we can use tempnam as the lock. This gets rid of the
while
, and also obviates the need to add delayed cleanup for the lock. I'll post a new patch that demonstrates this in a few minutes.Comment #232
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedI changed my mind. My solution still requires deferred cleanup, which would mean adding another class variable to store the temporary file. In the balance, maybe adding more state variables is worse than having the
while
. I'll do it as you suggested.Comment #233
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedUpdated patch with suggested solution from #230.
Comment #235
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedTest failure due to a spurious timeout connecting to Packagist. Requeued.
Comment #236
heddnOnce a build is finished, I wonder if we should always release the lock. Is there any way to ever release these locks if we preserve the build? I'd be worried that these locks could start to pile up.
Comment #237
heddnThis being private causes issues with class inheritance, due to the way traits operate at runtime. Unless we want to require all implementing classes to also use the trait directly, we should make this protected.
Otherwise we get:
Comment #238
heddnIn #210 I said that checking for
.ht.router.php
didn't need to be done. However, I'm not so sure I'm so cool with that any more. If it is available, then I feel we should use it. I have to be very careful with the URLs I visit. If they include any periods in them, then the test fails.Comment #239
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented#236: Locks are automatically released when the instance is destroyed, so I'm not even sure we need to explicitly release it in teardown.
#237: There are only problems with accessing private members if you try to do that directly, without a wrapper method. Private fields behave as you would expect them to if you provide methods for manipulating $existingCommands. We should do this rather than making this field protected.
#238: I really think this should be a follow-on issue. I think it is important to get this to a committable state, and then worry about refinements. We can just document limitations in visited URLs.
Comment #240
Mile23PHPUnit test objects are held for quite a while in the result object, like all the way to the end of the testing process. Under run-tests.sh this isn't such a big deal because we do one test class per process, but under phpunit tool it could be a problem. Also, it's just better style to explicitly clean up.
+1. In fact, here's a new child issue: #3085728: Add access rules for build tests
Comment #241
Mile23OK, Re: #220:
That test is left over from early dev times when I was checking to make sure the workspace directory was correct.
Now, fixing it, I realize that it's probably a good idea to always make sure the workspace + working_dir exists (otherwise symfony
Process
tells us it's usingcwd()
, which we don't want).Comment #242
heddnThis addresses #237.
So, one could argue that we should have a test for the use of self vs static... but we do have follow-ups and this is easily caught with them. So I'm suggesting we just wait for those to test this. I'm fine w/ follow-ups for the rest of the feedback. LGTM.
Comment #243
Mile23Edit: Comment made in error.
Comment #244
Mile23Comment #245
MixologicSince this is adding a new test type, and has no potential to interfere with any existing installations, I think its time we got this in, and start exercising it so we can iterate on any additional polish.
Both automatic updates and composer initiative could strongly benefit from this, so, lets not let perfect be the enemy of the good, and if there are more things needed, we can add more issues to shine it up.
Comment #246
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented+1 #245
Comment #247
alexpottCommitted 8524be362e and pushed to 8.8.x. Thanks!
Fixed unused use on commit.
Comment #249
Mile23!
Thanks everyone. :-)
Comment #250
Mile23CR has out of date demo code. I'll update later today if no one else gets to it.
Next step: #3085728: Add access rules for build tests
Comment #251
Mile23Updated CR and also docs page but the docs could probably use more work.
Docs: https://www.drupal.org/docs/8/phpunit/build-testing
Comment #252
Krzysztof DomańskiNew test can randomly fail.
https://www.drupal.org/pift-ci-job/1426940
Comment #253
MixologicHmm. there's four other instances of this.
https://www.drupal.org/pift-ci-job/1425967
https://www.drupal.org/pift-ci-job/1426830
https://www.drupal.org/pift-ci-job/1426933
https://www.drupal.org/pift-ci-job/1427002
Is this indicative that the findAvailablePort() isnt working as intended, or is it that we're assuming too much in the test itself?
Also, should we NW this and revert or file a followup?
Comment #254
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedI wouldn't want to revert this for a few spurious test failures. Let's just file a follow-up to fix the bug.
Comment #255
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedCreated follow-up: #3086179: Fix spurious failures attempting to allocate a web server in the build tests
Comment #256
Mixologicyeah, reverting is rough because randofails are so difficult to replicate, and therefore an impossible bar to know if you have actually fixed it or not.
My main concern was whether or not the *timing* is bad, with people trying to get stuff into core. But given that it happened about 5 times in a couple of days, Im thinking the drag on velocity isnt nearly as bad as the difficulty of getting to the root of this without being able to see that its not happening anymore.
Comment #257
alexpott@greg.1.anderson something odd happened... #233 interdiff was not actually applied to the patch in that comment. So that code is not in HEAD. Going to create a follow-up for that - see #3086235: Ensure BuildTestBase workspace directories are unique
Comment #258
mikelutzAs long as we are following up, this appears to introduce a dependency on symfony/browser-kit, but does not declare the dependency. It's currently only installed becuase it's required y behat/mink-browser-kit-driver.
It's also getting installed at version 4.3 on php 7.1 with a composer update, but that adds string typehinting to the request method which we aren't compatible with pre-php 7.2
Can we explicitly require-dev symfony/browser-kit: ^3.4.0 (and we should be fine with type-hinting when we update to 4.3 on Drupal 9 with a minimum php version of 7.2)
Comment #259
mondrake+1 on #258 - that’s what also #3044175: [DO NOT COMMIT] Vendor minimum testing issue is doing
Comment #260
heddnOpened #3086332: Explicitly require-dev symfony/browser-kit: 3.4.0 for #258.
Comment #261
xjmThis is currently tagged for the 8.8.0 release notes, but it sounds like a (massively useful) non-disruptive API addition. If there's a disruption, the release note should briefly summarize it. If it's just something we want to celebrate, let's tag it "8.8.0 highlights" instead.
Comment #262
heddnNon-disruptive. Retagging.
Comment #263
xjmThanks @heddn!