As far as I can see, all our hooks are currently untested. It would be great to change that, for as many of them as possible.

We should also make sure all the hooks we call are actually documented.

Estimated Value and Story Points

This issue was identified as a Beta Blocker for Drupal 8. We sat down and figured out the value proposition and amount of work (story points) for this issue.

Value and Story points are in the scale of fibonacci. Our minimum is 1, our maximum is 21. The higher, the more value or work a certain issue has.

Value: 2
Story Points: 13

CommentFileSizeAuthor
#16 add_tests_for_the-2625264-16.patch7.99 KBborisson_
#16 interdiff.txt6.24 KBborisson_
#12 2625264-11--hook_tests.patch8.62 KBdrunken monkey
#12 2625264-11--hook_tests--interdiff.txt2.17 KBdrunken monkey
#7 2625264-7--hook_tests--interdiff.txt3.18 KBdrunken monkey
#7 2625264-7--hook_tests.patch8.95 KBdrunken monkey
#6 add_tests_for_the-2625264-6.patch9.32 KBborisson_
#6 interdiff.txt1.42 KBborisson_
#5 add_tests_for_the-2625264-4.patch9.15 KBborisson_
#5 interdiff.txt1.54 KBborisson_
#2 add_tests_for_the-2625264-2.patch8.85 KBborisson_
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

drunken monkey created an issue. See original summary.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented
Status: Active » Needs work
Issue tags: +Needs tests
FileSize
add_tests_for_the-2625264-2.patch8.85 KB

So, I spent some time in the airport and on the plane working on this patch after drupalcamp vienna,

There is no test for hook_search_api_field_type_mapping_alter. The other hooks documented in search_api.api.php have been tested.

I also noticed that the documentation for hook_search_api_data_type_info_alter was incorrect, I think this can go in with the same patch but I don't mind splitting that up.

borisson_’s picture

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

Setting to NR - so we can see if the bot agrees with what should be a green patch.

Nick_vh’s picture

Nick_vh
he/him
Location Ghent
CreditAttribution: Nick_vh at Acquia commented
Status: Needs review » Needs work

Testing that the hooks are called is fairly easy, but for example, hook search_api_test_hooks_search_api_index_items_alter should be tested in a way that we actually verify that the alter took place in the indexing process :)

I suppose you know that, and setting it back to needs work

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented
Issue tags: -Needs tests
FileSize
interdiff.txt1.54 KB
add_tests_for_the-2625264-4.patch9.15 KB

Added a test for hook_search_api_field_type_mapping_alter. All hooks in the .api file have tests now.

As mentioned in #4, all this does is testing that the hook is actually being called. We need to make the tests more specific to each hook.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented
Status: Needs work » Needs review
FileSize
interdiff.txt1.42 KB
add_tests_for_the-2625264-6.patch9.32 KB

Implemented a more specific test for search_api_test_hooks_search_api_index_items_alter.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey as a volunteer commented
FileSize
2625264-7--hook_tests.patch8.95 KB
2625264-7--hook_tests--interdiff.txt3.18 KB

Great work, thanks a lot!
Just a few minor issues with your patch:

  1. +++ b/src/Tests/HooksTest.php
@@ -0,0 +1,177 @@
+    $server_add_form = $this->urlGenerator->generateFromRoute('entity.search_api_server.add_form', array(), array('absolute' => TRUE));

    Why not just pass a path to drupalGet()?
    Anyways, generateFromRoute() is marked as internal, so we shouldn't use it. 

  2. +++ b/tests/search_api_test_hooks/search_api_test_hooks.info.yml
@@ -0,0 +1,8 @@
+#hidden: true

    Why is that commented out?

Please see/test/review my attached patch, then I can commit.

borisson_’s picture

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

#7.1 Sure, yeah.
#7.2 That was for testing. Shouldn't be commented.

Your patch looks great; commit away.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey as a volunteer commented
Status: Needs review » Fixed

OK, commited.
Thanks again!

  • drunken monkey committed d8eed28 on 8.x-1.x 
    Revert "Issue #2625264 by borisson_, drunken monkey: Added tests for the...
drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey as a volunteer commented
Title: Add tests for the query/result alter hooks » Add tests for the module's alter hooks
Status: Fixed » Needs review
FileSize
2625264-11--hook_tests--interdiff.txt2.17 KB
2625264-11--hook_tests.patch8.62 KB

Reverted, tests failed suddenly after committing this.
Re-posting my patch (with paths instead of Url objects, though).

borisson_’s picture

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

I don't understand how this patch can break any test not in HooksTest. It doesn't change any code that's being executed. The only non-test change is the in .api.php and that doesn't get executed.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey as a volunteer commented
Status: Needs review » Needs work

I don't understand how this patch can break any test not in HooksTest. It doesn't change any code that's being executed. The only non-test change is the in .api.php and that doesn't get executed.

As explained elsewhere, the fails in SQLite are unrelated. And the new fails are caused by the removal of hook_search_api_views_query_alter() in the meantime (in #2044393: Changes in search query functionality).

Thus, I also have no idea how the patch in #12 can still be passing tests. o_O Weird, weird stuff happening with the test bot in the last few days …

Anyways, we'll also need to add hooks for the new tag-based query/result alter hooks introduced in #2044393: Changes in search query functionality, so this needs work anyways. And I guess I'll just also test locally, to make sure the test bot is not just guessing at the results.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented
Assigned: Unassigned » borisson_

I think the patch in #12 is passing because of #2634114: Patches not applied correctly for contrib modules, PHP >= 5.4?.

I'll add the extra tests.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented
Assigned: borisson_ » Unassigned
Status: Needs work » Needs review
FileSize
interdiff.txt6.24 KB
add_tests_for_the-2625264-16.patch7.99 KB

I couldn't help myself and I've made some unrelated changes that make the test somewhat lighter and easier. I added in the extra tests.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey for Acquia commented
Status: Needs review » Fixed

Thanks a lot, looks good!
Committed.
(Also tested manually locally before.)

Status: Fixed » Closed (fixed)

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