https://www.drupal.org/pift-ci-job/432739 - we still have some fails, resolve them!

CommentFileSizeAuthor
#10 fix_tests_with_the-2790133-10.patch2.87 KBborisson_
#10 interdiff.txt540 bytesborisson_
#8 fix_tests_with_the-2790133-8.patch2.64 KBborisson_
#5 2790133-5--fix_tests.patch5.29 KBdrunken monkey
#4 2790133-3.png61.22 KBmarthinal
#4 2790133-2.png114.68 KBmarthinal
#4 2790133-1.png117.28 KBmarthinal
#2 2c41808.png88.55 KBborisson_
#2 921b616.png66.42 KBborisson_
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

borisson_ created an issue. See original summary.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer and commented
FileSize
921b616.png66.42 KB
2c41808.png88.55 KB

I figured out which search api commit introduced our failures.

it's http://cgit.drupalcode.org/search_api/commit/?h=8.x-1.x&id=921b616ff2a7c...

I added a test that did:
$this->drupalGet('admin/config/search/search-api/index/database_search_index/fields');

When I do git checkout 921b616 this is the result of the page:

The same thing with the previous commit gives a better result: git checkout 2c41808

Most of the failures we currently have are related to the "keywords" field not being indexed, and so we can't select it and create a facet out of it. This perpetuates troughout the entire tests and gives us all the fails we currently have.

Now figuring out what actually in that commit caused the keywords field to no longer be indexed.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer and commented

So, I tested this by doing git revert 921b616ff2a7ce1322dc86047619901b848ba7a4 -X theirs from the search api repo and running al the tests.

These are the results:
Commit reverted: https://gist.github.com/borisson/57e2f459cc0f0066518e80e47012fac2
With a fresh checkout: https://gist.github.com/borisson/09c296d707b76cb69f52f47b4056e3d0

So there are still other failures, but there's a lot less of 'm. So now we should figure out what exactly in the commit caused the keywords field to not be index anymore.

marthinal’s picture

marthinal CreditAttribution: marthinal commented
Priority: Normal » Critical
FileSize
2790133-1.png117.28 KB
2790133-2.png114.68 KB
2790133-3.png61.22 KB

I think I found where is the problem:

http://cgit.drupalcode.org/search_api/commit/?h=8.x-1.x&id=921b616ff2a7c...

+      if (!Utility::retrieveNestedProperty($properties[$datasource_id], $field->getPropertyPath())) {
+        $this->removeField($field_id);
+      }

Here the fields we're removing:

And here the fields we're indexing:

So yep, makes sense:

IMHO we need to change the way we're adding the fields to the index and we need to understand the correct way. Probably we can reuse code from search API tests to verify the correct way and how to fix our problem.

IMHO this is critical because tests are broken.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey as a volunteer commented
Status: Active » Needs review
FileSize
2790133-5--fix_tests.patch5.29 KB

The problem seems to be one we also had to solve in the Search API: namely that it now suddenly matters, that the bundles we're using in the tests aren't properly defined at the point when the index is created.
The attached patch should solve this, with it only fails in \Drupal\facets\Tests\IntegrationTest::testBlockView() remain.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer and commented

Like @drunken monkey said, this helped for almost all tests, we still need to resolve the \Drupal\facets\Tests\IntegrationTest::testBlockView() test, it also looks like we have a bunch of fails on php5.x that don't happen on php7, so we need to figure those out as well.

https://www.drupal.org/pift-ci-job/437913
https://www.drupal.org/pift-ci-job/437917

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer and commented
FileSize
fix_tests_with_the-2790133-8.patch2.64 KB

The fail in the javascript test was quite easy to resolve. The fail in the block test is still a mystery.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer and commented

I think the search api query is not executed for blocks. Or at least it looks like that, I have no idea how to resolve that. I also don't see any tests in Search API that use a block display either to confirm or deny that suspicion.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer and commented
FileSize
interdiff.txt540 bytes
fix_tests_with_the-2790133-10.patch2.87 KB

Looks like that also still fails in search api: #2792569: Views with a block display do not display results, so I guess we can comment the test for now?

  • borisson_ committed ea6a7de on 8.x-1.x 
    Issue #2790133 by borisson_, drunken monkey: Fix tests with the newest...
borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer and commented

Pushed #10, this should lead to green tests, at least for php7.

The last submitted patch, 5: 2790133-5--fix_tests.patch, failed testing.

The last submitted patch, 8: fix_tests_with_the-2790133-8.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 10: fix_tests_with_the-2790133-10.patch, failed testing.

borisson_’s picture

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

Closing this issue, as php7-tests are now green.

Status: Fixed » Closed (fixed)

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

The last submitted patch, 5: 2790133-5--fix_tests.patch, failed testing.

The last submitted patch, 8: fix_tests_with_the-2790133-8.patch, failed testing.

Status: Closed (fixed) » Needs work

The last submitted patch, 10: fix_tests_with_the-2790133-10.patch, failed testing.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer and commented
Status: Needs work » Closed (works as designed)

Was already committed, bad testbot.