Skip to content

skip cs related tests in PHP 8 #724

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

Merged
merged 1 commit into from
Nov 2, 2020

Conversation

jrushlow
Copy link
Collaborator

skips styling tests in PHP 8 until cs tooling is available

@OskarStark
Copy link
Contributor

I would rather let them fail until PHP 8 is stable

@jrushlow
Copy link
Collaborator Author

Agreed but, cs tests block actual tests. Lesser of two evils is to skip cs based tests for now so we can test the actual code base, not the styling of the code. This of course will get reverted when cs tooling is 8x compatible.

@OskarStark
Copy link
Contributor

I thought we did in #715 🧐

cc @weaverryan

@weaverryan
Copy link
Member

I thought we did in #715 🧐

The difference is that we're going to skip them completely now, instead of running them and telling php-cs-fixer to stop complaining that we're using it on an unsupported version. The reason for that change is that @jrushlow in #725 adds support for php 8 attributes. That forces us to skip php-cs-fixer entirely, because I believe it will explode on the syntax or at least think it's bad cs (but I'm just assuming, I could be wrong).

@jrushlow it is true that we should "undo" the parts in #715 in the CI that set the PHP_CS_FIXER_IGNORE_ENV env var for php 8 - otherwise we have 2 things that basically do the same thing.

@OskarStark
Copy link
Contributor

@jrushlow can you try to remove the env from CI ?

@jrushlow
Copy link
Collaborator Author

jrushlow commented Nov 2, 2020

@weaverryan @OskarStark yes and yes.

We could leave the CI code in place for that, but ultimately we are not going to need to it with the conditional in the test suite it self to skip cs.

For reference to self in the future - when running the test suite locally w/o this PR:
1 of many on PHP8 RC3

1) Symfony\Bundle\MakerBundle\Tests\Maker\MakeControllerTest::testExecute with data set "controller_basic" (Symfony\Bundle\MakerBundle\Test\MakerTestDetails Object (...))
File 'src/Controller/FooBarController.php' has a php-cs problem: PHP needs to be a minimum version of PHP 5.6.0 and maximum version of PHP 7.4.*.

@weaverryan
Copy link
Member

Thanks Jesse!

@weaverryan weaverryan merged commit 35c5c77 into symfony:master Nov 2, 2020
@jrushlow jrushlow deleted the tests/skip-cs-8 branch November 9, 2020 12:46
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.

4 participants