Skip to content
This repository has been archived by the owner on Oct 2, 2019. It is now read-only.
This repository has been archived by the owner on Oct 2, 2019. It is now read-only.

Update phpunit/phpunit #6

Open
buskamuza opened this issue Feb 22, 2018 · 9 comments
Open

Update phpunit/phpunit #6

buskamuza opened this issue Feb 22, 2018 · 9 comments

Comments

@buskamuza
Copy link

buskamuza commented Feb 22, 2018

Important: use 2.3-develop as a target branch.

Current: 6.2.4
Update to: 7.0.0

Known issues:

  1. \Magento\Framework\TestFramework\Unit\Listener\ReplaceObjectManager implements removed \PHPUnit\Framework\BaseTestListener . Should be replaced with \PHPUnit\Framework\TestListenerDefaultImplementation
  2. Requires dropping support of PHP 7.0 (should happen in Magento 2.3)
  3. Scalar Type Declarations and Return Type Declarations are now used where possible (as a result, the API of PHPUnit\Framework\TestListener, for instance, has changed)
    1. Magento implements \PHPUnit\Framework\TestListener in test frameworks (integration and unit)
    2. There may be more places where Magento testing framework depends on PHPUnit interfaces and their signatures should be changed
  4. Potential risk: The visibility of some methods has been changed from protected to private. Will be discovered when above issues fixed and tests are run.
@mikeymike
Copy link

I've upgraded PHPUnit and fixed the unit tests locally, I'll push up over the weekend for Travis to run integration tests etc.

I also went ahead and replaced all non namespaced uses of PHPUnit classes for their namespaced counterparts which should make future upgrades smoother.

@buskamuza
Copy link
Author

Hi @mikeymike .
Did you have a chance to proceed the work on this issue?

@mikeymike
Copy link

@buskamuza sorry I fell ill and have been afk pretty much since my last update.

I'll get this pushed up asap, I want to split my commit as it contains a lot of changes which are just in comments (for namespace changes) and I didn't want it to pollute the key changes I made

@buskamuza
Copy link
Author

buskamuza commented Apr 6, 2018

Got it. Thank you for the update!
I hope you feel better now.

@buskamuza
Copy link
Author

@mikeymike , would you mind creating a PR for what you have?

@mikeymike
Copy link

@buskamuza sorry for the delay, I tried to rebase and got stung because of the large namespace updates I made for all the PHPUnit classes. I also realise that by me doing that it would likely cause others the same pain for their branches if it got merged in, so I've chosen to remove the commit and rebase. We can always update the old style class refs in a separate PR

Here's the PR anyway, pushed up for Travis to run funtional / integration tests... magento-engcom/php-7.2-support#125

@joanhe
Copy link

joanhe commented Apr 26, 2018

Reopen this issue because it takes a lot more memory to run all unit tests after upgrade phpunit to version 7. It appears that phpunit 7 takes more than 2G memory to getTestSuite() and addTestFiles() of all CE unit tests. It only takes phpunit 6 about 384M memory to getTestSuite() and addTestFiles() of all CE unit tests. Due to this memory issue with phpunit 7, it causes b2b unit test build hang frequently. After the discussion, it is decided to postpone the upgrade of phpunit 7 for now.

@sebastianbergmann
Copy link

I am not aware of a change in PHPUnit 7 that would explain such an increase in memory usage.

@buskamuza
Copy link
Author

@sebastianbergmann , how about this https://github.com/sebastianbergmann/phpunit/blob/master/src/Framework/TestSuite.php#L104 ?

It's absent in 6.2.4 - https://github.com/sebastianbergmann/phpunit/blob/6.2.4/src/Framework/TestSuite.php

The issue with it is that an instance of \PHPUnit\Framework\TestSuite is created for every suite and if there are thousands of suites, it stores \get_declared_classes() thousands times.
I tried to remove this storage and use old approach https://github.com/sebastianbergmann/phpunit/blob/6.2.4/src/Framework/TestSuite.php#L319-L321 and tests could start. I understand that it might remove a fix this array is used for. But looks like this may be the cause of the issue.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants