[PATCH] phpunit throws exception when parsing failure

[mcarpent@…]

Existing phpunit code from phptools attempts to use functions from xmlio, but uses the xml.dom.minidom parser. This leads to a generated exception and no failure log for the failed tests.

[mcarpent@…]

I noticed the same bug in the modified version from ticket #199. I've since written a new version that will use the xmlio classes and parse through subsuites correctly.

I'm not a python programmer, so I'm not sure if this would be an acceptable solution for the community or not... but here's what I did:

• Grab the test suites using xmlio.parse.children('testsuite')
• Pass the result to parsesuite
  • for suite in testsuite
    • if suite.children
      • recurse
    • else parse the entry normally

[mcarpent@…]

Recursing phpunit which uses xmlio

[anonymous]

pls have a look at #199, where I attached a patched phptools.py months ago.

[mcarpent@…]

From the attached file in #199

60	                    result = list(testcase.childNodes)
61	                    if result:
62	                        test.append(xmlio.Element('traceback')[
63	                            result[0].gettext()
64	                        ])
65	                        test.attr['status'] = result[0].name

Bitten-slave excepts on line 63 because gettext is an xmlio method and testcase.childNodes return xml.dom.Element objects. Unfortunately that means I couldn't use the method from #199 since there was a need to loop through child testsuites rather than just skipping over them.

[mcarpent@…]

phpunit xml which fails on tunk, and version from #199. Works with this patch

[anonymous]

Now that I am at work I have attached the xml file I was using for testing. I was testing the versions from ticket #199, and this version with this snippet:

    import bitten.recipe
    import bitten.build.phptools
    ctxt = bitten.recipe.Context("d:/www/trunk")
    bitten.build.phptools.phpunit(ctxt, "test.xml")

The version from ticket #199 produces the following exception which stops bitten-slave from reporting failure data to the master:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "c:\python25\lib\site-packages\bitten-0.6dev_r559-py2.5.egg\bitten\build\
phptools.py", line 63, in phpunit
    result[0].gettext()
AttributeError: Text instance has no attribute 'gettext'

The version which I have posted uses only xmlio methods, and avoids this exception. The drawback to this is that the xml.dom method used originally gets all child nodes which are named 'testsuite', whereas the the xmlio version only searches as the current level. This is why I turned to a recursive method to parse the testsuites rather than the method from #199 which just skips suites with subsuites. I've basically done the same thing, but in a different way due to the limitations of xmlio.

(note this is running on windows, but I have the same behaviour on my linux boxes)

[mrhyde]

here's my version which uses name attribute rather then looking for children

[osimons]

Using closure instead of module-level method.

[osimons]

Thanks for patches and input. I'm not a PHP user, but from reading #199 and this ticket, the patch by mrhyde makes sense as far as I can see. Looks like it also handles the nesting issues reported in #199, right?

I've just updated it to use a closure to keep the phpunit() method self-contained.

At least Bitten tests pass - other than that I'm clueless about PHP or its testing...

If someone can confirm that a) it is still a problem/need, and b) the patch works for #199 and #316, I'll happily apply it and close tickets.

[r.wilczek@…]

enhanced and fixed version of phptools.py as attached to #199

[r.wilczek@…]

I just attached an enhanced and fixed version of my phptools.py. The calls to gettext in line 63 are removed. I am using this version extensively for more than a year on Linux without any problems.

Tested to run with PHPUnit 3.3.17.

[osimons]

Thanks for the updated file - very useful. I have played with it, and do have some questions and comments:

1. Is the change of method signatures really needed? Can't it use the regular ctxt.basedir logic that all other similar commands use? I'm very hesitant about changing method/command signatures.
2. Same with new codesniffer command - does it really need sourcedir input?
3. I'd really like unittest + docs for new codesniffer command...
4. coverage() makes an extraction using getElementsByTagName('line'), but the current test for coverage has no such attribute so I can't get the tests to pass. Is the dummy report in coverage test not correct or current?

So, If we can a) keep method signatures, b) get passing tests, and c) update docs, this would be a very useful contribution.

I've never touched PHP, so don't really know where to start. However, if you can point me to some publicly available PHP source + a working recipe for the PHP commands, I'd be happy to get PHP building running on my development setup and help out with this.

[r.wilczek@…]

1. Signature-changes. Well ...
   
   • The phpunit-command needs a sourcedir-parameter, because
     
     • the tests are executed with and on exported stuff, and the file-attribute in PHPUnit's results.xml refers to the location of the exported files in the build-slave's filesystem. We simply don't know what the build-slave is doing and where he exports and tests.
     • the hyperlink in the build's "Test Results"-view should not depend on that, but link to the location of the files in the repository. This is why I need the parameter: The build must tell me the filesystem-location he used so that I can erase it. Without that parameter the view's hyperlinks always lead you to some nirvana in ​http://yourtrac/'''browser'''/path/to/buildslaves/filesystem
   • The coverage-command uses dir- and classdir-parameters because
     
     • dir is used for the same reason as sourcedir in the phpunit-command: Enable the Code-Coverage-view to link to the files in the repository and not to something on the build-slave's local filesystem
     • classdir is used to skip coverage-information regardless of PHPUnit's coverage-filtering. I agree that this violates the principle of single responsibility, because the user can (and should) use PHPUnit's whitelist/blacklist-configuration to tell what he wants to see in the report and view. I introduced that parameter with a former version of PHPUnit and am glad to have it, for whitelisting/blacklisting sometimes changes in PHPUnit's development. To make a long story short: classdir limits the view to the files in that directory, so the coverage of other files (helper, testfiles itself etc.) which reside somewhere else is not rendered.

2. Forget about the codesniffer-command - simply delete it. I forgot to do this myself before uploading the file out of my running project.

3. Here is a step from my current PHP-project doing the PHPUnit-tests and reporting results and coverage to bitten:

   <step id="Unit-Tests" description="Tests and Code-Coverage analysis">
     <!-- Doing the unittests with PHP's phing (== invoking PHPUnit on the commandline)
          and storing the resultfiles in the builds/orm/reports/${revision}-folder:
          <exec command="phpunit --coverage-clover /home/bitten/builds/orm/reports/${revision}/coverage.xml 
                                 --log-xml /home/bitten/builds/orm/reports/${revision}/results.xml
                                 AllTests ${sources}tests/AllTests.php" 
     -->
     <php:phing file="/home/build.xml"
       args="-Dpath=${path} -Drevision=${revision} -quiet code-coverage"/>
   
     <!-- Invoking the phpunit-command from phptools.py with the given results-XML-file -->
     <php:phpunit file="/home/bitten/builds/orm/reports/${revision}/results.xml"
       sourcedir="/home/bitten/builds/orm/rephORM"/>
   
     <!-- Invoking the coverage-command from phptools.py with the coverage-XML-file
          and limiting the rendering to files in /classes/ only -->
     <php:coverage file="/home/bitten/builds/orm/reports/${revision}/coverage.xml"
       dir="/home/bitten/builds/orm/rephORM"
       classdir="/classes"/>
   </step>
   

4. Yes, there are line-elements in current coverage-reports. I will attach a coverage-report created with current PHPUnit 3.3.17 which probably contains all types of information PHPUnit can supply. Basically the XML looks like that:

   <coverage generated="1248771791" phpunit="3.3.17">
   <project name="All Tests" timestamp="1248771791">
   <file name="/home/bitten/builds/orm/rephORM/classes/ORM/Map/Index/Primary.php">
       <class name="ORM_Map_Index_Primary" namespace="global" fullPackage="Map" package="Map">
           <metrics methods="5" coveredmethods="5" statements="10"
                    coveredstatements="13" elements="15" coveredelements="18"/>
       </class>
       <line num="34" type="stmt" count="1"/>
       <line num="51" type="method" count="776"/>
       <line num="53" type="stmt" count="776"/>
       <line num="54" type="stmt" count="776"/>
       <line num="62" type="method" count="735"/>
       <line num="64" type="stmt" count="735"/>
       <line num="65" type="stmt" count="735"/>
       <line num="66" type="stmt" count="735"/>
       <line num="74" type="method" count="124"/>
       <line num="76" type="stmt" count="124"/>
       <line num="77" type="stmt" count="124"/>
       <line num="78" type="stmt" count="124"/>
       <line num="79" type="stmt" count="124"/>
       <line num="80" type="stmt" count="124"/>
       <line num="88" type="method" count="746"/>
       <line num="90" type="stmt" count="746"/>
       <line num="91" type="stmt" count="419"/>
       <line num="100" type="method" count="5"/>
       <line num="102" type="stmt" count="5"/>
       <line num="105" type="stmt" count="1"/>
       <metrics loc="104" ncloc="38" classes="1" methods="5" coveredmethods="5"
                statements="15" coveredstatements="15" elements="20" coveredelements="20"/>
   </file>
   <!-- more <file>-elements -->
   </project>
   </coverage>
   

5. I will attach a current result-report too, because it shows that there is more than one kind of nested testsuites in PHPUnit: The ones created the "traditional way" (adding testcases to testsuites), and the ones created "implicitely" by using PHPUnit's "dataProvider"-annotation

[r.wilczek@…]

Large and complex XML-results from PHPUnit

[r.wilczek@…]

Large and complex XML-coverage from PHPUnit

[osimons]

Thanks!

About 1: I'd still argue you can mange without the additional arguments. You do know the location of your files through ctxt.basedir - all commands fetch code or operate on code should use paths relative to this. It is the same that happens with the Python unittest and lint output for instance, and these commands manage to rework filenames so that they both display directly and link directly - see reports in 'test' section in build:1516 for instance. Now if the command logic for some reason was not correct (as for instance it wasn't with pylint until i recently fixed it, see #418 / [669]) then just tweaking the filename manipulation code should be sufficient. As for the skipping of files using classdir, would it not be sufficient to only include the files located inside the ctxt.basedir (build directory) - that should skip external libraries and the like, no?

About 3: Thanks, I'll try to locate some php code with unittests and get your recipe working.

About 4-5: Thanks - looks useful for updating and extending unittests for commands.

[r.wilczek@…]

enhanced and fixed version of phptools.py as attached to #199 without lava-flow

[r.wilczek@…]

attached a phptools.py-version without undocumented lava-flow (codesniffer-command)

[osimons]

About 1+3 again, I see you use absolute paths in your recipe - that likely explains your needs. Could you not instead run bitten-slave --work-dir=DIR --build-dir=BUILD_DIR ... to point your slave to the correct locations? That is how things are made to work - and this will then populate ctxt.basedir that you need.

Thanks for new file, I see that has a new phing command instead?

[r.wilczek@…]

err ... no, the phing command has been there before and was shipped with bitten.

Thanks for your hint for configuring the slave! I will try to get rid of that parameter.

(Diving into the unknown water's of the Python-ocean again ...)

[r.wilczek@…]

ahm ... what can I do to get an account in this trac?

[osimons]

Replying to r.wilczek@…:

err ... no, the phing command has been there before and was shipped with bitten.

Heh. Shows you I'm really familiar with the PHP stuff... :-)

As for account, I don't think so - only developers have Edgewall accounts. Is there any permission that is too strict for you as anonymous? If it is a display-thing, then see Preferences and set a name and an email - it is stored in session.

About --build-dir=DIR, as another example see docs for recently added hg:pull command that updates an existing repos - same use-case really: wiki:Documentation/commands.html#hg-pull

[r.wilczek@…]

I want to provide a mini PHP-environment with a simple testsuite for you to play with.

Trac rejects my PHP_Playground.tar.gz as well as PHP_Playground.zip as potential spam. :-(

[r.wilczek@…]

Could you point me to some more documentation about --work-dir and --build-dir?

Which data will be provided to ctxt.basedir when wich of the options is set to what?

[mcarpent@…]

Well, personally I like mrhyde's solution better. It just seems... simpler. Been a while since I've been working on PHP with Bitten and PHPUnit, but I can see about setting up an environment to test it out.

[Roland Wilczek <r.wilczek@…>]

I just tried mrhyde's solution with my PHP-testsuite here (Needs either to be fixed with ctxt-parameter or with ​http://bitten.edgewall.org/attachment/ticket/316/t316-phpunit_parsing-r669.diff). The problem is that it is not able to process all kinds of nested testsuites PHPUnit provides.

[ERROR   ] Internal error in build step 'Unit-Tests'
Traceback (most recent call last):
  File "/usr/local/lib64/python2.6/site-packages/Bitten-0.6dev-py2.6.egg/bitten/slave.py", line 268, in _execute_step
    step.execute(recipe.ctxt):
  File "/usr/local/lib64/python2.6/site-packages/Bitten-0.6dev-py2.6.egg/bitten/recipe.py", line 191, in execute
    ctxt.run(self, child.namespace, child.name, child.attr)
  File "/usr/local/lib64/python2.6/site-packages/Bitten-0.6dev-py2.6.egg/bitten/recipe.py", line 95, in run
    function(self, **args)
  File "/usr/local/lib64/python2.6/site-packages/Bitten-0.6dev-py2.6.egg/bitten/build/phptools.py", line 73, in phpunit
    _process_testsuit(ctxt, testsuit, results)
  File "/usr/local/lib64/python2.6/site-packages/Bitten-0.6dev-py2.6.egg/bitten/build/phptools.py", line 38, in _process_testsuit
    _process_testsuit(ctxt, testcase, results)
  File "/usr/local/lib64/python2.6/site-packages/Bitten-0.6dev-py2.6.egg/bitten/build/phptools.py", line 38, in _process_testsuit
    _process_testsuit(ctxt, testcase, results)
  File "/usr/local/lib64/python2.6/site-packages/Bitten-0.6dev-py2.6.egg/bitten/build/phptools.py", line 38, in _process_testsuit
    _process_testsuit(ctxt, testcase, results)
  File "/usr/local/lib64/python2.6/site-packages/Bitten-0.6dev-py2.6.egg/bitten/build/phptools.py", line 38, in _process_testsuit
    _process_testsuit(ctxt, testcase, results)
  File "/usr/local/lib64/python2.6/site-packages/Bitten-0.6dev-py2.6.egg/bitten/build/phptools.py", line 41, in _process_testsuit
    test.attr['fixture'] = testcase.attr['class']
  File "/usr/local/lib64/python2.6/site-packages/Bitten-0.6dev-py2.6.egg/bitten/util/xmlio.py", line 262, in __getitem__
    raise KeyError(name)
KeyError: 'class'

PHPUnit knows of testcases with (traditional manual nesting) and without class-attribute (nested testsuites via @dataProvider-annotation).

[Roland Wilczek <r.wilczek@…>]

depend on ctxt, waste redundant parameters and made coverage-limiting optional

[Roland Wilczek <r.wilczek@…>]

ok, here I am again. just attached a version of phptools.py with the following signatures:

def phpunit(ctxt, file_=None)
def coverage(ctxt, file_=None, classdir='')

phpunit

• depends on ctxt.basedir now and correctly renders hyperlinks into the trac-browser.
• supports all kinds of nested testsuites in PHPUnit

coverage

• depends on ctxt.basedir and correctly renders hyperlinks into the trac-browser
• if the optional classdir-parameter is given as a subdirectory of ctxt.basedir the reporting is limited to files of that subdirectory:

  bitten-slave http://localhost/trac/project/builds --work-dir=/home/bitten/builds/project/ --build-dir="export"
  <php:coverage file="/home/bitten/builds/reports/${revision}/coverage.xml" classdir="/classes"/>
  

  In this scenario the coverage-reporting is limited to the files in /home/bitten/builds/project/export/classes

osimons: Is having an optional parameter ok for you?

[Roland Wilczek <r.wilczek@…>]

remove redundant variables and be more expressive

[Roland Wilczek <r.wilczek@…>]

just attached a "beautified" version.

I thought about removing the optional classdir-parameter in the coverage-command and replacing it with ctxt.basedir via configuring the slave with --build-dir and --work-dir.

Problem is, that when ctxt.basedir is limited to a subdirectory, phpunit would render wrong file-attributes in line 86. So having phpunit and coverage in the same recipe/step(?) and limiting coverage to a subdirectory, would not work.

Or is there a way to inject additional information to the commands via ctxt? Or allows ctxt already to tell what is given via --build-dir and what is given via --work-dir?

[Roland Wilczek <r.wilczek@…>]

Be more pricky about block-scope and avoid redundant element-extraction

[osimons]

I think you can see quite the same logic in the Python tools - see specifically source:/trunk/bitten/build/pythontools.py#L213​ that basically does continue (ie. ignores by continuing to next iteration) files that are not inside the build directory. All files that aren't os.path.isabs(path) are first joined with ctxt.basedir to make all absolute.

That basically skips files that are not inside the build directory. The commands should preferably not support features that aren't part of the underlying command unless there is a strong reason for it.

I haven't had time to look at latest file yet, but will later tonight.

[Roland Wilczek <r.wilczek@…>]

fix typos and extract method

[Roland Wilczek <r.wilczek@…>]

well, then you should look at v7 - the previous ones had typos.

I will think of the parameter again. Finally it is easy to remove the parameter at all and let it to the user to play around with slave-configuration. It is not essential and defaults to an empty string.

[osimons]

Thanks for the small 'Foo' test project received by email. I've added it to my test repos to run a full build cycle that includes this step:

  <step id="Unit-Tests" description="Tests and Code-Coverage analysis">
    <sh:exec executable="${php.phpunit}"
             args="--coverage-clover coverage.xml --log-xml results.xml Foo/tests/AllTests.php" />
    <php:phpunit file="results.xml"/>
    <php:coverage file="coverage.xml"/>
  </step>

That works like a charm for unittest and coverage reports. I've got no build.xml to run the phing command, nor am I inclined to learn it well enough to make my own...

Digging into why the coverage xml has changed so much, I suddenly noticed why: It parses PHPUnit coverage report, and not the one from Phing as currently in trunk (that I haven't managed to produce). No wonder they are different, and in reality that makes it a new command/feature.

Therefore I think we need to let the coverage command signature remain unchanged, but we change internals to handle both Phing (existing code) and PHPUnit coverage reports (as from new patch) - parse the XML, and call relevant closure to do the actual processing depending on what the XML looks like.

I've updated my patch with this new logic, and all old tests pass unchanged + new phpunit coverage report looks just like v7. Patch also updates docs and makes room for a phpunit coverage test that I haven't made yet (will be done before committing).

Patch follows.

[osimons]

Updated patch. Uses slightly modified original recursive phpunit patch (parent) + coverage that handles both Phing and PHPUnit.

[Roland Wilczek <r.wilczek@…>]

I just wanted to remove that optional parameter from my version. I think it's better to simply process any content of the XML-files. The user is very well able to configure what is contained in the files on the PHPUnit-side (blacklisting/whitelisting).

But now I hesitate to upload v8, for you seem to be back to your sources?

By the way: forget about Phing's coverage-reports. They are not able to deal with nested testsuites ... That's why your way to directly <sh:exec> phpunit is correct.

I think if your script correctly processes the large XMLs I uploaded yesterday you're done.

[Roland Wilczek <r.wilczek@…>]

removed optional parameter and internal refactoring. Signatures are compatible again

[Roland Wilczek <r.wilczek@…>]

Just attached my PHPUNit-only version with the optional parameter in coverage removed.

The user should use PHPUnit's more powerfull blacklisting/whitelisting to limit the coverage report.

[Roland Wilczek <r.wilczek@…>]

argh ... THIS one is the correct version. Forget v8

[osimons]

I've committed [671] that I think is a good, working solution. Thanks!