Switch output dependency from TAP to Teamcity #237

padraic · 2017-04-29T20:42:47Z

Just some simple changes. I disabled early termination of the ProcessRunner since it shouldn't be needed if we are telling PHPUnit to stop on first failure (via command line option), and it ends up truncating the failure messages when we do it manually.

However, I think the \Humbug\ProcessRunner needs a bit of attention beyond this. I've commented out lines, and there seems to some interference when underlying unit-under-test is also using processes. You can spot it by attempting to use Humbug on https://github.com/Seldaek/monolog (for example). The failure between Humbug, and running PHPUnit straight on Monolog, are very different, but you can switch failures by manipulating the ProcessRunner. This was happening WITHOUT my changes, so it's a separate pre-existing issue.

Anyways, will add tests and such over next day or so.

… output.

…ing ---teamcity flag) Checkout output on all scenarios just to be sure nothing else bugs out the output

padraic · 2017-04-30T13:34:51Z

Tests have a single failure which seems to have crept in via a possible PHPUnit bug from PHPUnit 5.2.0 onwards. I'm not sure if it's expected or not, but I've reported it here: sebastianbergmann/phpunit#2665

If the outcome is something expected/deprecated or otherwise not to be fixed, I'll scrub the offending test.

omissis · 2017-05-10T07:03:14Z

@padraic I was wondering if it might make sense to have adapters for each logging format, so to avoid couple the implementation to a specific one and be able to swap between them more easily.

padraic · 2017-05-10T07:36:10Z

@omissis That makes sense. There was a reason originally for the tight coupling, but that's no longer an issue.

padraic · 2017-05-12T11:06:04Z

@humbug/core Ready for review here now. There is one test now marked skipped - against the old PHPUnit suite assembly method using AllTests.php files. Can't quite find it documented in the PHPUnit changelog, but Sebastian certainly doesn't consider this a supported feature anymore.

theofidry

I did a first review, there's some questions for you :)

theofidry · 2017-05-12T12:59:30Z

src/ProcessRunner.php

-                $process->stop();
-                return;
+                $hasFailure = true;
+                //$process->stop();


why the comments?

theofidry · 2017-05-12T13:01:19Z

tests/Adapter/_files/regression/server-argv/phpunit.xml.dist

+                <directory>.</directory>
+            </exclude>
+        </whitelist>
+    </filter>
 </phpunit>


missing ending blank line

theofidry · 2017-05-12T13:01:55Z

features/running/developer_sees_initial_testrun_error_if_suite_not_passing.feature

-            Stdout:
-               > TAP version 13
-               > not ok 1 - Error: FooTest::testAddsNumbers
+            ##teamcity[testFailed


we no longer get to know where it's failing?

The teamcity block will have it - but the block varies between test runs, so I just checked that it was output for the scenario..

theofidry · 2017-05-12T13:03:03Z

src/Renderer/Text.php

+
+    protected function extractFail($output)
+    {
+        $result = preg_match('%##teamcity\[testFailed.*\]%', $output, $matches);


if (preg_match('%##teamcity\[testFailed.*\]%', $output, $matches)) { return $matches[0]; } return 'No failure output was detected by Humbug, but a failure was reported by PHPUnit.';

Could also be simplified by a ternary operator

theofidry · 2017-05-12T13:03:22Z

src/Renderer/Text.php

@@ -316,4 +316,13 @@ protected function headAndTail($output, $lineCount = 20, $omittedMarker = '[...M
            array_slice($lines, -$lineCount, $lineCount)
        ));
    }
+
+    protected function extractFail($output)


can we make it private?

theofidry · 2017-05-12T13:04:43Z

src/Adapter/AdapterAbstract.php

-            && !preg_match("%# TODO%", $line)) {
-                return false;
-            }
+        if (preg_match("%##teamcity\[testFailed%", $output)) {


return (bool) preg_match("%##teamcity\[testFailed%", $output);

You have to watch the casting to bool with preg_match - that's why I prefer taking the scenic route ;).

but isn't it what is done with the if statement? Too me both look strictly equivalent

Here's the short version that would work (excl. errors which return FALSE):

return 0 === preg_match("%##teamcity\[testFailed%", $output);

Added via commit just now.

theofidry · 2017-05-12T13:05:59Z

tests/Adapter/PhpunitTest.php

        $this->assertTrue($adapter->ok($result));
    }

    public function testAdapterRunsPhpunitCommandWithAlltestsFileTarget()
    {
+        $this->markTestSkipped('See https://github.com/sebastianbergmann/phpunit/issues/2665');


looks like this issue has been closed and that we shouldn't run that test

True - test should actually be deleted now.

omissis · 2017-05-12T17:15:17Z

src/Adapter/AdapterAbstract.php

        if ($result) {
-            return (int) end($matches[1]);
+            $this->okCount += $result;
+            return $this->okCount; // was: (int) end($matches[1]);


I would expect a has* method to always return a boolean

It probably should, and it's a public method to boot, but something we can clean up with greater forethought for 2.0 when defining interfaces/extension points.

padraic · 2017-05-13T18:41:52Z

@theofidry Some changes made.

theofidry

Last nitpicking, all good for me otherwise

theofidry · 2017-05-21T17:32:19Z

src/Adapter/AdapterAbstract.php

-            && !preg_match("%# TODO%", $line)) {
-                return false;
-            }
+        if (preg_match("%##teamcity\[testFailed%", $output)) {


but isn't it what is done with the if statement? Too me both look strictly equivalent

padraic · 2017-05-21T18:28:27Z

@theofidry In case my comment was lost in changing that, I updated the preg_match block to

return 0 === preg_match("%##teamcity\[testFailed%", $output);

Initial conversion from depending on TAP output. Switches to Teamcity…

1a1241b

… output.

padraic added the enhancement label Apr 29, 2017

padraic added 4 commits April 30, 2017 10:55

No need to split lines with Teamcity syntax having a newline marker

ebfac2c

Patch some tests towards passing state

4aa65a3

Update test fixtures to teamcity output

5792f56

Skip AllTests test when < PHPUnit 6 (fail here is a PHPUnit bug ignor…

c24a98f

…ing ---teamcity flag) Checkout output on all scenarios just to be sure nothing else bugs out the output

Skip over one test - unsupported PHPUnit feature being tested

571a953

padraic mentioned this pull request May 5, 2017

PHPUnit 6 Support #204

Closed

Merge branch 'master' into feature/teamcity

798b419

Merge branch 'master' into feature/teamcity

928c7fa

theofidry suggested changes May 12, 2017

View reviewed changes

omissis reviewed May 12, 2017

View reviewed changes

padraic changed the title ~~[WIP] Switch output dependency from TAP to Teamcity~~ Switch output dependency from TAP to Teamcity May 13, 2017

padraic added 2 commits May 13, 2017 19:21

Merge branch 'master' into feature/teamcity

f36aaae

Revisions for review points

03fe7c5

padraic added 3 commits May 13, 2017 20:23

Small style fix

b74b7b8

Reformat failure output to be easier to read

95f229c

Update testing tool mins to evade bug in TC format

2344e3d

theofidry approved these changes May 21, 2017

View reviewed changes

padraic added 3 commits May 21, 2017 19:02

Small adjustment to pcre match result check

4d71172

CS: fix trailing space

b387bf8

Exclude any extraneous test result from error detail collection

ec66182

Merge branch 'master' into feature/teamcity

7ca2e7e

theofidry merged commit 326371f into master May 30, 2017

theofidry deleted the feature/teamcity branch May 30, 2017 06:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch output dependency from TAP to Teamcity #237

Switch output dependency from TAP to Teamcity #237

padraic commented Apr 29, 2017 •

edited

padraic commented Apr 30, 2017

omissis commented May 10, 2017

padraic commented May 10, 2017

padraic commented May 12, 2017

theofidry left a comment

theofidry May 12, 2017

theofidry May 12, 2017

theofidry May 12, 2017

padraic May 12, 2017

theofidry May 12, 2017

theofidry May 12, 2017

theofidry May 12, 2017

theofidry May 12, 2017

padraic May 13, 2017

theofidry May 21, 2017

padraic May 21, 2017

theofidry May 12, 2017

padraic May 12, 2017

omissis May 12, 2017

padraic May 13, 2017

padraic commented May 13, 2017

theofidry left a comment

theofidry May 21, 2017

padraic commented May 21, 2017

Switch output dependency from TAP to Teamcity #237

Switch output dependency from TAP to Teamcity #237

Conversation

padraic commented Apr 29, 2017 • edited

padraic commented Apr 30, 2017

omissis commented May 10, 2017

padraic commented May 10, 2017

padraic commented May 12, 2017

theofidry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

padraic commented May 13, 2017

theofidry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

padraic commented May 21, 2017

padraic commented Apr 29, 2017 •

edited