parisholley / wordpress-fantastic-elasticsearch

Improve wordpress search performance/accuracy and enable faceted search by leveraging an ElasticSearch server.
MIT License
162 stars 64 forks source link

Fix issue #84 #93

Closed feribg closed 10 years ago

feribg commented 10 years ago

Simple fix for issue #84 and add PHPStorm files to .gitignore

feribg commented 10 years ago

No clue how that build fails, i ran the tests twice and literally its impossible to fail given the change I made...

parisholley commented 10 years ago

I have played with it in awhile, but I think the issue is with the versions on travis CI. Their env has changed since I last made changes.

feribg commented 10 years ago

Are you going to look into fixing the tests for the current TravisCI env? Since its quite difficult to contribute otherwise not knowing what failed because of env and what is legit broken, or I can send you screenshots of passed tests before pull requests but...

parisholley commented 10 years ago

To be honest, I don't really have any time to maintain this project at the moment, I wish I did. That being said, when you fork this repo, you should have a travis CI build available to you (or atleast you should be able to set one up with no issue), as there is a conf file in the project that tells travis how to work.

feribg commented 10 years ago

I will try to resolve, the issues are because of ES version is the latest "1.1.1" on travis, when I change it to 1.1.1 for the tests it fails with the same errors.

feribg commented 10 years ago

Fixed it to add support for elasticsearch 1.1.1 and updated all the dependencies too. It seems now that the build is passing. You can have a brief look over the test changes if you want and merge it. Actually I was thinking that removing the vendors from the repo makes sense, I dont see a reason to track them?

parisholley commented 10 years ago

I took a short glance and not sure that the changes are valid. It is important that any changes made are backwards compatible (which is why there are multiple version when tests are run). If features break between versions, the plugin needs to account for it (by either disabling the functionality, warning the user, etc).

The biggest thing is I noticed you were changing the order of some of the test values, and many of the tests are written to specifically test the order in which results are returned.

feribg commented 10 years ago

Yes, I tried to change the order of values only for tests where there isn't any specific weight to it, or ones that do not test sorting. And actually those are the ones that fail on new versions of ES, since if you put two values sequentially you are not guaranteed to get them in the same order if you query for them with no criteria (something that the prev. tests assumed). For tests where the order matters I tried to base the order on the date added (for example pagination) and not the the order of adding to the index (which in new ES is irrelevant when querying). I dont see any other way to fix those except either disregarding the order or changing all the search queries for those tests to have valid post dates and then query the ES by date to get a meaningful order, otherwise new ES will return random results on every fresh test run.

Also the oldest recommended version of ES is in the 0.90.x branch so i changed the version.sh to account for that at this point 0.20 is beyond legacy and im not sure if there is reason to support it, as it will only introduce headaches.

parisholley commented 10 years ago

Gotcha, I will review more thoroughly. Did the tests fail on the old 0.20 version you replaced in the bash script?

Also, big thank you btw for getting this green again!

feribg commented 10 years ago

No problem i hope its fine now, but if you see anything suspicious let me know for sure and i will address it. And right now in 0.20 im failing only 1 test the Indexed::testClear(); I cant seem to find the reason for that yet though. Generally the clear index case has been problematic overall.

parisholley commented 10 years ago

i've merged this into 3.0, thanks!