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

Run analysis with level 9, but use a lower level for tests directory #10056

Closed
ruudk opened this issue Oct 26, 2023 · 2 comments
Closed

Run analysis with level 9, but use a lower level for tests directory #10056

ruudk opened this issue Oct 26, 2023 · 2 comments

Comments

@ruudk
Copy link
Contributor

ruudk commented Oct 26, 2023

Feature request

This has already been asked at least one time, and @ondrejmirtes responded:

Hi, I’d recommend you to generate the baseline on level 6 and use PHPStan on that level for your whole codebase. You shouldn’t have the need to regenerate it, you shouldn’t allow any new errors to be added.

Also it’d definitely help you to get rid of the most frequent errors, you can lower the number of errors if you focus on the most annoying.

For the tool to be less annoying I’d recommend you to set reportUnmatchedIgnoredErrors to false. https://phpstan.org/user-guide/ignoring-errors#reporting-unused-ignores

I have no plans to allow different level per path because that’s what the baseline is for. But you can do that if you run PHPStan multiple times on multiple directories, it should be doable with a single config file. You can reuse parts of the config file with “includes”: https://phpstan.org/config-reference#multiple-files

Originally posted by @ondrejmirtes in #3640 (comment)

I agree that one can use the baseline to ignore certain errors. And we do too.

We run our project with level 9 because we want strictness in our src code that hits production. But I notice that tests now become tedious to write:

For example, this is a valid PHPunit test:

$token = new SomeToken('https://some-url');
$result = json_decode(base64_decode((string) $id, true), true, 512, JSON_THROW_ON_ERROR);

self::assertSame('https://some-url', $result['webhookEndpoint']);
self::assertIsInt($result['createdAt']);

Simple and to the point. Since the unit test passes, we know the code works.
If sometimes changes along the way, the test will fail.

But PHPStan is (rightfully) not happy:

  • Parameter #1 $json of function json_decode expects string, string|false given.
  • Cannot access offset 'webhookEndpoint' on mixed.
  • Cannot access offset 'createdAt' on mixed.

So I need to write it like this:

$token = new SomeToken('https://some-url');
$json = base64_decode((string) $token, true);
self::assertNotFalse($json); // base64_decode can return false, so we remove that

$result = json_decode($json, true, 512, JSON_THROW_ON_ERROR);

self::assertIsArray($result); // go from mixed to array
self::assertArrayHasKey('webhookEndpoint', $result); // assert the key exists
self::assertSame('https://some-url', $result['webhookEndpoint']);
self::assertArrayHasKey('createdAt', $result); // assert the key exists
self::assertIsInt($result['createdAt']);

This is just a simple example, but this really takes out the joy of writing a simple unit test.

A solution to this could be to create a separate config for the tests directory. But this leads to other issues:

  • PHPStorm doesn't understand this
  • We always have to run PHPStan twice, on a large codebase (15000+ classes) this becomes slow

Did PHPStan help you today? Did it make you happy in any way?

I interact with PHPStan whole day, and it helps me spot so many potential problems that I couldn't have spotted with just unit testing 💙

@ruudk ruudk changed the title Run analysis with level 9, but use a lower level for tests Run analysis with level 9, but use a lower level for tests directory Oct 27, 2023
@ondrejmirtes
Copy link
Member

I understand this problem but trying to modify PHPStan to use different level for each directory isn't really architecturally possible at this point. These config parameters are set once when PHPStan launches and are part of the whole assembled object hierarchy and part of rule class constructors.

But I think you can easily solve this problem and make writing unit tests joyful again. What about adding this method to your AbstractTestCase that you can extend all your test case classes from?

/** @return array<mixed> */
public function decodeJsonToArray(string $json): array
{
    $result = json_decode(base64_decode((string) $id, true), true, 512, JSON_THROW_ON_ERROR);
    self::assertIsArray($result);
    return $result;
}

That should help you with the repetition at least a little bit.

Another option is to map JSON arrays into DTOs. I'd do that if you have similar challenges not only in your tests/, but also in src/. https://github.com/CuyZ/Valinor is a great option if you want to tackle level 9 in your code.

Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants