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

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

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

Comments

Projects
None yet
5 participants
@paul-m
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

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Jun 1, 2017

Member

Please submit a PR. The limitation should be removed!

Member

nicolas-grekas commented Jun 1, 2017

Please submit a PR. The limitation should be removed!

@paul-m

This comment has been minimized.

Show comment
Hide comment
@paul-m

paul-m Jun 2, 2017

Contributor

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.

Contributor

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

bug #24498 [Bridge\PhpUnit] Fix infinite loop when running isolated m…
…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

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Oct 9, 2017

Member

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

Member

nicolas-grekas commented Oct 9, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@paul-m

paul-m Oct 12, 2017

Contributor

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

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Oct 13, 2017

Member

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

Member

nicolas-grekas commented Oct 13, 2017

@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

bug #24548 [Bridge\PhpUnit] Handle deprecations triggered in separate…
… 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 Oct 13, 2017

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