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

[TEST] add PHP_CodeSniffer to build #638

Merged
merged 4 commits into from
Nov 7, 2017

Conversation

mhujer
Copy link
Contributor

@mhujer mhujer commented Aug 24, 2017

ontributors are supposed to follow PSR-2 (https://github.com/elastic/elasticsearch-php/blob/master/.github/CONTRIBUTING.md). So it would be best to check it automatically during TravisCI build.

This PR fixes the CS and adds checking of the tests as a part of the build. (I plan to the the src later)

--

This PR depends on #649

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

composer.json Outdated
@@ -36,5 +38,16 @@
"psr-4": {
"Elasticsearch\\Tests\\": "tests/Elasticsearch/Tests/"
}
},
"config": {
"sort-packages": 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.

this ensures that future changes to composer.json are sorted automatically (when calling composer require

@polyfractal
Copy link
Contributor

Looks like some conflicts popped up after I made some tweaks to the connection pool tests. Would you mind investigating, when you get a chance? No rush though :)

@mhujer
Copy link
Contributor Author

mhujer commented Sep 12, 2017

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

@mhujer
Copy link
Contributor Author

mhujer commented Sep 12, 2017

@polyfractal the build errors against ES 6.0.0-rc1 (it won't even start the cluster?). And it fails for 6.x because that already uses 6.1.0 branch.

@polyfractal
Copy link
Contributor

Urgh, i'll look at the RC1 failure in a bit... it's probably something like a change in startup configuration. That happens from time to time during the major version bump and throughout beta/RCs... the only good time to break things like startup config options are at major versions.

The 6.x failures look like an upstream problem with the test to me, but I only briefly skimmed it. Will investigate more in a bit :)

@mhujer mhujer mentioned this pull request Sep 14, 2017
@polyfractal
Copy link
Contributor

So the RC1 failure was due to a change in a script config, namely max_compilation_per_minute to max_compilation_rate (elastic/elasticsearch#26399).

I made the change, and adjusted the build script to log the elasticsearch.log when the cluster fails to start so that debugging this is a bit easier on Travis.

I'm expecting this to fail on the 5.5 build though, so we may have to remove that from the Travis matrix. We could add in logic to use different configs depending on the version being tested... but I expect more of these to pop up over time as the versions diverge. It's probably not worth the effort, considering the official stance is that the stack stays on the same version.

I'll see what travis says and make the changes in a bit :)

@polyfractal
Copy link
Contributor

Slight update: ES 6.0 is un-broken, but failing a test. ES 6.x is broken for the exact-but-opposite problem (the 6.x image still has the old setting).

We use https://esvm-props.kibana.rocks/builds to get a conveniently formatted list of build/snapshot images. This is maintained by the Kibana team and generally stays up to date. Unfortunately, Kibana is blocked by a change in core ES (elastic/elasticsearch#26429), so they've kept the snapshots at an older commit to avoid breaking a bunch of tests.

Which breaks our tests, because the image is pre- the config change. Welp.

We can either A) wait out the fix and un-sticking of the build commits, or B) work on some code to pull our own latest images. Or C) drop that config option and hope we don't trip the circuit breaker, which is not great.

Or D) find the commit from the downloaded image and use that for our tests. That might be the cleanest solution now that i think about it.

As an aside, I think the failing test on 6.0 is due to a lagging commit as well, but I haven't had time to check.

@mhujer
Copy link
Contributor Author

mhujer commented Sep 14, 2017

@polyfractal See #649 - I put together a PoC of D) variant.

@polyfractal
Copy link
Contributor

I believe this should be unblocked now that #649 merged, right?

@mhujer
Copy link
Contributor Author

mhujer commented Nov 7, 2017

Yup. Just rebased it on master.

@polyfractal
Copy link
Contributor

Static testing activate!

@polyfractal polyfractal merged commit 3b87a80 into elastic:master Nov 7, 2017
@mhujer mhujer deleted the mh-phpcs-tests branch November 7, 2017 18:22
polyfractal pushed a commit that referenced this pull request Nov 14, 2017
* [TEST] PSR-2 fixes

* [TEST] PHP_CodeSniffer check PSR-2

* sort dependencies in composer.json

It will prevent some unnecessary future merge conflicts

* fix incorrectly merged code
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