Skip to content

Conversation

MathiasReker
Copy link

@MathiasReker MathiasReker commented Jan 28, 2019

Calls to PHPUnit\Framework\TestCase static methods must all be of the same type, either $this->, self:: or static::.

@magento-engcom-team
Copy link
Contributor

Hi @MathiasReker. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento-engcom-team give me test instance - deploy test instance based on PR changes
  • @magento-engcom-team give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

Copy link
Contributor

@orlangur orlangur left a comment

Choose a reason for hiding this comment

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

$this-> is preferable according to PHPUnit author, thanks for choosing this variant.

@MathiasReker could you please provide an automated way to apply such changes? I suppose php-cs-fixer or something was used. Also, to avoid code base inconsistency a static test similar to running phpcs should be implemented.

@MathiasReker
Copy link
Author

All my PR's are made from a custom shell script I wrote. I use different libraries and a lot of custom code.

@orlangur
Copy link
Contributor

@MathiasReker I see, thanks for quick response. We cannot process PRs with hundreds of changed files without an automated way to reproduce your changeset.

Also, as few custom code as possible must be used, or at least such changes should be separated from ones obtained using well-known tools.

Here is more details on static test which needs to be implemented: #19311 (comment) So basically we want such code issues nevermore appear in codebase.

@MathiasReker
Copy link
Author

Try to run this with php-cs-fixer: php ../php-cs-fixer.phar fix ./ --using-cache="no" --rules='{"php_unit_test_case_static_method_calls": {"call_type": "this"}}' --allow-risky="yes"

@orlangur
Copy link
Contributor

@MathiasReker great, yeah, I supposed something like that exists in php-cs-fixer :)

So let's start from static test running php-cs-fixer, update https://github.com/magento/magento2/blob/2.3-develop/.php_cs.dist with all useful rules and then update code base in a single PR.

@MathiasReker
Copy link
Author

I understand your point and I agree that it is the best way to do it. I can help you, but I can't work for free to that point. I don't use Magento, I just came by and though I could do a great work by letting my computer run a script overnight. :-)

@orlangur
Copy link
Contributor

@MathiasReker I see. Unfortunately applying code changes is not the only part of work, they should be also reviewed by somebody and enforced with some automated test to avoid regressions.

There were attempts before to apply php-cs-fixer, but without proper test they does not make a lot of sense, ref #12192 (comment)

@orlangur
Copy link
Contributor

@MathiasReker I closed PRs which seemed to be produced by php-cs-fixer to me. Those changes may be applied in a single PR as soon as we have the test to enforce changes.

@ghost
Copy link

ghost commented Jan 28, 2019

Hi @MathiasReker, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

@MathiasReker MathiasReker deleted the php-unit-test-case-static-method-calls branch January 28, 2019 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants