PHPUnit plugin improvements

[ptejada]

Contribution Type: new plugin Link to Intent to Implement: #1262

This pull request affects the following areas:

• [x] Front-End
• [x] Build Plugins

In raising this pull request, I confirm the following (please check boxes):

• [x] I have read and understood the contributing guidelines?
• [x] I have checked that another pull request for this purpose does not exist.
• [x] I have considered, and confirmed that this submission will be valuable to others.
• [ ] I have created or updated the relevant documentation for this change on the PHPCI Wiki.
• [x] Do the PHPCI tests pass?

Detailed description of change:

A new plugin is introduced instead of updating the existing one because of all the changes required, plus it will ease the comparison between the two plugins. UPDATE: Updates were merged into the original plugin.

The new phpunit plugin is fully backwards compatible with the original one so it can potentially replace it. The plugin is broken into three testable components: an options parser, result parser and the plugin itself.

Features:

1. The args option in the configuration file now supports a named list of CLI arguments. Ex:

   php_unit:
   args:
      filter: "test"
      group:
          - "groupOne"
          - "groupTwo"

2. The build log will now include the default phpunit output.
3. New JSON result parser to deal with outstanding issues of the TAP parser. Ref: #571 , #624
4. Failures and errors are now also stored in the Errors tab of the front-end.
5. Failures and errors will include a collapsed stack trace in front-end Information tab.

[vhejda]

Okey, I pulled your PR and am testing it on our PHPCI server. Thanks a lot for the effort. I agree with you that the original plugin had insufficient output. I spent large part of a day debugging why, when all the tests are successful and phpunit runs with no problems, the PHPCI plugin still fails (with no output). I finally found out it was a deprecation notice. If I had the stdout of phpunit the whole time in the Build log, I would know it right away.

I am still having some problems running your v2 at the moment, so it seems it's not 100% BC with the config I had for v1.

This is what I had, and was running fine with v1:

test:
    php_unit_v2:
        config:
            - "app/phpunit_fast.xml"
        args: "--testsuite=unit --bootstrap=vendor/autoload.php"

... and here's the error I got:

RUNNING PLUGIN: php_unit_v2
PHPUnit 4.8.24 by Sebastian Bergmann and contributors.
Time: 143 ms, Memory: 5.50Mb
No tests executed!
E_NOTICE: Uninitialized string offset: 0
Exception: Notice: Uninitialized string offset: 0 in /var/www/phpci/PHPCI/Plugin/Util/PhpUnitResult.php line 51
PLUGIN: FAILED

Apparently, you're missing a null/empty string check on file_get_contents($this->outputFile).

But that doesn't explain why I got "No tests executed" in the first place. Or does it?

Anyway, thanks again for the contribution, much appreciated!

[vhejda]

Running fine with:

    php_unit_v2:
        args:
            configuration: "app/phpunit_fast.xml"
            testsuite: "unit"
            bootstrap: "vendor/autoload.php"

Which actually seems more logical than having config outside and everything else in the args. I would still fix the BC parsing of the config option being an array, so that people could start using your plugin out-of-the-box, just with altering the name of the plugin to v2. Thanks again!

[dancryer]

This looks great - As @vooj-tae suggests it would be great if you could (in addition to supporting your method) make it backwards compatible.

I'd also suggest implementing this as a drop-in replacement for the current plugin, there's no benefit in keeping two slightly incompatible versions around.

[ptejada]

Thank you guys for the feedback. It was my intention to make it fully backwards compatible but it looks like it failed in this case.

Instead of passing the args string directly to the command the code attempts to parse the string into flags in the class PhpUnitOptions. I will look into this case.

On Mon, Nov 7, 2016, 6:46 AM Dan Cryer notifications@github.com wrote:

This looks great - As @vooj-tae https://github.com/vooj-tae suggests it would be great if you could (in addition to supporting your method) make it backwards compatible.

I'd also suggest implementing this as a drop-in replacement for the current plugin, there's no benefit in keeping two slightly incompatible versions around.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/Block8/PHPCI/pull/1275#issuecomment-258815333, or mute the thread https://github.com/notifications/unsubscribe-auth/AAPd3cpsuxqZrEGvyhx7KTLNRq2wC-h4ks5q7w-SgaJpZM4KStSh .

Sincerely,

Pablo Tejada

From Mobile

[ptejada]

@dancryer Fixed the issue brought up by @vooj-tae and also merge the improvements into the the original plugin.

[vhejda]

Thanks for merging the plugins!

One more thing now: From what I understand, specifying test directory should be optional, even when I don't have config file. The original plugin annotates in it's constructor:

$options['directory'] Output Directory. Default: %BUILDPATH%

Such config should be valid I think:

test:
    php_unit:

I would expect it to run phpunit on the whole build directory. Right now I get a fail due to this code in PhpUnit::execute:

    if (empty($xmlConfigFiles) && empty($directories)) {
        $this->phpci->logFailure(Lang::get('phpunit_fail_init'));
        return false;
    }

[ptejada]

@vooj-tae That is strange. The directory did not really defaults to the %BUILDPATH%, maybe it did at some point but anymore. The plugin does however supports the zero configuration feature. The plugin attempts to find the following files in the %BUILDPATH% if no config or directory is provided:

phpunit.xml
phpunit.xml.dist
tests/phpunit.xml
tests/phpunit.xml.dist

If it finds any of the files above it run the tests using the config file otherwise it will fail.

As an example this repo will work with the minimal config you provided https://github.com/moneyphp/money It has the phpunit.xml.dist in the root of the project.

We can set the directory to default to %BUILDPATH%, but am not sure if there is any value on doing so.

[vhejda]

You're right, it's some legacy comment left there.

Another thing: Seems like (at least for me), after this plugin merge, the stdout of phpunit stopped working again. Once more I get just this in Build log:

RUNNING PLUGIN: php_unit
PLUGIN: FAILED

It's the same with plugin success, no output there either.

Errors tab is empty, and Information tab just shows Errors: 1 and a name of the failed test. So I am pretty much back to the original phpunit plugin behaviour.

Also, more often than not, I am getting this bug: #905 - I don't know what changed, but I have to manually re-build almost every build now, sometimes couple of times, just to get it cloned.

I am beginning to give up on PHPCI, maybe it's just not the right tool for our needs.

[dancryer]

@ptejada Is this ready to be merged in your opinion? Do you have any thoughts on @vooj-tae's comment above?

[ptejada]

@dancryer There is another update I want to make to handle test results with a warning state test. I came across an scenario where referencing an unknown symbol with a @covers tag will set the test status to warning instead of pass in the phpunit json output. In the case where there are only warnings the overall plugin execution state should not marked as failed.

As for @vooj-tae last comment he might have not restarted his daemon after making merging the last update since I was not able to reproduce any of the issues he mentioned.

[Fenikkusu]

Given that this is effectively deprecating the TapParser (Unless I'm mistaken), you should likely remove TapParser as part of this. Otherwise, I would say you need to maintain the ability to use it and give the option to choose between TapParser or the json one you've created.

[ptejada]

@Fenikkusu Indeed, at this point the pull request does replace the TAP parser. Initially the new parser was implemented as separate plugin but now is just replacing the existing one.

I will make make an update to remove the TAP parser.

[whitekiba]

Is someone still working on this? I started using PHPCI a week ago and need support for a current PHPUnit version. I would work on this.

[ptejada]

I am currently using this PR in production without any issues using PHPunit 5.6

Unfortunately the json output was deprecated on PHPunit 6. The future prove solution is to build a jUnit output parser. I think i saw a PR for it but the options were not flexible as in this PR.

[tvbeek]

Local I did create a copy of the PHPUnit plugin that use a tool I wrote ( https://packagist.org/packages/tjvb/testreportmixer ) to transform JUnit output to tap. And then I use the TapParser to parse it. If wanted I can create a PR for that

[whitekiba]

Not only a jUnit parser would be needed. Also the progress output parser has to be replaced as the Tap Output in PHPUnit was deprecated and removed. I would start working on the PHPUnit plugin to make it compatible to PHPUnit 6+

[tvbeek]

Yes, I let PHPUnit output JUnit (that is available in PHPUnit 6), then transform it to tap and let the old parsing handle it.

[cnizzardini]

How's this coming along?