Skip to content

Conversation

@ruudk
Copy link
Contributor

@ruudk ruudk commented Mar 16, 2021

Trying to fix phpstan/phpstan#4650

Let's see if this one test fails, and then try to fix it.

@ondrejmirtes
Copy link
Member

You can verify that locally with vendor/bin/phing tests-fast, no need to go through CI each time :) See the README for more commands like that.

@ruudk
Copy link
Contributor Author

ruudk commented Mar 16, 2021

I was trying to test it locally but a lot of the tests failed for some reason. Will try vendor/bin/phing tests-fast. thanks.

@ondrejmirtes
Copy link
Member

That also shouldn't happen :) It's all green on my machine.

@ruudk
Copy link
Contributor Author

ruudk commented Mar 16, 2021

Interesting, doing another composer install solved it. Green now. But it should fail :D

@ruudk
Copy link
Contributor Author

ruudk commented Mar 16, 2021

I feel that this needs more tests to make sure it works as expected. I want to add them, but don't know where.

@ondrejmirtes
Copy link
Member

  1. Test also the NotIdentical expression (!==)
  2. Test assertType for the same expressions in the same test.

That would be sufficient from my perspective :)

Copy link
Member

Choose a reason for hiding this comment

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

This should be false obviously

Copy link
Member

Choose a reason for hiding this comment

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

This should be true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then it fails

1) PHPStan\Analyser\NodeScopeResolverTest::testFileAsserts with data set "/Users/Ruud/opensource/phpstan-src/tests/PHPStan/Analyser/data/bug-4650.php:12" ('type', '/Users/Ruud/opensource/phpsta...50.php', PHPStan\Type\Constant\ConstantStringType Object (...), PHPStan\Type\BooleanType Object (), 12) Expected type false, got type bool in /Users/Ruud/opensource/phpstan-src/tests/PHPStan/Analyser/data/bug-4650.php on line 12. Failed asserting that two strings are identical. --- Expected +++ Actual @@ @@ -'false' +'bool' /Users/Ruud/opensource/phpstan-src/tests/PHPStan/Analyser/NodeScopeResolverTest.php:11333 2) PHPStan\Analyser\NodeScopeResolverTest::testFileAsserts with data set "/Users/Ruud/opensource/phpstan-src/tests/PHPStan/Analyser/data/bug-4650.php:14" ('type', '/Users/Ruud/opensource/phpsta...50.php', PHPStan\Type\Constant\ConstantStringType Object (...), PHPStan\Type\BooleanType Object (), 14) Expected type true, got type bool in /Users/Ruud/opensource/phpstan-src/tests/PHPStan/Analyser/data/bug-4650.php on line 14. Failed asserting that two strings are identical. --- Expected +++ Actual @@ @@ -'true' +'bool' 

It's also why I think that the test is not good enough. Shouldn't it be tested with treatPhpDocTypesAsCertain: false?

@ruudk
Copy link
Contributor Author

ruudk commented Mar 17, 2021

@ondrejmirtes Any advice for this PR?

@ondrejmirtes
Copy link
Member

I see it now - the problem is that the tested function is an anonymous function, not a named one. It works as expected in a function: https://phpstan.org/r/29929e67-5641-4a4b-8c3d-d58d65bb58d8

PHPDocs above closures are not yet supported.

It's best to structure the test in a class + method rather than function as classes can be autoloaded and you don't have to require_once the file manually. Thanks.

@VincentLanglet
Copy link
Contributor

Hi @ruudk I'm really interested by this PR, do you have some time to finish it ? :)

@ruudk
Copy link
Contributor Author

ruudk commented Mar 24, 2021

I'll try to have another look at it this week.

@ruudk
Copy link
Contributor Author

ruudk commented Apr 20, 2021

@VincentLanglet Sorry, I won't be fixing this anytime soon.

@VincentLanglet
Copy link
Contributor

@VincentLanglet Sorry, I won't be fixing this anytime soon.

Will take a look

@ondrejmirtes
Copy link
Member

I've modernized the test so that it looks how tests are currently written, but it still fails.

@ondrejmirtes ondrejmirtes force-pushed the bug-4650 branch 2 times, most recently from ca73dcb to c0b6593 Compare April 20, 2021 10:51
@ondrejmirtes
Copy link
Member

Alright, the fix is correct, but the test is failing of unrelated reasons.

@ondrejmirtes ondrejmirtes merged commit d069408 into phpstan:master Apr 20, 2021
@ondrejmirtes
Copy link
Member

Now it works :) Thanks everyone.

@ruudk
Copy link
Contributor Author

ruudk commented Apr 20, 2021

Thanks 🙌

@ruudk ruudk deleted the bug-4650 branch April 20, 2021 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants