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

Validate usages of assert*-methods in TypeInferenceTestCase #2326

Merged
merged 5 commits into from
Apr 11, 2023

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Apr 5, 2023

as discussed in https://github.com/phpstan/phpstan-src/pull/2107/files#r1048450984

tested using

diff --git a/tests/PHPStan/Analyser/data/implode.php b/tests/PHPStan/Analyser/data/implode.php
index b2e2b9aa7..86a03a838 100644
--- a/tests/PHPStan/Analyser/data/implode.php
+++ b/tests/PHPStan/Analyser/data/implode.php
@@ -2,7 +2,8 @@

 namespace ImplodeFunctionReturn;

-use function PHPStan\Testing\assertType;
+use function PHPStan\Testing\assertNativeType;
+use function PHPStan\wrong\assertType;

 class Foo
 {
@@ -10,6 +11,8 @@ class Foo
        const ONE = 1;

        public function constants() {
+               assertNativeType('string', implode(',', [self::X, '345']));
+
                assertType("'12345'", implode(['12', '345']));

                assertType("'12345'", implode('', ['12', '345']));
➜  phpstan-src git:(pr/2118) ✗ vendor/bin/phpunit tests/PHPStan/Analyser/NodeScopeResolverTest.php
Xdebug: [Step Debug] Could not connect to debugging client. Tried: 127.0.0.1:9003 (through xdebug.client_host/xdebug.client_port) :-(
PHPUnit 9.5.23 #StandWithUkraine

Warning:       XDEBUG_MODE=coverage or xdebug.mode=coverage has to be set

E                                                                   1 / 1 (100%)

Time: 00:00.017, Memory: 54.00 MB

There was 1 error:

1) Error
The data provider specified for PHPStan\Analyser\NodeScopeResolverTest::testFileAsserts is invalid.
PHPUnit\Framework\AssertionFailedError: ERROR: Assert-Method PHPStan\wrong\assertType imported with wrong namespace called from line 16.
/Users/staabm/workspace/phpstan-src/src/Testing/TypeInferenceTestCase.php:182
/Users/staabm/workspace/phpstan-src/src/Node/ClassStatementsGatherer.php:115
/Users/staabm/workspace/phpstan-src/src/Analyser/NodeScopeResolver.php:553
/Users/staabm/workspace/phpstan-src/src/Analyser/NodeScopeResolver.php:3037
/Users/staabm/workspace/phpstan-src/src/Analyser/NodeScopeResolver.php:1751
/Users/staabm/workspace/phpstan-src/src/Analyser/NodeScopeResolver.php:618
/Users/staabm/workspace/phpstan-src/src/Analyser/NodeScopeResolver.php:294
/Users/staabm/workspace/phpstan-src/src/Analyser/NodeScopeResolver.php:569
/Users/staabm/workspace/phpstan-src/src/Analyser/NodeScopeResolver.php:294
/Users/staabm/workspace/phpstan-src/src/Analyser/NodeScopeResolver.php:668
/Users/staabm/workspace/phpstan-src/src/Analyser/NodeScopeResolver.php:294
/Users/staabm/workspace/phpstan-src/src/Analyser/NodeScopeResolver.php:638
/Users/staabm/workspace/phpstan-src/src/Analyser/NodeScopeResolver.php:249
/Users/staabm/workspace/phpstan-src/src/Testing/TypeInferenceTestCase.php:71
/Users/staabm/workspace/phpstan-src/src/Testing/TypeInferenceTestCase.php:197
/Users/staabm/workspace/phpstan-src/tests/PHPStan/Analyser/NodeScopeResolverTest.php:19

ERRORS!
Tests: 1, Assertions: 0, Errors: 1.

diff --git a/tests/PHPStan/Analyser/data/implode.php b/tests/PHPStan/Analyser/data/implode.php
index b2e2b9aa7..8e80ecead 100644
--- a/tests/PHPStan/Analyser/data/implode.php
+++ b/tests/PHPStan/Analyser/data/implode.php
@@ -2,7 +2,7 @@

 namespace ImplodeFunctionReturn;

-use function PHPStan\Testing\assertType;
+use function PHPStan\Testing\assertNativeType;

 class Foo
 {
@@ -10,6 +10,8 @@ class Foo
        const ONE = 1;

        public function constants() {
+               assertNativeType('string', implode(',', [self::X, '345']));
+
                assertType("'12345'", implode(['12', '345']));

                assertType("'12345'", implode('', ['12', '345']));
➜  phpstan-src git:(asserts) ✗ vendor/bin/phpunit tests/PHPStan/Analyser/NodeScopeResolverTest.php
PHPUnit 9.5.23 #StandWithUkraine

Warning:       No code coverage driver available

E                                                                   1 / 1 (100%)

Time: 00:00.011, Memory: 52.00 MB

There was 1 error:

1) Error
The data provider specified for PHPStan\Analyser\NodeScopeResolverTest::testFileAsserts is invalid.
PHPUnit\Framework\AssertionFailedError: ERROR: Missing import for assertType() on line 15.
/Users/staabm/workspace/phpstan-src/src/Testing/TypeInferenceTestCase.php:131
/Users/staabm/workspace/phpstan-src/src/Node/ClassStatementsGatherer.php:115
/Users/staabm/workspace/phpstan-src/src/Analyser/NodeScopeResolver.php:553
/Users/staabm/workspace/phpstan-src/src/Analyser/NodeScopeResolver.php:3037
/Users/staabm/workspace/phpstan-src/src/Analyser/NodeScopeResolver.php:1751
/Users/staabm/workspace/phpstan-src/src/Analyser/NodeScopeResolver.php:618
/Users/staabm/workspace/phpstan-src/src/Analyser/NodeScopeResolver.php:294
/Users/staabm/workspace/phpstan-src/src/Analyser/NodeScopeResolver.php:569
/Users/staabm/workspace/phpstan-src/src/Analyser/NodeScopeResolver.php:294
/Users/staabm/workspace/phpstan-src/src/Analyser/NodeScopeResolver.php:668
/Users/staabm/workspace/phpstan-src/src/Analyser/NodeScopeResolver.php:294
/Users/staabm/workspace/phpstan-src/src/Analyser/NodeScopeResolver.php:638
/Users/staabm/workspace/phpstan-src/src/Analyser/NodeScopeResolver.php:249
/Users/staabm/workspace/phpstan-src/src/Testing/TypeInferenceTestCase.php:71
/Users/staabm/workspace/phpstan-src/src/Testing/TypeInferenceTestCase.php:197
/Users/staabm/workspace/phpstan-src/tests/PHPStan/Analyser/NodeScopeResolverTest.php:19

ERRORS!
Tests: 1, Assertions: 0, Errors: 1.

@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

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

This needs a test. Please add a new class that extend TypeInferenceTestCase and do something with $this->expectException('PHPUnit\Framework\ExpectationFailedException'); there (https://stackoverflow.com/questions/3917909/how-to-indicate-that-a-phpunit-test-is-expected-to-fail). Thanks.

@clxmstaab clxmstaab force-pushed the asserts10 branch 2 times, most recently from 90142ce to 4c6d0ed Compare April 5, 2023 13:11
@staabm
Copy link
Contributor Author

staabm commented Apr 5, 2023

@herndlm looks like the new patching solution has problems on windows with php 7.2

@@ -0,0 +1,59 @@
<?php declare(strict_types = 1);

namespace PHPStan\Tests;
Copy link
Member

Choose a reason for hiding this comment

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

This namespace could be Testing.

Copy link
Contributor Author

@staabm staabm Apr 5, 2023

Choose a reason for hiding this comment

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

already existing test-classes in the same folder use PHPStan\Tests.

Testing is used in the src/ folder

Copy link
Member

Choose a reason for hiding this comment

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

But here you're testing PHPStan\Testing\TypeInferenceTestCase class.

public function testWrongAssertTypeNamespace(): void
{
$this->expectException(AssertionFailedError::class);
$this->expectExceptionMessage('ERROR: Assert-Method SomeWrong\Namespace\assertType imported with wrong namespace called from line 8.');
Copy link
Member

Choose a reason for hiding this comment

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

The error messages could be cleaned up a bit.

  1. ERROR: at the beginning isn't needed, it's obvious when the test fails that it's an error.
  2. "on line" 8, not "from line"
  3. The message should read: "Function PHPStan\Testing\assertType imported with wrong namespace SomeWrong\Namespace..."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. nearly all other error messages in TypeInferenceTestCase use the ERROR: prefix - stripped it
  2. done
  3. done

@@ -125,7 +127,13 @@ public static function gatherAssertTypes(string $file): array
}

$functionName = $nameNode->toString();
if ($functionName === 'PHPStan\\Testing\\assertType') {
if (in_array($functionName, ['assertType', 'assertNativeType', 'assertVariableCertainty'], true)) {
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 case-insensitive + please also test it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

if ($functionName === 'PHPStan\\Testing\\assertType') {
if (in_array($functionName, ['assertType', 'assertNativeType', 'assertVariableCertainty'], true)) {
self::fail(sprintf(
'ERROR: Missing import for %s() on line %d.',
Copy link
Member

Choose a reason for hiding this comment

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

It's not import but "use statement".

@ondrejmirtes
Copy link
Member

Fixed the Windows problem 2df5f87

@staabm
Copy link
Contributor Author

staabm commented Apr 6, 2023

Looking at it again, I think a dataProvider could be useful

@ondrejmirtes
Copy link
Member

Yes :)

@clxmstaab clxmstaab force-pushed the asserts10 branch 2 times, most recently from c4094cb to 9b6217d Compare April 6, 2023 07:43
@staabm staabm force-pushed the asserts10 branch 2 times, most recently from 008660d to 92185a3 Compare April 8, 2023 06:06
@ondrejmirtes ondrejmirtes merged commit a7f1fa3 into phpstan:1.10.x Apr 11, 2023
370 of 377 checks passed
@ondrejmirtes
Copy link
Member

Thank you.

@staabm staabm deleted the asserts10 branch April 11, 2023 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants