-
Notifications
You must be signed in to change notification settings - Fork 507
Validate usages of assert*-methods in TypeInferenceTestCase #2326
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
Conversation
This pull request has been marked as ready for review. |
There was a problem hiding this 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.
90142ce
to
4c6d0ed
Compare
@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; |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.'); |
There was a problem hiding this comment.
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.
ERROR:
at the beginning isn't needed, it's obvious when the test fails that it's an error.- "on line" 8, not "from line"
- The message should read: "Function PHPStan\Testing\assertType imported with wrong namespace SomeWrong\Namespace..."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- nearly all other error messages in TypeInferenceTestCase use the
ERROR:
prefix - stripped it - done
- 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)) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.', |
There was a problem hiding this comment.
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".
Fixed the Windows problem 2df5f87 |
Looking at it again, I think a dataProvider could be useful |
Yes :) |
c4094cb
to
9b6217d
Compare
008660d
to
92185a3
Compare
Thank you. |
as discussed in https://github.com/phpstan/phpstan-src/pull/2107/files#r1048450984
tested using