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] improve code quality of tests #610

Merged
merged 5 commits into from
Aug 10, 2017

Conversation

mhujer
Copy link
Contributor

@mhujer mhujer commented Aug 3, 2017

I've started improving the code quality of the tests. I'm doing it step-by-step in separate commits, so it can be easily reviewed.

I have a few other improvements in my backlog (using ::class instead of stringish classnames, adding type checks where appropriate, maybe some other things). @polyfractal do you prefer to have them all in one bigger PR or in more smaller PRs?

I plan to (eventually) do similar fixes for the src/ itself, but I wanted to start with the tests, because one does not have to worry about the BC here.

@polyfractal
Copy link
Contributor

Great, thanks! My main working laptop died last week, so I'm still re-setting up my environment. I'll try to get everything setup and review this today/tomorrow. A quick skim looks good though!

do you prefer to have them all in one bigger PR or in more smaller PRs?

Let's go with smaller PRs, they are easier to review that way and we can get stuff in incrementally.

Thanks again for this! The unit tests have definitely been neglected, I tend to rely on the yaml REST tests for the most part :)

@polyfractal
Copy link
Contributor

Also, just so you know, some of the yaml REST tests are failing for unrelated, upstream issues. So don't worry about them overly, we're waiting on fixes in core Elasticsearch.

@polyfractal
Copy link
Contributor

Merging, thanks again for this!

@polyfractal polyfractal merged commit 9ea2156 into elastic:master Aug 10, 2017
@mhujer mhujer deleted the mh-tests-fixes branch August 11, 2017 19:33
p365labs pushed a commit to p365labs/elasticsearch-php that referenced this pull request Sep 10, 2017
* [TEST] add missing namespace declarations

* [TEST] fixes

* [TEST] add strict_types declaration

* [TEST] fix invalid expectedException classes

* [TEST] remove unused uses
p365labs pushed a commit to p365labs/elasticsearch-php that referenced this pull request Sep 10, 2017
* [TEST] add missing namespace declarations

* [TEST] fixes

* [TEST] add strict_types declaration

* [TEST] fix invalid expectedException classes

* [TEST] remove unused uses
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