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.

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

[5.5] Switch to PHPUnit 6 #17755

Merged
merged 1 commit into from
Feb 9, 2017
Merged

[5.5] Switch to PHPUnit 6 #17755

merged 1 commit into from
Feb 9, 2017

Conversation

tillkruss
Copy link
Contributor

Switch PHPUnit from 5.7 to 6.0.

@taylorotwell
Copy link
Member

Looks like lots of namespace related stuff will have to be fixed.

@tillkruss
Copy link
Contributor Author

Yeah, working on that now.

@tillkruss tillkruss force-pushed the phpunit-6 branch 3 times, most recently from 380e301 to 0608845 Compare February 3, 2017 17:38
@GrahamCampbell
Copy link
Member

All the changes made to the src folder should go to Laravel 5.4, so it can be used with the new version of phpunit too.

@GrahamCampbell
Copy link
Member

Infact, specifically, the changes also need to be updated to work with the old version too.

@tillkruss
Copy link
Contributor Author

How would you do that?

  • PHPUnit 5.6 uses PHPUnit_Framework_Error_Notice.
  • PHPUnit 6.0 uses PHPUnit\Framework\Error\Notice.

@GrahamCampbell
Copy link
Member

Those things don't matter in our own test suite since we're only using phpunit 6, but changes in our src matter. You'll have to add class exists checks.

@tillkruss
Copy link
Contributor Author

Sorry, I still don't get it. You mean make the extends XXX conditional with a class_exists() check?

Feel free to edit the PR.

@GrahamCampbell
Copy link
Member

Gah, we should revert the other phpunit changes we made on 5.4 then, and say we only support phpunit 5 on there.

@taylorotwell
Copy link
Member

Someone will have to coherently summarize the actual issue before I can comment.

@tillkruss
Copy link
Contributor Author

I'm not aware of any issues, but Graham probably know this better than me.

PHPUnit is PHP7+ only and we already switched from PHPUnit_Framework_TestCase to PHPUnit\Framework\TestCase in Laravel 5.4.

@crynobone
Copy link
Member

crynobone commented Feb 4, 2017

Gah, we should revert the other phpunit changes we made on 5.4 then, and say we only support phpunit 5 on there.

too late for that, Laravel 5.4 (Testing) technically only works with PHPUnit 5.7 (which introduce the namespace class in backward forward compatible structure).

@gmsantos
Copy link
Contributor

gmsantos commented Feb 4, 2017

PHPUnit 6 is not backward compatible with PHPUnit 5.*

What we have in PHPUnit 5.7 is some foward compatibility layer to ease the migration to PHPUnit 6 (that was implemented on #17058)

No need to revert nothing in Laravel 5.4, it will continue to work with PHPUnit 5.7. But the upgrade for PHPUnit 6 is only for Laravel 5.5.

@GrahamCampbell
Copy link
Member

No need to revert nothing in Laravel 5.4, it will continue to work with PHPUnit 5.7.

On laravel 5.4 we've killed phpunit 4 support for no reason.

@crynobone
Copy link
Member

crynobone commented Feb 4, 2017

On laravel 5.4 we've killed phpunit 4 support for no reason.

If you don't have any intention to maintain and check that it's properly working with lower PHPUnit (have different metric on Travis CI to test against 4.8) why even bother. Last I test against lowest, anything lower than 4.8 doesn't work with even earlier version of Laravel (5.3.x if I remember correctly).

Laravel 5.3 even requires min PHPUnit 5.4 and above https://packagist.org/packages/laravel/framework#v5.3.0

@GrahamCampbell
Copy link
Member

@crynobone No, I'm referring to people using the framework test classes, NOT what version of phpunit the framework uses to test itself.

@crynobone
Copy link
Member

crynobone commented Feb 4, 2017

No, I'm referring to people using the framework test classes, NOT what version of phpunit the framework uses to test itself.

I AM TALKING about this, I do maintain Testbench and even I can't really guarantee with Laravel 5.3 Testing Classes would work with other PHPUnit ~4.8 and ~5.4 regardless of what I put in the suggestion.

@GrahamCampbell
Copy link
Member

I do maintain Testbench and even I can't really guarantee with Laravel 5.3 Testing Classes would work with other PHPUnit ~4.8 and ~5.4 regardless of what I put in the suggestion.

Lol, well, just because your package doesn't work with phpunit 4.8, it doesn't mean most of laravel's core testing classes don't work with it. Infact, I'm pretty sure they do work fine.

Anyway, that got savage quickly. :trollface:

@crynobone
Copy link
Member

Lol, well, just because your package doesn't work with phpunit 4.8.

with other than* PHPUnit ~4.8 and ~5.4. It does work with 4.8, but nothing less.

Anyway, that got savage quickly.

Yes, master @GrahamCampbell, boss always win.

@taylorotwell
Copy link
Member

taylorotwell commented Feb 4, 2017

So what's the answer here? Just let Laravel 5.4 ride on PHPUnit 5.7 and then bring in PHPUnit 6 with Laravel 5.5? I'm fine with that if so.

@GrahamCampbell
Copy link
Member

So what's the answer here? Just let Laravel 5.4 ride on PHPUnit 5.7 and then bring in PHPUnit 6 with Laravel 5.5? I'm fine with that if so.

Yeh, sounds fine. 👍

@tillkruss
Copy link
Contributor Author

Thanks for all the input guys!

@tillkruss
Copy link
Contributor Author

tillkruss commented Feb 5, 2017

Sorry, why was this closed? The PR was targeted to 5.5.

@GrahamCampbell
Copy link
Member

Not sure.

@taylorotwell taylorotwell reopened this Feb 8, 2017
@taylorotwell taylorotwell merged commit f48aed2 into laravel:master Feb 9, 2017
@tillkruss tillkruss deleted the phpunit-6 branch April 16, 2017 18:06
@crynobone crynobone mentioned this pull request Feb 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants