Skip to content

[TEST] add PHPStan to build #628

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

Merged
merged 5 commits into from
Sep 12, 2017
Merged

Conversation

mhujer
Copy link
Contributor

@mhujer mhujer commented Aug 23, 2017

I suggest adding PHPStan as a part of the build (for the tests now, for the src later). It is a tool that statically analyzes PHP code and detects potential issues - such as missing required parameters for methods, incorrect parameter types and many others.

We've been using it at the company I'm working at since December and it helped us to catch various issues early in development phase.

--

This PR depends on #627 (which must be merged first to prevent conflicts)

@@ -171,7 +171,7 @@ public function testAsyncIntegration($testProcedure, $skip, $setupProcedure, $fi
}

if (null !== $setupProcedure) {
$this->processProcedure(current($setupProcedure), 'setup');
$this->processProcedure(current($setupProcedure), 'setup', $fileName);
Copy link
Contributor Author

@mhujer mhujer Aug 23, 2017

Choose a reason for hiding this comment

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

This one is related to recent change 51b9b9b - and it would be discovered immediately

@@ -328,6 +328,9 @@ public function operationDo($operation, $lastOperationResult, &$context, $testNa
$endpointParams->client['headers'] = $headers;
}

if (!is_string($method)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It may not be a string at this point and $this->mapEndpoint would fail, hence the added check.

@polyfractal
Copy link
Contributor

I like it! I'm all for static type checking and analytical lints. After working heavily in Rust and Java, coming back to PHP is painful without a compiler watching my back :)

@mhujer
Copy link
Contributor Author

mhujer commented Aug 24, 2017

I plan to do the similar clean up to the src directory (tests were just the beginning).

Cool thing about PHPStan is that is has something called "levels". You start by checking and fixing on the first one (which reports the most obvious errors) and gradually level-up :-)

@polyfractal polyfractal merged commit 553b433 into elastic:master Sep 12, 2017
@polyfractal
Copy link
Contributor

Static checking activate!

@mhujer mhujer deleted the mh-phpstan-tests branch September 12, 2017 14:43
@mhujer
Copy link
Contributor Author

mhujer commented Sep 12, 2017

😄

@mhujer
Copy link
Contributor Author

mhujer commented Sep 12, 2017

For some reason there was a bit of code duplicated in the merge. I've removed it in 6980f63 (as a first commit part in #638)

polyfractal pushed a commit that referenced this pull request Nov 14, 2017
* PHPUnit 5.7.21 + update config + fix risky tests

* PHPUnit 6.3.0

* use $this->expectException instead of annotation

* [TEST] fixes of issues detected by phpstan

* phpstan setup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants