Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for phpdbg with fast code coverage #258

Merged
merged 2 commits into from Jan 30, 2016
Merged

Support for phpdbg with fast code coverage #258

merged 2 commits into from Jan 30, 2016

Conversation

@JanTvrdik
Copy link
Contributor

JanTvrdik commented Oct 2, 2015

Most of critical phpdbg bugs are fixed since RC4, some bugs are however still present.

https://bugs.php.net/bug.php?id=70532
https://bugs.php.net/bug.php?id=70531
https://bugs.php.net/bug.php?id=70614

@Mikulas

This comment has been minimized.

Copy link
Contributor

Mikulas commented Oct 2, 2015

🏆

(also it partially solves #241)

@JanTvrdik JanTvrdik force-pushed the JanTvrdik:phpdbg branch from 7c8c329 to 5a6f7d3 Oct 2, 2015
*/
private static function startXdebug()
{
if (!extension_loaded('xdebug')) {

This comment has been minimized.

Copy link
@Majkl578

Majkl578 Oct 2, 2015

Contributor

This should (imho) also check PHP_SAPI !== 'phpdbg'?

This comment has been minimized.

Copy link
@JanTvrdik

JanTvrdik Oct 2, 2015

Author Contributor

Why? Phpdbg is pointless when you use xdebux strategy. Did you meant to use or or and?

throw new \Exception("Unable to detect PHP version (output: $output).");
} elseif (version_compare($matches[1], '7.0.0', '<')) {
throw new \Exception('Unable to use phpdbg on PHP < 7.0.0.');
}

This comment has been minimized.

Copy link
@Majkl578

Majkl578 Oct 2, 2015

Contributor

I've seen some really strange behavior when I ran phpdbg with XDebug loaded as a module. Maybe also check for XDebug not being loaded?

This comment has been minimized.

Copy link
@JanTvrdik

JanTvrdik Oct 2, 2015

Author Contributor

What? Phpdbg is only usable on php7 and xdebux is only usable on php5.

This comment has been minimized.

Copy link
@Majkl578

Majkl578 Oct 2, 2015

Contributor

XDebug will be usable on PHP 7 as well, once it's ported. It is already working for debugging (not coverage though).

@Majkl578

This comment has been minimized.

Copy link
Contributor

Majkl578 commented Oct 2, 2015

👍

Since phpdbg seems to be much faster than XDebug (and it is also a native part of PHP), how about making it a default interpreter for PHP 7.0+?


If anyone is testing this, be aware that before PHP 7.0.0RC5, there is a bug in phpdbg coverage report when return types are used: php/php-src@6c61286.

@milo

This comment has been minimized.

Copy link
Member

milo commented Oct 3, 2015

👍

Some Tester's tests are failing for me, I thing we should wait for final 7.0.0. In meantime:

  • is the ZendPhpDbgInterpreter needed? The only difference is the -qrr arg
  • I'm thinking about PhpInterpreter::hasXdebug() renaming, it's needed for coverage only
  • is the Coverage::STRATEGY_AUTO needed? Seems better to me send the strategy explicitly somehow from runner
@JanTvrdik

This comment has been minimized.

Copy link
Contributor Author

JanTvrdik commented Oct 3, 2015

Some Tester's tests are failing for me

You need RC4 to be able to run phpdbg and RC5 to make all tests OK.

The only difference is the -qrr arg

No. clipboard01

I'm thinking about PhpInterpreter::hasXdebug() renaming

Or removing entirely. It's currently not used anywhere.

Seems better to me send the strategy explicitly somehow from runner

Runner may use different interpreter than jobs.

It's annoying to generate coverage now, because you need to write two long arguments (coverage + coverage-src). Feel free to add coverarage-strategy but the parameter must be optional. The autodetection is simple a will work OK for 99.9 % of people.

@JanTvrdik

This comment has been minimized.

Copy link
Contributor Author

JanTvrdik commented Oct 3, 2015

Thinking about it – it will probably be even better to split ZendPhpInterpreter to ZendPhpCgiInterpreter and ZendPhpCliInterpreter.

@milo

This comment has been minimized.

Copy link
Member

milo commented Oct 3, 2015

Some Tester's tests are failing for me

You need RC4 to be able to run phpdbg and RC5 to make all tests OK.

Oh, I cloned from GitHub, there is no RC5 yet.

The only difference is the -qrr arg

No.

Hmm, --version works for both.

Seems better to me send the strategy explicitly somehow from runner

Runner may use different interpreter than jobs.

Can be detected in an /info.php way.

@JanTvrdik JanTvrdik force-pushed the JanTvrdik:phpdbg branch from 5a6f7d3 to b0a2c60 Oct 15, 2015
@Majkl578

This comment has been minimized.

Copy link
Contributor

Majkl578 commented Oct 16, 2015

7.0.0 RC5 is out.

@hrach

This comment has been minimized.

Copy link

hrach commented Nov 9, 2015

👍 Is there any drawback/issue that needs to be solved?

@milo

This comment has been minimized.

Copy link
Member

milo commented Nov 9, 2015

Waiting for stable 7 at least.

@JanTvrdik

This comment has been minimized.

Copy link
Contributor Author

JanTvrdik commented Nov 9, 2015

If anybody is willing to help – download the PR and run tests against latest PHP 7, last time I checked this one test was failing. Unfortunately I did not have the time to determine whether it is bug in tester or PHP 7.

@hrach

This comment has been minimized.

Copy link

hrach commented Nov 9, 2015

Waiting for stable 7 at least.

Well, I do not want to push on merging a feature with failing tests, however, I see no reason not to merge a feature for an unstable version of php into development branch. Nette is full of support for php7.

@Majkl578

This comment has been minimized.

Copy link
Contributor

Majkl578 commented Nov 9, 2015

however, I see no reason not to merge a feature for an unstable version of php into development branch

Definitely 👍, that argument makes no sense to me.

Btw. PHP 7 release is postponed at least until the end of november.

@milo

This comment has been minimized.

Copy link
Member

milo commented Nov 9, 2015

RC5 was not failling, later is failing. I don't want to invest my time into bug hunting. It's not only about Tester code, but compiling next RC and setup environment... that's time consuming.

That's the reason why I'm waiting for stable release.

@JanTvrdik

This comment has been minimized.

Copy link
Contributor Author

JanTvrdik commented Nov 9, 2015

I don't want to invest my time into bug hunting.

Ehm… now you can fix phpdbg before 7.0.0 released.

why I'm waiting for stable release

It will not start working unless someone will make it work. Waiting is pointless. The situation will only get worse.

@milo

This comment has been minimized.

Copy link
Member

milo commented Nov 9, 2015

@JanTvrdik Please, delete the vulgar words.

Anyway, if you promise me that you will care about phpdbg and you will solve reported bugs about it, I'll invest the time into it.

@hrach

This comment has been minimized.

Copy link

hrach commented Nov 9, 2015

I didn't want to bring emotions here. Still, the "at least" seems to be restrictive and impropper. :) That's all.

@Majkl578

This comment has been minimized.

Copy link
Contributor

Majkl578 commented Nov 9, 2015

@milo: Although I understand your approach which it makes sense for a library, unfortunately it's not that logical for a tool that is used to run tests. Especially when talking about php-next, which is pretty critical now as the GA is around the corner.

And in meantime, PHPUnit has PHP 7 support (incl. phpdbg) for over a month now...

@JanTvrdik

This comment has been minimized.

Copy link
Contributor Author

JanTvrdik commented Nov 9, 2015

@milo I can't promise that all bugs will be resolved before 7.0.0. I'm saying that they might be fixed. After the release you would need to learn how to time travel.

I understand your approach which it makes sense for a library

I don't. If you want your library to support PHP 7 you should always start working on that before PHP 7 is released not after. This is especially important for libraries which use low-level or uncommon APIs (such as Tester or Tracy).

@@ -13,6 +13,11 @@
*/
class Collector
{
const

This comment has been minimized.

Copy link
@dg

dg Nov 9, 2015

Member

What about two Collector classes?

This comment has been minimized.

Copy link
@JanTvrdik

JanTvrdik Nov 9, 2015

Author Contributor

There is some common code, so you would need three classes – one for each strategy and one base class. The logic for deciding which strategy to use would probably end-up extracted outside of those classes in \Tester\Environment::setupCoverage.

To summarize – it would require more code but should be possible.

@Mikulas

This comment has been minimized.

Copy link
Contributor

Mikulas commented Dec 6, 2015

Ok so the tests fail on 7.0.0 as well.

Stderr: Job::getErrorOutput() will presumably never work, phpdbg sends (correctly) all output to stdout, even when in underlaying process prints to stderr.

  • Proposal A: skip getErrorOutput test for phpdbg.
  • Proposal B: remove getErrorOutput altogether from Job and return complete output in getOutput. Extensions of Job may implement another interface with getStandardOutput and getErrorOutput

Ansi codes in output could be fixed with this patch

src/Runner/ZendPhpDbgInterpreter.php:41
+ fwrite($pipes[0], "set colors 0\n");
+
@Mikulas

This comment has been minimized.

Copy link
Contributor

Mikulas commented Dec 8, 2015

Stderr: @JanTvrdik found it's actually fixed on php-7.0.1RC1 php/php-src@c1189ec

Ansi codes: alternative fix for noninteractive mode which is what Job invokes

src/Runner/ZendPhpDbgInterpreter.php:63
-       return $this->path . ' -qrr ' . $this->arguments;
+       return $this->path . ' -qrrb ' . $this->arguments;

Other than those bugs, are there any issues that would prevent getting this merged?

@JanTvrdik JanTvrdik force-pushed the JanTvrdik:phpdbg branch 2 times, most recently from a25ae37 to 8e0f3b3 Dec 9, 2015
@JanTvrdik

This comment has been minimized.

Copy link
Contributor Author

JanTvrdik commented Dec 9, 2015

PING

@dg

This comment has been minimized.

Copy link
Member

dg commented Jan 22, 2016

What about this? dg@e0a9875

@Majkl578

This comment has been minimized.

Copy link
Contributor

Majkl578 commented Jan 22, 2016

Just a note: you can be running PHPDBG SAPI while having XDebug enabled as well, but in that case XDebug is not what we want to use, ever, right?

@milo

This comment has been minimized.

Copy link
Member

milo commented Jan 25, 2016

@dg Seems better.
@JanTvrdik Some attitude to this?

@hrach

This comment has been minimized.

Copy link

hrach commented Jan 25, 2016

@milo why it seems better? It seems as a spagtheti code, badly testable, wired, ...

@dg

This comment has been minimized.

Copy link
Member

dg commented Jan 25, 2016

It is only simplified, with fixed exception message. Still not sure about this #258 (comment)

@JanTvrdik

This comment has been minimized.

Copy link
Contributor Author

JanTvrdik commented Jan 25, 2016

  • yes, there should be check for PHP 7.0.0+; phpdbg in PHP 5.6 does not support code coverage
  • with @dg's refactoring it would be difficult to implement command line switch to manually force specific code coverage strategy, so it really depends on whether we see --coverage-strategy xdebug as useful or not
@milo

This comment has been minimized.

Copy link
Member

milo commented Jan 25, 2016

I don't see the reason for choosing the coverage strategy manually for now and I would wait for real-world issue. In that case, David's simplification seems OK to me.

@JanTvrdik Would you add the version check and such simplifications?

@JanTvrdik JanTvrdik force-pushed the JanTvrdik:phpdbg branch from 6a538b9 to ab20fd5 Jan 25, 2016
@JanTvrdik

This comment has been minimized.

Copy link
Contributor Author

JanTvrdik commented Jan 26, 2016

@milo done

@Mikulas

This comment has been minimized.

Copy link

Mikulas commented on src/Runner/ZendPhpDbgInterpreter.php in ee846c7 Jan 26, 2016

Wasn't that stderr fix released in 7.0.1 or something later? Just making sure you considered that, it should probably be allowed anyway.

@@ -228,9 +230,6 @@ private function createRunner()
/** @return string */
private function prepareCodeCoverage()
{
if (!$this->interpreter->hasXdebug()) {
throw new \Exception("Code coverage functionality requires Xdebug extension (used {$this->interpreter->getCommandLine()})");
}

This comment has been minimized.

Copy link
@milo

milo Jan 27, 2016

Member

Imho, this should stay there with modified message. And as a workaround, ZendPhpDbgInterpreter::hasXDebug() should return TRUE.

This comment has been minimized.

Copy link
@Majkl578

Majkl578 Jan 27, 2016

Contributor

That doesn't make any sense. PHPDBG may or may not have XDebug loaded, just like normal CLI/CGI SAPI.

This comment has been minimized.

Copy link
@milo

milo Jan 27, 2016

Member

The ZendPhpDbgInterpreter::hasXDebug() is used only for detection that Coverage is possible. I have a plan for refactoring.

This comment has been minimized.

Copy link
@milo

milo Jan 27, 2016

Member

@JanTvrdik Or maybe better ... && !$this->interpreter instanceof ZendPhpDbgInterpreter.

@milo

This comment has been minimized.

Copy link
Member

milo commented Jan 27, 2016

@JanTvrdik One note in code. I'm sorry I didn't noticed earlier.

@dg

This comment has been minimized.

Copy link

dg commented on src/Runner/ZendPhpDbgInterpreter.php in ee846c7 Jan 27, 2016

or TRUE?

@JanTvrdik

This comment has been minimized.

Copy link
Contributor Author

JanTvrdik commented Jan 27, 2016

Everything related to hasXDebug should be refactored, but it is outside of the scope of this PR.

@milo

This comment has been minimized.

Copy link
Member

milo commented Jan 27, 2016

Agree. I thought only to return TRUE, nothing more.

@JanTvrdik JanTvrdik force-pushed the JanTvrdik:phpdbg branch 3 times, most recently from d045331 to 038cbed Jan 30, 2016
@JanTvrdik JanTvrdik force-pushed the JanTvrdik:phpdbg branch from 038cbed to 9b62462 Jan 30, 2016
milo added a commit that referenced this pull request Jan 30, 2016
Support for phpdbg with fast code coverage
@milo milo merged commit e227a3f into nette:master Jan 30, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@milo

This comment has been minimized.

Copy link
Member

milo commented Jan 30, 2016

Thank you!

@JanTvrdik JanTvrdik deleted the JanTvrdik:phpdbg branch Jan 30, 2016
@@ -62,7 +62,7 @@ public static function setupColors()
{
self::$useColors = getenv(self::COLORS) !== FALSE
? (bool) getenv(self::COLORS)
: (PHP_SAPI === 'cli' && ((function_exists('posix_isatty') && posix_isatty(STDOUT))
: ((PHP_SAPI === 'cli' || PHP_SAPI === 'phpdbg') && ((function_exists('posix_isatty') && posix_isatty(STDOUT))

This comment has been minimized.

Copy link
@dg

dg Feb 5, 2016

Member

Is PHP_SAPI === 'phpdbg' correct? PHP_SAPI is 'cli' when argument -qrrb -S cli is used.

This comment has been minimized.

Copy link
@milo

milo Feb 5, 2016

Member

@dg Handy when called phpdbg path/to/test.phpt manually.

@fprochazka

This comment has been minimized.

Copy link
Contributor

fprochazka commented Feb 7, 2016

Awesome work guys!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.