Skip to content

PhpUnit-Bridge can't deal with @runTestsInSeparateProcesses #23003

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

Closed
paul-m opened this issue Jun 1, 2017 · 5 comments
Closed

PhpUnit-Bridge can't deal with @runTestsInSeparateProcesses #23003

paul-m opened this issue Jun 1, 2017 · 5 comments

Comments

@paul-m
Copy link
Contributor

paul-m commented Jun 1, 2017

Q A
Bug report? sort of
Feature request? maybe
BC Break report? no
RFC? yes/no
Symfony version 3.2.8

I'm working on Drupal 8 core. Drupal has adopted symfony/phpunit-bridge as a way to catch deprecations. The codebase has a lot of @trigger_error('', E_USER_DEPRECATED) type code in it.

The problem is that a lot of our PHPUnit-based tests are functional and need to run in isolated processes.

There are two problems:

  1. We can't test @expectedException in isolated tests. They always fail.

  2. Errors triggered by @trigger_error() never surface in isolated tests.

There are two reasons for this:

For 1 above, SymfonyTestsListener sets the expectation for the test run before the isolation occurs. Whatever deprecation error is emitted, it happens in another process. When it's done, the 'host' listener then finds no deprecation message so it fails the test.

For 2 above, the problem is that a) Symfony\Bridge\PhpUnit\DeprecationErrorHandler is never registered in isolation, due to the way the isolation occurs. Also, once you solve the problem of registering the error handler, it fails the test by calling exit(1). As it turns out, PHPUnit ignores the return value of the child process, so we never get the fail.

I've written a patch for Drupal that basically overcomes these limitations, but it's effectively a fork of symfony/phpunit-bridge, and it'd be much better not to do that.

You can see the patch here: https://www.drupal.org/node/2870194#comment-12111028

Is there a reason symfony/phpunit-bridge doesn't support isolated tests? I couldn't find a testing policy document to see if the limitation was arbitrary or intentional.

@nicolas-grekas
Copy link
Member

Please submit a PR. The limitation should be removed!

@paul-m
Copy link
Contributor Author

paul-m commented Jun 2, 2017

Splitting this up because it's complex and might require an upstream change for phpunit.

First, I'm working on marking isolated tests with @expectedDeprecation as risky. Progress here: master...paul-m:mark-isolated-tests-risky

Not ready for a PR because I don't know enough about the symfony phpunit script to understand how to make it pass without hanging.

@nicolas-grekas nicolas-grekas self-assigned this Jun 15, 2017
nicolas-grekas added a commit that referenced this issue Oct 9, 2017
…ethod (nicolas-grekas)

This PR was merged into the 3.3 branch.

Discussion
----------

[Bridge\PhpUnit] Fix infinite loop when running isolated method

| Q             | A
| ------------- | ---
| Branch?       | 3.3
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

May unlock #23003 (ping @paul-m)
And required to make #23711 pass.

Commits
-------

e07c2f1 [Bridge\PhpUnit] Fix infinite loop when running isolated method
@nicolas-grekas
Copy link
Member

@paul-m the hanging might have been solved in #24498, so that submitting your PR might be unlocked now, looking forward to it :)

@paul-m
Copy link
Contributor Author

paul-m commented Oct 12, 2017

Thanks. I'm a bit of a newbie to the Symfony way of doing things, so I wasn't sure if it was me missing a step or something.

It looks like a ^3.3 fix. I'm coming at this from the Drupal 8 world, where we're stuck at phpunit-bridge 3.2.*. See https://www.drupal.org/node/2882826

@nicolas-grekas
Copy link
Member

@paul-m about https://www.drupal.org/node/2882826, the bridge should work with a recent enough version of PHPunit 4.8.*

fabpot added a commit that referenced this issue Oct 13, 2017
… processes (paul-m)

This PR was merged into the 3.3 branch.

Discussion
----------

[Bridge\PhpUnit] Handle deprecations triggered in separate processes

| Q             | A
| ------------- | ---
| Branch?       | 3.3
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #23003, #16726
| License       | MIT
| Doc PR        | -

As reported in #23003, deprecations triggered in process-isolated test cases are not gathered.
This caught us already: HttpFoundation is still using deprecated code paths, but we missed them because of that issue with the bridge.

Here is the fixed output:
![capture du 2017-10-13 13-45-12](https://user-images.githubusercontent.com/243674/31544827-fe7ffee0-b01c-11e7-8020-4001735ce7a3.png)

Credits to @paul-m for working on the issue first.

Commits
-------

ca0fedd [BrowserKit] Handle deprecations triggered in insulated requests
ff379ef [Bridge\PhpUnit] Handle deprecations triggered in separate processes
@fabpot fabpot closed this as completed Oct 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants