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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Damien Tournoud’s picture

Status: Active » Needs review
FileSize
7.09 KB

Let's try this.

Damien Tournoud’s picture

Slightly 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.

rfay’s picture

Subscribe. I'll try to give this a run.

Damien Tournoud’s picture

Title: Concurrent testing on Windows » Cannot run tests on Windows
Category: feature » bug
Priority: Normal » Critical

Running tests on Windows now fail because we hit the limit of the command line length when forking the child runner:

The command line is too long.

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.

ksenzee’s picture

I'll try to get my Windows VM set up to run tests, so I can try this out.

ksenzee’s picture

Hm. 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

aspilicious’s picture

did you edit the php timeout settings?

ksenzee’s picture

I 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.

boombatower’s picture

Priority: Critical » Major

According 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.

attiks’s picture

FileSize
44.94 KB

I 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

attiks’s picture

Testing as we speak witrh the patched version, the --all parameters is working, to be continued

attiks’s picture

FileSize
27.21 KB

Patch 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

attiks’s picture

Status: Needs review » Needs work

see 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?

sun.core’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 771448-windows-concurrent-testing.patch, failed testing.

Damien Tournoud’s picture

Status: Needs work » Needs review
FileSize
7.18 KB

Reroll.

Damien Tournoud’s picture

With a proper git prefix.

Status: Needs review » Needs work

The last submitted patch, 771448-windows-concurrent-testing.patch, failed testing.

Damien Tournoud’s picture

Status: Needs work » Needs review
FileSize
7.13 KB

Fix a silly parse error.

aspilicious’s picture

Can 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...

chx’s picture

Title: Cannot run tests on Windows » Use proc_open() instead of pcntl_fork() in simpletest
Version: 7.x-dev » 8.x-dev
Category: bug » task
sun’s picture

Re-rolled as -p1 patch with latest code from drupaltesting.org.

22 days to next Drupal core point release.

Damien Tournoud’s picture

This has been running on drupaltesting.org for more then a year.

sun’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
8.01 KB

This 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.

moshe weitzman’s picture

FYI, 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.

webchick’s picture

I'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?

moshe weitzman’s picture

The 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.

webchick’s picture

Be 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;

sun’s picture

@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.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok, 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!

Status: Fixed » Closed (fixed)
Issue tags: -Needs backport to D7

Automatically closed -- issue fixed for 2 weeks with no activity.