Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
As a starting point of #771430: Add test bot running IIS + Windows, we need to be able to run tests in parallel on Windows. Windows doesn't support the PCNTL extension that we use to fork test processes, but fortunately we can rewrite the test spawner to launch separate processes instead of forking.
Comments
Comment #1
Damien Tournoud CreditAttribution: Damien Tournoud commentedLet's try this.
Comment #2
Damien Tournoud CreditAttribution: Damien Tournoud commentedSlightly improved version, that can now catch fatal error in the test runners and properly report them. Basically the same idea.
This also fixes a stability issue we have with some slow test clients on the testing infrastructure: on the old version of run-tests.sh, all the tests were run in a fork of the parent process, and as a consequence inherited all the constants, including REQUEST_TIME. The flood tests fail because of that as soon as the test run last more then an hour.
Comment #3
rfaySubscribe. I'll try to give this a run.
Comment #4
Damien Tournoud CreditAttribution: Damien Tournoud commentedRunning tests on Windows now fail because we hit the limit of the command line length when forking the child runner:
This patch changes the way we fork the runners, and fixes this issue.
As a consequence, it just got promoted from a feature request to a critical bug.
Comment #5
ksenzeeI'll try to get my Windows VM set up to run tests, so I can try this out.
Comment #6
ksenzeeHm. I can run individual tests on Windows from the command line (using --class), but I can't use the --all option - and that's true both with and without the patch. I'm probably making some newb Windows mistake. Here's what I get:
-------------------------
C:\xampp\xampp\htdocs\drupal7>\xampp\xampp\php\php.exe scripts\run-tests.sh --url http://localhost/drupal7 --php \xampp\xampp\php\php.exe --all
Drupal test run
---------------
All tests will run.
Test run started: Wednesday, August 11, 2010 - 11:49
Test summary:
-------------
Test run duration: 0 sec
Comment #7
aspilicious CreditAttribution: aspilicious commenteddid you edit the php timeout settings?
Comment #8
ksenzeeI did. But I doubt it would have mattered - it really was taking 0 sec for the script to exit. It didn't even try hard.
Comment #9
boombatower CreditAttribution: boombatower commentedAccording to how we have marked other issues...anything that blocks testing is considered major not critical since it doesn't block you from running Drupal.
Comment #10
attiks CreditAttribution: attiks commentedI tried running the test as well, but php scripts\run-tests.sh --url http://localhost --all didn't work (same as #6), for the moment not sure why, so I wrote a batch script that runs all test per category, it has taken a while (6.5 hours) but it works.
BTW this was without using the patch in #2
FYI: test results included
Comment #11
attiks CreditAttribution: attiks commentedTesting as we speak witrh the patched version, the --all parameters is working, to be continued
Comment #12
attiks CreditAttribution: attiks commentedPatch in #2 works, but i had to apply by hand, so i guess it needs a reroll
All tests passed, except one: Fatal error: Call to a member function getDirectoryPath() on a non-object in E:\sites\drupal\7\567072\drupal-7.x-dev\modules\image\image.module on line 809
This was on exact the same drupal installation, so don't know what happened
Comment #13
attiks CreditAttribution: attiks commentedsee also #956230: Call to a member function getDirectoryPath(), i reran the failed test using the browser and it pass, so I suspect it's the patch?
Comment #14
sun.core CreditAttribution: sun.core commented#2: 771448-windows-concurrent-testing.patch queued for re-testing.
Comment #16
Damien Tournoud CreditAttribution: Damien Tournoud commentedReroll.
Comment #17
Damien Tournoud CreditAttribution: Damien Tournoud commentedWith a proper git prefix.
Comment #19
Damien Tournoud CreditAttribution: Damien Tournoud commentedFix a silly parse error.
Comment #20
aspilicious CreditAttribution: aspilicious commentedCan you summarize this please? And can we test this?
So without this patch I can't run any tests in windows?
(I did before...)
Confused...
Comment #21
chx CreditAttribution: chx commentedComment #22
sunRe-rolled as -p1 patch with latest code from drupaltesting.org.
22 days to next Drupal core point release.
Comment #23
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis has been running on drupaltesting.org for more then a year.
Comment #24
sunThis works beautifully. Excellent use of proc_open().
Merely fixed stale docs about PCNTL and minor coding style issues. No functional changes, so ready to fly.
Comment #25
moshe weitzman CreditAttribution: moshe weitzman commentedFYI, drush does not use run-tests.sh so no risk there. Drush does use proc_open() for all backend operations and we have received little complaints. This patch looks like a good move for 8, and 7 too IMO. It is not like run-tests.sh can take down your site. No need for extreme caution.
Comment #26
webchickI'm glad this won't screw over Drush (and apparently also doesn't screw up testing bot or ci.drupal.org). However, this patch looks terrifying for D7 because it's changing function signatures all over the place and other things. Can a summary be provided of what's happening here and why I shouldn't be scared to commit this to a stable point release?
Comment #27
moshe weitzman CreditAttribution: moshe weitzman commentedThe proposed change switches our technique of parallelizing tests from requiring a PHP extension that is not offerred on Windows to using just core PHP. So this benefits everyone who doesn't have PCNTL, which is a large percentage of php installs.
@webchick - i submit that those rules do not apply here. run-tests functions are not an API. the simpletest functions that it calls are the API. we don't have to provide backwards compat. Neither do we have to be terrified. If it breaks, we might bother a Jenkins install somewhere. No web sites can break because of this. It is kind of like changing a function signature thats internal to a hidden test module.
Comment #28
webchickBe that as it may, a summary of the changes here would still be nice. This patch is doing a heck of a lot more than s/pcntl_fork/proc_open/g;
Comment #29
sun@webchick: Just gave this another review due to your comment, but all of the changes actually pertain to usage of proc_open() only. I'm not sure whether you're familiar with proc_open() [many are not], but it's PHP's most advanced native set of system process control interface functions. Compared to simple process helpers like exec(), proc_open() opens a true process interface, including all available pipes (stdin, stdout, stderr), allowing to fetch the process state, and even retrieving/sending (further) data from/to it. We're not making use of the latter here, but we do use the facility to check the status of concurrently invoked sub-processes, in order to invoke a new one after an existing exited.
I agree with everyone else that there's nothing that is able to break, with regard to our usual meaning of breaking sites. Aside from that, this patch runs on drupaltesting.org for a very long time already, and also works on my Windows box - on which I'm finally able to use concurrency with it.
Comment #30
webchickOk, thanks. Let's give this a shot. We still have a couple of weeks to back out if we hit troubles.
Committed and pushed to 8.x and 7.x. Thanks!