Closed (fixed)
Project:
Search API
Version:
8.x-1.x-dev
Component:
Tests
Priority:
Normal
Category:
Feature request
Assigned:
Reporter:
Created:
20 Apr 2016 at 11:28 UTC
Updated:
17 May 2016 at 13:34 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
mkalkbrennerComment #3
mkalkbrennera small update to clean up the module list.
Comment #4
mkalkbrennerComment #5
mkalkbrennerI moved the entity field definitions that are required for the example content from search_api_test_db to a separate test module called search_api_test_example_content.
BackendTestBase::setUp() now installs that field definitions instead of the database specific BackendTest.
Comment #6
drunken monkeyThanks, great work so far!
Attached is a re-roll with the regression test for #2616804: An overlong word (more than 50 characters) was encountered while indexing. I also added a few missing comments and did some rewrite of test entity creation so it's more stable if some methods are overridden (and don't add any entities anymore). Could probably still be improved (i.e., not hard-code the entity IDs – should actually be pretty easy now, come to think of it).
However, the major problem is that the base class is still much too DB-centric. A lot of its methods (e.g.,
checkMultiValuedInfo(),editServerPartial(),editServerMinChars()) don't make any sense for Solr or other backends.Instead, the class should cover all the base functionality of backends and then have some clearly defined extension points where child classes can just call whatever methods they want. There, the DB backend would test changes to its specific backend options, and things like the internal DB info, while other backends might do entirely different things.
Also, the doc blocks for the regression tests don't comply to the coding standards. In the first line, I'd instead really describe what is tested (e.g., "Tests indexing of long multi-byte words.") and then only reference the issues in the extended description (or even with
@seetags and proper URLs – not sure).Comment #7
mkalkbrennerThe interdiff looks good and from my point of view the the doc blocks of the regression tests are the last remaining tasks.
I already thought about this and decided to keep them in the base class in my first patch. I currently started to implement the first tests for the search_api_solr_multilingual backend. There it is possible to configure something like min chars. And if you use a managed index these values can be modified on the fly. That's why I kept these functions in the base class.
As already stated above I recommend to improve the doc blocks and to commit the patch.
Comment #8
drunken monkeyThey are still backend-specific, even if one of them is also available in one other backend. Please either correct this, or wait until I get the time to do it myself.
Comment #9
mkalkbrennerI can work on this.
Could you name all functions you would like to be moved?
Comment #10
mkalkbrennerComment #11
mkalkbrennerI reviewed all the tests and moved db specific ones to the db BackendTest.
Comment #12
mkalkbrennerJust a small fix. Forgot to remove the call to regressionTest2511860() from the unspecific regression tests. It has been moved to the db backend specific ones already.
Comment #13
mkalkbrennerUsing the patch in #12, the Solr backend tests pass as well:
https://travis-ci.org/mkalkbrenner/search_api_solr/builds/127272521
Comment #14
mkalkbrennersearch_api_solr_multilingual passes, too
https://travis-ci.org/mkalkbrenner/search_api_solr_multilingual/builds/1...
RTBC ;-)
Comment #15
borisson_Patch is not very easy to review, but I think I it looks great, the testbot agrees here and in solr so let's get this in.
Comment #17
drunken monkeyUnbelievably, Joris missed a few comment style issues, but apart from that this indeed looks good. Thanks!
After confirming with Markus, I also renamed
regressionTestSecondServer()tocheckSecondServer().Otherwise, looks good. Committed.
Comment #18
mkalkbrennerThank you!