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

[6.x] PHPUnit 9 support #30947

Merged
merged 2 commits into from Dec 27, 2019
Merged

[6.x] PHPUnit 9 support #30947

merged 2 commits into from Dec 27, 2019

Conversation

GrahamCampbell
Copy link
Member

@GrahamCampbell GrahamCampbell commented Dec 26, 2019

Why Laravel 6 and not 7?

It is highly likely that PHPUnit 8 won't work in at least one of PHP 8.0, 8.1, 8.2, or 8.3. Since Laravel 6.0 is LTS, it is likely that we will want to continue to support newer versions of PHP, and so we need to be able to run on the latest PHPUnit.

Why use graham-campbell/testbench-core?

PHPUnit 8 deprecated assertArraySubset and Laravel ported the code over, but in PHPUnit 9, this port no longer works. Much more work is needed for PHPUnit 9, and moreover, the code for PHPUnit 9 won't work in PHPUnit 8 due to difference interfaces. Luckily, we can use my testbench core package, which handles this mess.

NB There does exist another package that backports assertArraySubset, but it appears to be abandoned already, and doesn't work on PHPUnit 9.

Why the branching code in src/Illuminate/Foundation/Testing/Assert.php?

In order to maintain BC, if people are using PHPUnit 8, we use Laravel's existing code. There are 3 cases to consider:

  1. If graham-campbell/testbench-core is installed, always use its implementation.
  2. If not installed and we are on PHPUnit 8, use Laravel's existing backport.
  3. If not installed and we are on PHPUnit 9, then we throw an exception whenever someone tries to use the method.

What to do in Laravel 7?

Perhaps we should remove Laravel's internal backport entirely, and depend on my testbench core package? This PR doesn't deal with that.

@mfn
Copy link
Contributor

mfn commented Dec 26, 2019

Just wanted to voice that I praise the forward thinking to support "at some point older Laravel version with newer dependencies", especially for PHPUnit.

I've experienced this myself that sometimes the Laravel upgrade can't be performed in a timely manner (big project, big dependencies, inferior test suite, etc.) but it's still nice to be able to use newer versions of the dependent libraries, if it's only to already prepare as much as possible before the actual upgrade.

So, thanks 👍

@driesvints
Copy link
Member

Can't we just require graham-campbell/testbench-core in the "require" dependencies and remove the if statement?

@GrahamCampbell
Copy link
Member Author

Can't we just require graham-campbell/testbench-core in the "require" dependencies and remove the if statement?

I was planning on adding this to the default laravel/laravel require-dev in 7.x.

@GrahamCampbell
Copy link
Member Author

Can't we just require graham-campbell/testbench-core in the "require" dependencies and remove the if statement?

The other problem with doing this is that people end up with testbench-core when they didn't want dev deps.

@driesvints
Copy link
Member

@GrahamCampbell but your package has 0 dependencies and just a handful of classes? I can't see how that would be a bad thing.

@GrahamCampbell
Copy link
Member Author

My package does actually depend on phpunit. It just isn't listed as an explicit dependency, to account for people possibly wanting phpunit as a phar. I wouldn't recommend loading my package without phpunit, otherwise autoloading some of the classes will crash the interpreter.

@GrahamCampbell
Copy link
Member Author

In particular, I'd expect it to cause problems with preloading.

@GrahamCampbell
Copy link
Member Author

TBH, I think it's fine not to require it. Like, we don't require mockery or phpunit within the framework. :)

@bakerkretzmar
Copy link
Contributor

@GrahamCampbell just to clarify, does this change mean that with PHPUnit 9 we should always import Illuminate\Foundation\Testing\Assert and call Assert::assertArraySubset?

I upgraded to PHPUnit 9 and installed graham-campbell/testbench-core and that works, but $this->assertArraySubset(), self::assertArraySubset(), and static::assertArraySubset() don't.

Thanks!

@GrahamCampbell
Copy link
Member Author

No, you don't need to install my testbench package anymore.

@GrahamCampbell
Copy link
Member Author

#30989 and #31113 were subsequently merged.

@GrahamCampbell
Copy link
Member Author

$this->assertArraySubset(), self::assertArraySubset(), and static::assertArraySubset() don't.

Yes, they were never intended to work.

@GrahamCampbell just to clarify, does this change mean that with PHPUnit 9 we should always import Illuminate\Foundation\Testing\Assert and call Assert::assertArraySubset?

Actually, no. Laravel has included this code actually just for its own internal use. It is intended that you change your own tests to not use this deprecated PHPUnit functionality, or stay on PHPUnit 8 if you don't want to change your code.

@GrahamCampbell
Copy link
Member Author

So, just to be clear, the laravel Assert class is internal. It should not be called by your tests directly.

@GrahamCampbell
Copy link
Member Author

This is why this change has not been mentioned in the docs. It is strictly an internal change to allow laravel's testing internals to call this code that is no longer in phpunit.

@GrahamCampbell
Copy link
Member Author

If you want to call it in your own code, you can use my testbench package, actually. My package is designed for your use case.

@GrahamCampbell
Copy link
Member Author

Directly pull in my package's trait.

@bakerkretzmar
Copy link
Contributor

@GrahamCampbell sounds good, thanks a lot!

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.

None yet

5 participants