Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
If we move the parts that are independent from the backend technology into the base class we have a good starting point for implementing tests for different backends like Solr or elastic search.
In addition the regression tests should be split into separate functions to support easy backend specific overwrites.
Comment | File | Size | Author |
---|---|---|---|
#15 | add_a_generic_base_for-2709951-15.patch | 89.08 KB | borisson_ |
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
@see
tags 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!