Support from Acquia helps fund testing for Drupal Acquia logo

Comments

beltofte created an issue. See original summary.

beltofte’s picture

Status: Active » Closed (won't fix)
juhog’s picture

Status: Closed (won't fix) » Needs review
FileSize
9.85 KB

I wrote some unit tests for the module. I hope this is the correct place to post the patch, eventhough the issue is closed. Apologies if this isn't the correct place.

Notes about the patch:
- I used Search API module's unit tests as examples and tried to apply the same ideas
- The tests cover only one method: alterIndexedItems()
- EntityManager removed, replaced by EntityFieldManager
- EntityFieldManager dependency is injected using a setter method
- Minor fix to bundleHasField() docblock (the return value type)

juhog’s picture

The test file had some coding standard issues. This patch fixes those.

borisson_’s picture

Status: Needs review » Needs work

I only have one meaningful thing to say about this patch. That is the last point in this review. The rest already looks super good. Great work!

  1. +++ b/src/Plugin/search_api/processor/SearchApiExcludeEntityProcessor.php
    @@ -132,14 +163,14 @@ class SearchApiExcludeEntityProcessor extends ProcessorPluginBase implements Plu
    -   * @return array
    -   *   Options array with bundles.
    +   * @return bool
    +   *   TRUE if the entity bundle has the field, FALSE otherwise.
    

    Oh, it is good that you also fixed this in the meanwhile, it was wrong.

  2. +++ b/tests/src/Unit/Processor/SearchApiExcludeEntityProcessorTest.php
    @@ -0,0 +1,200 @@
    + * @coversDefaultClass \Drupal\search_api_exclude_entity\Plugin\search_api\processor\SearchApiExcludeEntityProcessor
    ...
    + * @see \Drupal\search_api_exclude_entity\Plugin\search_api\processor\SearchApiExcludeEntityProcessor
    

    It's not strictly needed to have both the @see and the @coversDefaultClass point at the same processor, on the other hand, it doesn't hurt either.

  3. +++ b/tests/src/Unit/Processor/SearchApiExcludeEntityProcessorTest.php
    @@ -0,0 +1,200 @@
    +    $this->assertTrue(!empty($items[$item1->getId()]), "Item without any exclude fields wasn't removed.");
    +    $this->assertTrue(!empty($items[$item2->getId()]), "Item with 0/2 of exclude fields enabled wasn't removed.");
    +    $this->assertTrue(empty($items[$item3->getId()]), "Item with 1/2 of exclude fields enabled was removed.");
    +    $this->assertTrue(empty($items[$item4->getId()]), "Item with 2/2 of exclude fields enabled was removed.");
    

    We can change this with assertEmpty/assertNotEmpty to be clearer.

juhog’s picture

Thanks for the comments!

Here's a new patch with the following changes:

  • I removed the @see line. It's probably better that way.
  • I changed the asserts to assertArrayHasKey() and assertArrayNotHasKey(). I tried those other ones first, but assertNotEmpty() produced an "Undefined index" php notice and the test didn't finish.
borisson_’s picture

Status: Needs work » Reviewed & tested by the community

Yeah, that also works! Thank @juhog!

  • beltofte committed f7d6941 on 8.x-1.x authored by juhog
    Issue #2771087 by juhog, borisson_: Unit tests
    
beltofte’s picture

Status: Reviewed & tested by the community » Fixed

Committed!

Thanks @juhog and @borisson_!

Status: Fixed » Closed (fixed)

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