Reuse bootstrap: --runner WrapperRunner #29

Merged
merged 41 commits into from Jun 30, 2013

Projects

None yet

3 participants

@giorgiosironi
Collaborator

Related to #26
Results are the same with the old runner (Runner) and new one (WrapperRunner) afaik. Workers that crash trigger an exception in the main process. I plan to do more work on this (extracting a WorkerPool object, restarting workers and signaling crashed tests as errors, investigating performance), but this is the minimum marketable feature.

giorgiosironi and others added some commits Feb 2, 2013
@giorgiosironi giorgiosironi Tests to expose problems with: lots of short tests, slow bootstraps 4c47f4e
@giorgiosironi giorgiosironi Improved small-tests test case: they actually run in parallel now bfee98a
@giorgiosironi giorgiosironi Implemented hack for wrapping multiple PHPUnit commands in a single p…
…rocess
7265094
@giorgiosironi giorgiosironi No strict checking of errors: subprocesses may die and the result sti…
…ll be good
4ced1f9
@giorgiosironi giorgiosironi Result sucks, but 5 fixed worked processes are able to run the fixtur…
…e suite now
9adb74e
@giorgiosironi giorgiosironi Added Worker object a5b195b
@giorgiosironi giorgiosironi Pointing to new WrapperRunner 4962d3c
@giorgiosironi giorgiosironi Problems in results but all tests are run 7615311
@giorgiosironi giorgiosironi Runner should stay the same as the original 41aa2de
@giorgiosironi giorgiosironi Removed debug statements c424062
@giorgiosironi giorgiosironi ExecutableTest is compatible with master version 8cc33fd
@giorgiosironi giorgiosironi Workers are now managed via strea_select() instead of fixed sleep() a…
…nd busy waits
6153558
@giorgiosironi giorgiosironi Fixed fixture output f28c4dd
@giorgiosironi giorgiosironi New performance tests compare the runners for now 5150ddc
@giorgiosironi giorgiosironi More realistic test cases d25dbb1
@giorgiosironi giorgiosironi Comments ea119dc
@giorgiosironi giorgiosironi Test refactored as an it one b88e330
@giorgiosironi giorgiosironi Fixed output, which now considers all log files 5127815
@giorgiosironi giorgiosironi Dealing with Workers crashing 435010b
@giorgiosironi giorgiosironi Improvement in error management b3453ef
@giorgiosironi giorgiosironi Defining PARATEST environment variable c01b991
@giorgiosironi giorgiosironi Merging master and environment_variable b3a20f7
@giorgiosironi giorgiosironi Updated number of tests and assertions 9774189
@giorgiosironi giorgiosironi Early error detection in case of crash 8be391a
@giorgiosironi giorgiosironi Merge branch 'environment_variable' into reuse_bootstrap e849822
@giorgiosironi giorgiosironi Supporting but ignoring environment variables in commands 65500c4
@giorgiosironi giorgiosironi Supporting environment variables in Workers 80c2fc2
@giorgiosironi giorgiosironi Starting refactoring of WrapperRunner d4928b0
@giorgiosironi giorgiosironi Extracted method for manipulating Workers and streams e4a12fd
@giorgiosironi giorgiosironi Improved brittle tests based on order of test files a8f9fb2
@giorgiosironi giorgiosironi Merge branch 'environment_variable' into reuse_bootstrap 6c87122
@giorgiosironi giorgiosironi Updated TODO 0e045d8
@giorgiosironi giorgiosironi Checking errors with E_ALL daf91a0
@giorgiosironi giorgiosironi WrapperRunner displays error in case of Fatal b033624
@giorgiosironi giorgiosironi The standard behavior in case of Fatal Error is to interrupt the whol…
…e suite
2b2f848
@giorgiosironi giorgiosironi TEST_TOKEN is passed to Workers 4e2ed39
Giorgio Sironi Correctly exporting phpunit-wrapper binary, making it available and r…
…unning both from Git checkout and Composer installation
64229c1
Giorgio Sironi Fixed path of vendor/autoload.php 886c67a
Giorgio Sironi No need to expose phpunit-wrapper 07173b5
Giorgio Sironi Waiting for nothing cannot make the assignment cycle exit de7da02
Giorgio Sironi Added --no-test-tokens 471571d
@brianium brianium was assigned May 15, 2013
@brianium
Owner

Just wanted to let you know I have begun to review this monster. Looks like good stuff, but this could take a hot second.

@brianium brianium commented on the diff May 30, 2013
functional/FunctionalTestBase.php
+
+ protected function createSmallTests($number)
+ {
+ exec("php {$this->path}/generate.php $number", $output);
@brianium
brianium May 30, 2013

Is this being used? generate.php is in the small-tests directory and not in the default FIXTURES . DS . 'tests'

@brianium
Owner

Ok. I got through all the merge issues, and got most of the tests passing - but when I run the functional test PerformanceTest - I am hitting some strange output when the WrapperRunner is executed:

There was 1 error:

1) PerformanceTest::testRunningSuitesWithLongBootstrapsInFasterWithTheWrapperRunner
RuntimeException: Cannot parse output: 
Running phpunit in 2 processes with /home/brian/projects/paratest/vendor/bin/phpunit

Configuration read from /home/brian/projects/paratest/phpunit.xml.dist

ArrayArrayArrayArrayArray

I've only investigated far enough to determine is happening in the WrapperRunner after the the printer is started. I will investigate further tomorrow, but I wanted to put this here in case anyone had any ideas before then.

@brianium
Owner

The above was an issue with the ResultPrinter using the old feedback mechanism. Fixed.

@brianium
Owner

Ok. This is probably a result of some of the recent changes, but PerformanceTest is still failing at the method testRunningSuitesWithLongBootstrapsInFasterWithTheWrapperRunner because $worker->isFree() is returning false for a given worker. Logging the $lines used in Worker::updateStateFromAvailableOutput() I am getting the following message:

Executing: PARATEST=1 /home/brian/projects/paratest/vendor/bin/phpunit --no-globals-backup --bootstrap /home/brian/projects/paratest/test/bootstrap.php --configuration /home/brian/projects/paratest/phpunit.xml.dist --log-junit /tmp/PT_AGnOYc /home/brian/projects/paratest/test/fixtures/tests/UnitTestWithMethodAnnotationsTest.php
PHPUnit 3.7.21 by Sebastian Bergmann.

Class '/home/brian/projects/paratest/test/fixtures/tests/UnitTestWithMethodAnnotationsTest' could not be found in '/home/brian/projects/paratest/test/fixtures/tests/UnitTestWithMethodAnnotationsTest.php'.

If I run the command from the "Executing" line manually it works fine. I have pushed the branch I am working on merging here https://github.com/brianium/paratest/tree/giorgiosironi-reuse_bootstrap

@brianium
Owner

Apparently I pushed the wrong branch without the marged items. I will update this asap.

@brianium
Owner

Also I wanted to ask about compatibility with Windows. Windows has some known issues with using stream_select and file descriptors returned by proc_open. Will the WrapperRunner function under Windows? If not do we want to implement it and provide it as an option for *Nix users only?

@brianium
Owner

Ok - the above reference to https://github.com/brianium/paratest/tree/giorgiosironi-reuse_bootstrap should be up to date now.

@giorgiosironi
Collaborator

The Windows question has its own issue now, but in this context I think limiting a single runner to Unix is not a problem given the availability of the original one.

@brianium
Owner

I agree.

@giorgiosironi
Collaborator

I've checked out the branch, but I'm reproducing the already fixed issue, not the current one:

[11:30:00][giorgio@Desmond:~/code/paratest]$ bin/test functional
PHPUnit 3.7.13 by Sebastian Bergmann.

Configuration read from /home/giorgio/code/paratest/phpunit.xml.dist

................I......................EI.I......

Time: 05:39, Memory: 4.00Mb

There was 1 error:

1) PerformanceTest::testRunningSuitesWithLongBootstrapsInFasterWithTheWrapperRunner
RuntimeException: Cannot parse output: 
Running phpunit in 2 processes with /home/giorgio/code/paratest/vendor/bin/phpunit

Configuration read from /home/giorgio/code/paratest/phpunit.xml.dist

BEFORE ALL PENDING
........F..E.F.

/home/giorgio/code/paratest/functional/PerformanceTest.php:101
/home/giorgio/code/paratest/functional/PerformanceTest.php:92
/home/giorgio/code/paratest/functional/PerformanceTest.php:33

FAILURES!
Tests: 49, Assertions: 55, Errors: 1, Incomplete: 3.

@brianium
Owner

Have you looked into the Symfony Process component? It looks like they have a clean way of grabbing output when it is ready, even on windows systems, along with some OS specific optimizations. We are implementing it as part of our work on https://github.com/brianium/paratest/tree/feature/windows-compat, and so far it seems pretty solid. Would this be something that could be plugged into the WrapperRunner?

https://github.com/symfony/Process
http://symfony.com/doc/current/components/process.html

@giorgiosironi
Collaborator

It's a nice library, but I went for the PHP native functions because I need to wait on multiple processes at the same time, not on a single one (with stream_select()). Symfony Process seems to support wait() only a single process.
So I need to make a contribution to Symfony Process first to support this feature, as their $pipes variable is private. Is this a blocker for merging into master?

@brianium
Owner

I just thought I would mention it. It does not block anything as far as I'm concerned. Since it is an optional runner, I think it is fine to have its functionality confined to *nix for now.

@giorgiosironi
Collaborator

Debugging the last failing test. The problem should be that PHPUnit calls get_declared_classes() before and after inclusion of the file, so the class UnitTestWithMethodAnnotationsTest is probably already loaded when PHPUnit gets to that. get_declared_classes() before and after inclusion returns than the same result and PHPUnit sees the file as empty.
Working on improving the command executed to be more precise about the class, if possible.

@giorgiosironi
Collaborator

The problem is:

class UnitTestWithErrorTest extends UnitTestWithMethodAnnotationsTest

and the superclass gets included before its execution.

@giorgiosironi
Collaborator

Now the commands have the form:

vendor/bin/phpunit --option value ClassNameTest /folder/ClassNameTest.php

and work with namespaces too.
The suite of the giorgiosironi-reuse_bootstrap branch on this repository is green.

@giorgiosironi
Collaborator

Is something else needed for merging?

@giorgiosironi
Collaborator

I tried merging master into the branch to facilitate the job, but everything exploded with conflicts.

@giorgiosironi
Collaborator

Using the ours strategy on your master, I've created another branch with merged master:
https://github.com/brianium/paratest/tree/giorgiosironi-reuse_bootstrap-merged_master
that does not contain conflicts.
Suite is green on that branch too.

@brianium
Owner

Awesome. I will take a look asap. Are we hoping to get this in with 0.5.0?

@brianium
Owner

I might be missing something, but it appears as though the merge from master didn't quite take?

For instance: the ExecutableTest from master uses the Process component instead of proc_open, and environment variables are no longer being prefixed on the command string. The getCommandString method has been significantly cleaned up in master, and the run method has been updated as well.

I am comparing master against https://github.com/brianium/paratest/tree/giorgiosironi-reuse_bootstrap-merged_master.

I must confess I am unsure how to proceed :) I would love to get this added in, but with all the changes and ongoing development, it is a little overwhelming for me. Would it be at all helpful to put a "freeze" on master so some of these things can be added safely?

@ChubV ChubV referenced this pull request Jun 18, 2013
Closed

Roadmap, moving forward. #47

@brianium
Owner

Not sure the ours strategy is appropriate for this. It ditches changes from HEAD. I am thinking of creating a new branch and manually adding these changes, and in the mean time we should keep any major changes out of master. This will make 0.5.0!

@giorgiosironi
Collaborator

I tried first merging master into the branch with the default strategy. Lots of conflicts ensued, but you have better knowledge of what changed in master.
Then I tried with the ours strategy, but probably to no avail.
The working version is still https://github.com/brianium/paratest/tree/giorgiosironi-reuse_bootstrap-merged_master where there has been no merge.

@brianium
Owner

I've blocked some time this week to go through this manually. I'm not even sure a diff tool would give meaningful feedback - if only for the fact that there were comments added to every file since this pull request was added.

@brianium
Owner

No matter how hard I try, I cannot get this working. @giorgiosironi , would you be able to take a shot at manually merging this stuff into the latest? I gave it a shot and I can not get a passing functional suite, even given the latest changes with fully qualified namespaces.

@giorgiosironi
Collaborator

Will try now.

@giorgiosironi giorgiosironi merged commit 471571d into brianium:master Jun 30, 2013
@giorgiosironi
Collaborator

Merged manually from giorgiosironi-reuse_bootstrap, now my master and the official repository are in sync.

@giorgiosironi giorgiosironi deleted the giorgiosironi:reuse_bootstrap branch Jun 30, 2013
@dbaltas
Collaborator

The build for php 5.3 failed. php 5.4 OK.

@dbaltas
Collaborator

@giorgiosironi
I couldn't get a successful run of the functional tests
3 runs on revision a8132c4 with failures
Last Run on revision c4ea833 successful
Output in Pastie.
My Configuration

PHP 5.4.6-1ubuntu1.2 (cli) (built: Mar 11 2013 14:57:54)
Copyright (c) 1997-2012 The PHP Group
Zend Engine v2.4.0, Copyright (c) 1998-2012 Zend Technologies
with Xdebug v2.2.1, Copyright (c) 2002-2012, by Derick Rethans

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment