Skip to content

[PhpUnitBridge] Fix some errors when using serialized deprecations #31478

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
Closed
wants to merge 4 commits into from
Closed

[PhpUnitBridge] Fix some errors when using serialized deprecations #31478

wants to merge 4 commits into from

Conversation

l-vo
Copy link
Contributor

@l-vo l-vo commented May 11, 2019 •
edited
Loading

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

During the refactoring for #29211, some very minor problems seamed to appear. I created #31382 for fixing one of them but I forgot two others; sorry for that. Both problems occurs when using @runInSeparateProcess in tests too:

  • deprecation isSelf condition is inverted when using runInSeparateProcess
  • wrong trace stack is used to determine isIndirect state of the deprecation when using runInSeparateProcess

@l-vo l-vo changed the title Fix some errors when using serialized deprecations [WIP][PhpUnitBridge] Fix some errors when using serialized deprecations May 11, 2019
@l-vo l-vo mentioned this pull request May 11, 2019
Merged
7 tasks
@nicolas-grekas nicolas-grekas added this to the 4.3 milestone May 12, 2019
@l-vo l-vo force-pushed the fix_some_errors_when_using_serialized_deprecations branch 3 times, most recently from 31b4188 to 25da423 Compare May 15, 2019 19:46
@l-vo l-vo changed the title [WIP][PhpUnitBridge] Fix some errors when using serialized deprecations [PhpUnitBridge] Fix some errors when using serialized deprecations May 15, 2019
@l-vo l-vo marked this pull request as ready for review May 15, 2019 19:51
@l-vo
Copy link
Contributor Author

l-vo commented May 15, 2019 •
edited
Loading

Tests fail, I think it's because they use the PhpUnitBridge of the master branch.

@l-vo l-vo mentioned this pull request May 16, 2019
Closed
@greg0ire
Copy link
Contributor

greg0ire commented May 25, 2019 •
edited
Loading

I reproduce the build failure locally. I'm using php 7.3, what about you? Also, I'm running the tests from the directory of the component, src/Symfony/Bridge/PhpUnit

@greg0ire
Copy link
Contributor

greg0ire commented May 25, 2019 •
edited
Loading

Also, tests fail when I run them in another of the supported ways (which is not used in CI):

phpunit src/Symfony/Bridge/PhpUnit             
PHPUnit 6.5.14 by Sebastian Bergmann and contributors.

Testing src/Symfony/Bridge/PhpUnit
..........S....................FFF..EE...................S......  64 / 64 (100%)

Time: 467 ms, Memory: 10.00MB

There were 2 errors:

1) Symfony\Bridge\PhpUnit\Tests\DeprecationErrorHandler\DeprecationTest::testIsIndirectUsesRightTrace with data set #1 (false, 'a:4:{s:11:"deprecation";s:22:...it";}}', array(array('myfunc1'), array('Symfony\Bridge\PhpUnit\Legacy...rForV5', 'mymethod')))
Undefined index: function

/home/greg/dev/symfony/src/Symfony/Bridge/PhpUnit/Tests/DeprecationErrorHandler/DeprecationTest.php:151

2) Symfony\Bridge\PhpUnit\Tests\DeprecationErrorHandler\DeprecationTest::testIsIndirectUsesRightTrace with data set #2 (true, 'a:4:{s:11:"deprecation";s:22:...it";}}', array(array('myfunc1'), array('Symfony\Bridge\PhpUnit\Legacy...rForV5', 'mymethod')))
Undefined index: function

/home/greg/dev/symfony/src/Symfony/Bridge/PhpUnit/Tests/DeprecationErrorHandler/DeprecationTest.php:151

--

There were 3 failures:

1) Symfony\Bridge\PhpUnit\Tests\DeprecationErrorHandler\DeprecationTest::testIsSelf with data set #2 (true, 'a:4:{s:5:"class";s:0:"";s:6:"...:0:{}}', 'Symfony\Bridge\PhpUnit\Legacy...rForV5', '')
Failed asserting that false matches expected true.

/home/greg/dev/symfony/src/Symfony/Bridge/PhpUnit/Tests/DeprecationErrorHandler/DeprecationTest.php:101

2) Symfony\Bridge\PhpUnit\Tests\DeprecationErrorHandler\DeprecationTest::testIsSelf with data set #3 (true, 'a:5:{s:5:"class";s:0:"";s:6:"...:0:{}}', 'Symfony\Bridge\PhpUnit\Legacy...rForV5', '')
Failed asserting that false matches expected true.

/home/greg/dev/symfony/src/Symfony/Bridge/PhpUnit/Tests/DeprecationErrorHandler/DeprecationTest.php:101

3) Symfony\Bridge\PhpUnit\Tests\DeprecationErrorHandler\DeprecationTest::testIsSelf with data set #4 (false, 'a:5:{s:5:"class";s:0:"";s:6:"...:0:{}}', 'Symfony\Bridge\PhpUnit\Legacy...rForV5', '')
Failed asserting that true matches expected false.

/home/greg/dev/symfony/src/Symfony/Bridge/PhpUnit/Tests/DeprecationErrorHandler/DeprecationTest.php:101

Same with simply phpunit , but that's probably because it is mixing old and new code, might be fixed one your code is merged.

@greg0ire
Copy link
Contributor

greg0ire commented May 26, 2019 •
edited
Loading

@l-vo how do you run the test suite locally? This is how I proceed:

cd src/Symfony/Bridge/PhpUnit
phpunit

EDIT: and it fails on my machine, what about you?

@l-vo
Copy link
Contributor Author

l-vo commented May 26, 2019

@greg0ire from the repository root:

$ php phpunit src/Symfony/Bridge/PhpUnit

@greg0ire
Copy link
Contributor

Please try to make both methods work. The method that I use is used in CI. You will have to run composer update in the phpunit bridge directory first

@l-vo l-vo force-pushed the fix_some_errors_when_using_serialized_deprecations branch 8 times, most recently from a2af18a to 4149956 Compare May 28, 2019 09:14
@l-vo l-vo force-pushed the fix_some_errors_when_using_serialized_deprecations branch 16 times, most recently from c10ace7 to e9cf4df Compare June 1, 2019 18:29
@l-vo
Copy link
Contributor Author

l-vo commented Jun 1, 2019

@greg0ire I think I fixed the tests. They still fail on PHP 7.3 (deps=low); it is caused by ProcessIsolationTest which use the modified behavior @runTestsInSeparateProcesses. I don't understand what the CI exactly do but I'm almost sure it's because it uses a not up-to-date version of Deprecation.php (a version from the master branch from a vendor directory created by the CI).

@l-vo
Copy link
Contributor Author

l-vo commented Jun 1, 2019

Status: needs review

@l-vo l-vo force-pushed the fix_some_errors_when_using_serialized_deprecations branch from 31ab004 to edfdccf Compare June 2, 2019 12:51
@greg0ire greg0ire mentioned this pull request Oct 2, 2019
Merged
@l-vo
Copy link
Contributor Author

l-vo commented Oct 24, 2019

Closed in favour of #33820

@l-vo l-vo closed this Oct 24, 2019
nicolas-grekas added a commit that referenced this pull request Feb 4, 2020
@nicolas-grekas
bug #33820 [PhpUnitBridge] Fix some errors when using serialized depr…
84d32ac 
…ecations (l-vo)

This PR was submitted for the 4.3 branch but it was squashed and merged into the 4.4 branch instead.

Discussion
----------

[PhpUnitBridge] Fix some errors when using serialized deprecations

| Q             | A
| ------------- | ---
| Branch?       | 4.3
| Bug fix?      | yes
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       | n/a
| License       | MIT
| Doc PR        | n/a

This PR attempts to fix conflicts that arose in #31478

Creating as a draft for now as I think having separate test methods no longer make sense (`isSelf()` and `isIndirect()` have been replaced with `getType()`). @l-vo please review and confirm I did not loose anything valuable from your original contribution.

Commits
-------

056d598 [PhpUnitBridge] Fix some errors when using serialized deprecations
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

@greg0ire greg0ire greg0ire left review comments

Assignees
No one assigned
Labels
Bug Status: Needs Review
Projects
None yet
Milestone
4.3
Development

Successfully merging this pull request may close these issues.
4 participants
@l-vo @greg0ire @nicolas-grekas @carsonbot