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

Error in TypeInferenceTestCase when missing namespace #2148

Closed
wants to merge 4 commits into from

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Dec 27, 2022

inspired by #2147 added a error condition when test files contain named classes or functions but do not declare a namespace

catched the following error on first run (and even more on subsequent runs):

There was 1 error:

1) Error
The data provider specified for PHPStan\Analyser\NodeScopeResolverTest::testFileAsserts is invalid.
PHPUnit\Framework\AssertionFailedError: File C:\dvl\Workspace\phpstan-src-staabm\tests\PHPStan\Analyser/data/preg_split.php does not contain a namespace declaration
C:\dvl\Workspace\phpstan-src-staabm\src\Testing\TypeInferenceTestCase.php:195
C:\dvl\Workspace\phpstan-src-staabm\tests\PHPStan\Analyser\NodeScopeResolverTest.php:142

2) Error
The data provider specified for PHPStan\Analyser\NodeScopeResolverTest::testFileAsserts is invalid.
PHPUnit\Framework\AssertionFailedError: File C:\dvl\Workspace\phpstan-src-staabm\tests\PHPStan\Analyser/data/bug-1014.php does not contain a namespace declaration
C:\dvl\Workspace\phpstan-src-staabm\src\Testing\TypeInferenceTestCase.php:195
C:\dvl\Workspace\phpstan-src-staabm\tests\PHPStan\Analyser\NodeScopeResolverTest.php:154

@clxmstaab clxmstaab force-pushed the test-require-ns branch 3 times, most recently from 1c047e8 to 40cb59d Compare December 27, 2022 15:56
@staabm
Copy link
Contributor Author

staabm commented Dec 27, 2022

if this PR is accepted, we should also tackle the same errors in the phpstan extension repos, e.g. - anonymous class detection was fixed via b333809

1) Warning
The data provider specified for PHPStan\Type\Symfony\ExtensionTest::testFileAsserts is invalid.
File /home/runner/work/phpstan-src/phpstan-src/extension/tests/Type/Symfony/data/kernel_interface.php does not contain a namespace declaration

home/runner/work/phpstan-src/phpstan-src/extension/vendor/phpunit/phpunit/phpunit:60

@rajyan
Copy link
Contributor

rajyan commented Dec 29, 2022

The idea looks great to me! Is it possible to detect duplicate namespaces?

@staabm
Copy link
Contributor Author

staabm commented Dec 29, 2022

I think we could implement a phpstan collector to check for duplicate namespaces across all test files.

What I don't like with this PR is that it does not cover files e.g. included from Rule*Tests.
But maybe its still good enough for now as it discovered a lot of bugs already

@herndlm
Copy link
Contributor

herndlm commented Dec 29, 2022

something like https://github.com/slevomat/coding-standard/blob/master/doc/namespaces.md#slevomatcodingstandardnamespacesrequireonenamespaceinfile configured to run only in all test/*/data/* dirs or so could be a less aggressive alternative?

@staabm
Copy link
Contributor Author

staabm commented Dec 29, 2022

so could be a less aggressive alternative?

good idea.

what I like about this PR is, that it only enforces a namespace when you have named symbols contained.
therefore I was able to pinpoint which files are missing a namespace.

some tests might not have a namespace and adding one might imply changing a lot of test expectations (e.g. reported line numbers)

@ondrejmirtes
Copy link
Member

I don't think this is the right approach. We need something more general because:

  1. There are also RuleTestCase child classes with the same requirement.
  2. There's also LegacyNodeScopeResolverTest.

What would make more sense to me is a new workflow with a separate PHPStan config that would run PHPStan only on tests/*/data/* (which is currently excluded) and only with a custom ruleset with a few rules:

  • Every file needs to have a namespace
  • There cannot be duplicate functions and classes

Since that implementation is not going to have anything common with this PR, I'm closing it.

@ondrejmirtes
Copy link
Member

(If you want to send fixes for the discovered problems, sure, send a separate PR with no other changes :))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants