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.

Sign up for GitHub

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

Jump to bottom

Always use a template file for in process tests for much better DX #2548

Closed
wants to merge 5 commits into from
Closed

Always use a template file for in process tests for much better DX #2548

wants to merge 5 commits into from

Conversation

dawehner
Copy link
Contributor

@dawehner dawehner commented Mar 3, 2017 •
edited

When you have test failures in subprocess tests, they can be hard to debug, as phpunit
uses a piped in php file by default. This makes it impossible for xdebug to inspect the file
and also requires phpunit to ship with a php-eval.php file.

As suggested by @sebastianbergmann in #2008 (comment)
we can drop some subclassing, when we require the file parameter.

Note: I removed the windows class, as the php bug mentioned in it got solved in 2014, so it should be part of php7.

As suggested by sebastian this also moves the class, removes the factory etc.

@sebastianbergmann sebastianbergmann self-assigned this Mar 3, 2017
@sebastianbergmann sebastianbergmann added type/enhancement A new idea that should be implemented type/refactoring A refactoring that should be applied to make the code easier to understand and maintain labels Mar 3, 2017
@sebastianbergmann sebastianbergmann added this to the PHPUnit 6.1 milestone Mar 3, 2017
@sebastianbergmann
Copy link
Owner

A lot of tests fail. Can you have a look? Thanks.

@dawehner
Copy link
Contributor Author

dawehner commented Mar 3, 2017

Sure, I just didn't thought about running them :)

@dawehner
Copy link
Contributor Author

dawehner commented Mar 3, 2017

I was able to fix a good amount of the failures. Let's see I can figure out the others.

Bilge
Bilge reviewed Mar 5, 2017
View reviewed changes
src/Util/PHP.php Outdated
@@ -63,7 +66,7 @@ public function __construct()
$this->runtime = new Runtime();
}

/**
/**
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the pointer! No reason of course. I fixed that.

@sebastianbergmann
Copy link
Owner

@dawehner Any progress on those remaining test failures? Thanks!

@dawehner
Copy link
Contributor Author

dawehner commented Mar 8, 2017

@sebastianbergmann Not yet sadly. I'll try to work on it maybe tonight or more realistic on the weekend. To be honest some of them at least seem to be non trivial for me.

@sebastianbergmann
Copy link
Owner

@dawehner Thank you for the update.

@sebastianbergmann
Copy link
Owner

Sorry to bother you again: any progress? Thanks!

@dawehner
Copy link
Contributor Author

Sorry :( I hope I have some time tomorrow or tonight to debug what is going on. I had hopes to pull some data out of sun when I saw him recently, but you know, sometimes that's hard.

@sebastianbergmann sebastianbergmann removed this from the PHPUnit 6.1 milestone Apr 7, 2017
@sebastianbergmann
Copy link
Owner

Any update?

@dawehner
Copy link
Contributor Author

dawehner commented Jul 4, 2017 via email

@sebastianbergmann
Copy link
Owner

@dawehner Damn! I totally forgot to talk to you about this in Vienna.

keradus
keradus reviewed Oct 15, 2017
View reviewed changes
src/Util/PHP.php
*/
abstract class AbstractPhpProcess
class PHP
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Name is too generic, when you give only a name to someone, he will have on idea what to expect.
for logic in this class - maybe replace most of it with symfony/process ?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #1342. But that should not happen before #2015.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 for switching to process component in separated PR/issue.
Yet, still, PHP is way too generic and meaningless name, it shall be modified

keradus
keradus reviewed Oct 16, 2017
View reviewed changes
src/Runner/PhptTestCase.php
@@ -66,7 +66,7 @@ class PhptTestCase implements Test, SelfDescribing
* Constructs a test case with the given filename.
*
* @param string $filename
* @param AbstractPhpProcess $phpUtil
* @param \PHPUnit\Util\PHP $phpUtil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changing param type for regular method is BC breaker.
@sebastianbergmann , do you treat changing __construct params as BC breaker ? I know some do, some don;t

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good question!

keradus
keradus reviewed Oct 16, 2017
View reviewed changes
src/Util/PHP/WindowsPhpProcess.php
*
* @see https://bugs.php.net/bug.php?id=51800
*/
class WindowsPhpProcess extends DefaultPhpProcess
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now my code depending on this public class will stop working :(

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just because a class is public does not mean that is part of the public API of a package.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

damn, without any indicator that it's not part of public API, it's hard to know that :(
so one could rely on it

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. But should I really have to add @internal to almost all classes?

Copy link
Contributor

@keradus keradus Oct 16, 2017 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are 2 ways, either add @internal to non-public, or @api to public.
Without this info, user cannot know that it's internal, so he assumes it's open to use and follows SEMVER.

note: Usually, I mark most classes with @internal and I'm making most classes final (so non-internal got final), leaving only few of them open to extend via inheritance.

@dawehner
Copy link
Contributor Author

@sebastianbergmann
Oh damnit indeed. I cannot promise anything to be honest, there are just so many other things.
I think in terms of debuggability it would be still a great improvement, but I understand if you would close the issue as noone actively works on it.

@sebastianbergmann
Copy link
Owner

This branch has conflicts that must be resolved, please send a new pull request when you're ready.

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

@sebastianbergmann sebastianbergmann sebastianbergmann left review comments

@Bilge Bilge Bilge left review comments

@keradus keradus keradus left review comments

Assignees

@sebastianbergmann sebastianbergmann

Labels
type/enhancement A new idea that should be implemented type/refactoring A refactoring that should be applied to make the code easier to understand and maintain
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@dawehner @sebastianbergmann @Bilge @keradus