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

Assertion class as parameter for Codeception config #65

Open
wants to merge 2 commits into
base: 1.1.x
Choose a base branch
from

Conversation

Slamdunk
Copy link

Resolves #34

@@ -20,10 +20,14 @@ services:
- phpstan.typeSpecifier.functionTypeSpecifyingExtension
-
class: PHPStan\Type\PHPUnit\Assert\AssertMethodTypeSpecifyingExtension
arguments:
classWithAssertionMethods: PHPUnit\Framework\TestCase
Copy link
Author

@Slamdunk Slamdunk Mar 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've retained the class used in https://github.com/phpstan/phpstan-phpunit/pull/65/files#diff-afc1ebcbfd95543266d01a244f0e43deL27, but shouldn't this be PHPUnit\Framework\Assert too?

@ondrejmirtes
Copy link
Member

Hi, I don't know anything about Codeception. Do you typically use assert* methods from PHPUnit in conjunction with Codeception methods, or do you use either PHPUnit or Codeception?

Anyway, we could register everything by default (there's no downside) and just mention in the README that this extension works well with Codeception out of the box. WDYT?

@Slamdunk
Copy link
Author

It was my wish from the beginning, but it not feasible.

Codeception ships with 2 different types of test, the Unit one which is a subclass of PHPUnit\Framework\TestCase and already works out of the box, and the Functional/Acceptance ones. In a nutshell, Functional/Acceptance tests gather multiple classes and their methods, copy-paste their signatures into a giant class, and proxy all the calls to the original ones. This allows the user to choose which module to use in the tests (i.e. Filesystem, Redis, Symfony, FTP...) and call all the available assertions/helper-methods under the $this hat. The Assertion module is one of them and is a proxy to the PHPUnit one.

What's the blocking? The giant class created lies under the project folder tree, and of course to be correctly autoloaded it must have the project's namespace, not a generic one.

@nreynis
Copy link

nreynis commented Sep 30, 2021

I feel like this is related to #103

I don't exactly see the benefit to parametrize the assertion class instead of just fixing the FQCN. But I'm not familiar at all with codeception. Can you provide a link to that Acceptance class (or other relevant code) is it extending Assert?

I tried searching https://github.com/Codeception/phpunit-wrapper/tree/9.0/src but I didn't find it.

@Slamdunk
Copy link
Author

Can you provide a link to that Acceptance class (or other relevant code) is it extending Assert?

I doesn't exist, Codeception doesn't work this way.
It generates Traits that proxy the calls to PHPUnit's Assert class, and it generates them dynamically based on how the developer configured Codeception.

There is no way phpstan/phpstan-phpunit can fix #34 with an hardcoded code change.

@nreynis
Copy link

nreynis commented Oct 2, 2021

The thing is making the class parametrizable will fix your use case by breaking PHP Unit analysis.

I think a better approach would be to create another package dedicated to codeception, dependant on this one. This way you can have two instance of the extension working at the same time.

@Slamdunk
Copy link
Author

Slamdunk commented Oct 4, 2021

The thing is making the class parametrizable will fix your use case by breaking PHP Unit analysis.

May I ask you how this break would happen?

I think a better approach would be to create another package dedicated to codeception, dependant on this one. This way you can have two instance of the extension working at the same time.

This PR already enable to have both PHPUnit and Codeception working in the same time, where do you see this isn't the case?

@nreynis
Copy link

nreynis commented Oct 4, 2021

🤦 You are right I didn't understand the PR correctly. I somehow thought you were just feeding the existing service with another default value. But you are in fact creating other instances with different parameters.

This should work fine. 👍

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.

assertInternalType()
3 participants