The Solr backend contained a copy of the IntegrationTest that has been adjusted to the Solr backend.
Since #2753815: Port Simpletest web tests to BrowserTestBase tests is committed. the required base class doesn't exists anymore.
The review discovered that the old IntegartionTest in the search_api_solr was outdated anyway. From my point of view it makes sense to remove the clone from the search_api_solr backend and to apply some small changes to the the implementation in search_api to prepare it to be runable on different backends.

CommentFileSizeAuthor
#12 2828848-12--integration_test_flexible_backend.patch35.66 KBdrunken monkey
#12 2828848-12--integration_test_flexible_backend--interdiff.txt3.54 KBdrunken monkey
#8 2828848-8--integration_test_flexible_backend.patch35.66 KBdrunken monkey
#8 2828848-8--integration_test_flexible_backend--interdiff.txt4.88 KBdrunken monkey
#5 2828848_5.patch36.48 KBmkalkbrenner
#5 3-5-interdiff.txt30.57 KBmkalkbrenner
#3 2828848_3.patch5.67 KBmkalkbrenner
#3 2-3-interdiff.txt601 bytesmkalkbrenner
#2 2828848.patch6.13 KBmkalkbrenner
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mkalkbrenner created an issue. See original summary.

mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
CreditAttribution: mkalkbrenner at bio.logis Genetic Information Management GmbH for OSCE: Organization for Security and Co-operation in Europe commented
Status: Active » Needs review
FileSize
2828848.patch6.13 KB
mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
CreditAttribution: mkalkbrenner at bio.logis Genetic Information Management GmbH for OSCE: Organization for Security and Co-operation in Europe commented
FileSize
2-3-interdiff.txt601 bytes
2828848_3.patch5.67 KB

A small fix where a string must not be replaced by a variable. That just fails on different backends.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented
Status: Needs review » Needs work

I have some nits to pick, because that's how I roll - sorry markus.

Overall though, I think it's a great idea to only have to override two methods (configureBackendAndSave + indexItems) for other backends to also be able to run the same tests on those backends.

  1. +++ b/tests/src/Functional/IntegrationTest.php
@@ -231,6 +232,17 @@ class IntegrationTest extends SearchApiBrowserTestBase {
+  protected function configureBackendAndSave($edit) {

    Let's typehint on array here as well, because why not. 

  2. +++ b/tests/src/Functional/IntegrationTest.php
@@ -231,6 +232,17 @@ class IntegrationTest extends SearchApiBrowserTestBase {
+    // Nothing to condigure here for the test backend.

    /s/condigure/configure/ 

  3. +++ b/tests/src/Functional/IntegrationTest.php
@@ -231,6 +232,17 @@ class IntegrationTest extends SearchApiBrowserTestBase {
+    $this->submitForm($edit, $this->t('Save'));

    No need to wrap this in a t, see https://www.drupal.org/node/2783189#t

mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
CreditAttribution: mkalkbrenner at bio.logis Genetic Information Management GmbH for OSCE: Organization for Security and Co-operation in Europe commented
Status: Needs work » Needs review
FileSize
3-5-interdiff.txt30.57 KB
2828848_5.patch36.48 KB

@borisson_: I addressed all your concerns. I thought that removing string translation is out of scope for this patch, but here we go.

But I think we should open a dedicated issue to remove the StringTranslationTrait from the SearchApiBrowserTestBase and to adjust all other tests.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented
Status: Needs review » Reviewed & tested by the community

@mkalkbrenner oh yeah, it's definitly out of scope to do that for the entire file, but there was only 1 line (the one I quoted) added with a t.

We also need a new issue for all other tests, that's true.

No need to change it back. RTBC from my point of view. Let's see what @drunken_monkey has to say.

mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
CreditAttribution: mkalkbrenner at bio.logis Genetic Information Management GmbH for OSCE: Organization for Security and Co-operation in Europe commented

Works with Solr!
https://travis-ci.org/mkalkbrenner/search_api_solr

The PHP 5 Drupal\Core\StringTranslation\StringTranslationTrait issue will be gone if we remove all the translations from the tests ;-)

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey as a volunteer commented
Status: Reviewed & tested by the community » Needs review
FileSize
2828848-8--integration_test_flexible_backend--interdiff.txt4.88 KB
2828848-8--integration_test_flexible_backend.patch35.66 KB

The t() removal is indeed very much out of scope for this issue, but I agree with Joris, no need to revert that now. Anyways, great that there is now definitive information that we shouldn't use t() even for button labels in tests – I remember a discussion some time ago about this. Can't find the issue anymore, though – so probably we do need a new one. In any case, thanks for posting this, Joris!
If we're doing the removal for that class, we should just be thorough and also eliminate FormattableMarkup (which is nothing more than an inlining of the t() call).

Apart from that, only minor nit-picks – the largest one being that you also replaced the search_api_test data type with the new property, which probably wouldn't fly with any other backend plugin ID.

Status: Needs review » Needs work

The last submitted patch, 8: 2828848-8--integration_test_flexible_backend.patch, failed testing.

mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
CreditAttribution: mkalkbrenner at bio.logis Genetic Information Management GmbH for OSCE: Organization for Security and Co-operation in Europe commented 
+++ b/tests/src/Functional/IntegrationTest.php
@@ -801,7 +800,7 @@ protected function addFieldsToIndex() {
-      'fields[revision_log][type]' => $this->serverBackend,
+      'fields[revision_log][type]' => 'search_api_test',

That was just an accident due to the identical name, just like in 2-3-interdiff.txt. I guess you're right :-)

mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
CreditAttribution: mkalkbrenner at bio.logis Genetic Information Management GmbH for OSCE: Organization for Security and Co-operation in Europe commented

If we're doing the removal for that class, we should just be thorough and also eliminate FormattableMarkup (which is nothing more than an inlining of the t() call).

That's not true. FormattableMarkup::__toString() calls FormattableMarkup::placeholderFormat() which doesn't do anything else than escaping and formating the placeholders. The translation system is not triggered. I think you should go back to my patch and it will pass again.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey as a volunteer commented
Status: Needs work » Needs review
FileSize
2828848-12--integration_test_flexible_backend--interdiff.txt3.54 KB
2828848-12--integration_test_flexible_backend.patch35.66 KB

Ah, of course, sorry! Confused that with TranslatableMarkup … The name should have been a hint.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Woo

mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
CreditAttribution: mkalkbrenner at bio.logis Genetic Information Management GmbH for OSCE: Organization for Security and Co-operation in Europe commented

The derived tests are passing on the Solr backend :-)
RTBC from my site as well.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey as a volunteer commented
Status: Reviewed & tested by the community » Fixed

OK, great to hear.
Committed.
Thanks again, both of you!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.