The \Drupal\facetapi\Tests\IntegrationTest is still very tightly coupled to search api and this will break tests on drupal.org because search api is not a hard dependency (see #2609970: Search_api is a needed dependency (for now?) for consulting the config screens).

We completed #2612084: Add a new facet source for drupal core, so this can now be started.

TODO

  1. Change the integration test so that it doesn't depend on search api anymore but only tests admin pages + keeps testUrlAlias, testRenameFacet, testOverviewPermissions. It can instead depend on core search.
  2. Add a new CoreSearchIntegrationTest that depends on core search as well but specifically tests the functionality of core search.
  3. Add a SearchApiIntegrationTest that tests search api specific integration (adding a views page adds a new facet source, test different types of fields, ...)

.2 and .3 can be done in followups because they look like a lot of work. We want to to get a the classes in here already though. This issue adds the needed classes / setup we need to add extra test coverage. I don't think we need extra coverage going out of this, but we can't lose any either.

Comments

borisson_ created an issue. See original summary.

borisson_’s picture

Version: » 8.x-1.x-dev
Issue summary: View changes
Status: Postponed » Active

Updated IS.

marthinal’s picture

Assigned: Unassigned » marthinal
marthinal’s picture

Finally I had time to work on it.

I've detected a couple of issues:

1- class FacetDisplayForm

    // @TODO Causes a warning when search_api module is not available
    // The following theme is missing from the file system: search_api
    $form['#attached']['library'][] = 'search_api/drupal.search_api.index-active-formatters';

2- Not sure why the id is built using "--2". And at this moment I have no time to take a look at this part.
class WidgetIntegrationTest

    $this->drupalPostForm(NULL, array('type[page]' => 'page'), $this->t('submit'));
    // @TODO Verify if the id is correct.
    $this->assertFieldChecked('edit-type-page--2');

This is a first step. If we need to add- modify tests for the core module or Search API then we should open new issues IMHO :) Otherwise this patch will be too big

Status: Needs review » Needs work

The last submitted patch, 4: 2613186--4.patch, failed testing.

The last submitted patch, 4: 2613186--4.patch, failed testing.

marthinal’s picture

2- Fixed.

Let's take a look at the tests results.

borisson_’s picture

@marthinal, I took a look at the patch, it looks great but next time please use git diff -M, this reduces the patch size in half because git can actually say that files are moved instead of deleted/added.

Anyway, I took a good look at the patch and related code. I think we should fix the issues mentioned in #4 and leave the tests as-is.
The original thought of this issue was enabling testing on d.o, but because we already have that, the reasons for this aren't as strong.
We can add a tests that tests things like the integration test for core search without having to go trough such an extensive rewrite of all the tests.

I'm really sorry that you made all this effort before I thought of this.

borisson_’s picture

Added a related issue.

Status: Needs review » Needs work

The last submitted patch, 7: 2613186-7.patch, failed testing.

The last submitted patch, 7: 2613186-7.patch, failed testing.

borisson_’s picture

To keep it a little bit easier, we should probably just add an extra integration test for core search, this is an initial patch for that.

Status: Needs review » Needs work

The last submitted patch, 12: untangle_the-2613186-11.patch, failed testing.

The last submitted patch, 12: untangle_the-2613186-11.patch, failed testing.

marthinal’s picture

As discussed with borisson_ we could use the integration test from my last patch.

1) We need to fix #2655382: Copy library from search api into facets..

2) Review that we are covering what we need.

3) I've found a new bug. We have a dependency with views module. We need a new issue to fix the problem.

The last submitted patch, 15: 2613186-15.patch, failed testing.

The last submitted patch, 15: 2613186-15.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 15: 2613186-15-without-views.patch, failed testing.

The last submitted patch, 15: 2613186-15-without-views.patch, failed testing.

borisson_’s picture

Overall this is looking great, I did find some smaller issues with the code though.

  1. +++ b/core_search_facets/src/Tests/IntegrationTest.php
    @@ -0,0 +1,407 @@
    +use Drupal\core_search_facets\Tests\WebTestBase as CoreSearchFacetsWebTestBase;
    

    Nice, this makes a good distinction between the base classes, marthinal++

  2. +++ b/core_search_facets/src/Tests/IntegrationTest.php
    @@ -0,0 +1,407 @@
    +/**
    + * Tests the overall functionality of the Facets admin UI.
    + *
    + * @group core_search_facets
    + */
    

    I think this comment can be updated. Tests the admin UI with the core search facet source.

  3. +++ b/core_search_facets/src/Tests/IntegrationTest.php
    @@ -0,0 +1,407 @@
    +   * @param $facet_name
    +   *   The name of the facet.
    

    This should be @param string $facet_name for coding standards.

  4. +++ b/core_search_facets/src/Tests/IntegrationTest.php
    @@ -0,0 +1,407 @@
    +  public function setShowAmountOfResults($facet_name, $show = TRUE) {
    

    This method can be protected. We don't need it to be public.

  5. +++ b/core_search_facets/src/Tests/IntegrationTest.php
    @@ -0,0 +1,407 @@
    +  /**
    +   * Helper function: asserts that a facet block does not appear.
    +   */
    ...
    +  /**
    +   * Helper function: asserts that a facet block appears.
    +   */
    

    We can probably remove the "Helper function:" here, other methods don't have that indication.

  6. +++ b/core_search_facets/src/Tests/IntegrationTest.php
    @@ -0,0 +1,407 @@
    +
    

    unneeded blank line.

  7. +++ b/core_search_facets/src/Tests/IntegrationTest.php
    @@ -0,0 +1,407 @@
    +      //'facet_source_id' => 'core_node_search:node_search',
    +      'facet_source_configs[core_node_search:node_search][field_identifier]' => 'type',
    

    Why is this in comment?

  8. +++ b/core_search_facets/src/Tests/IntegrationTest.php
    @@ -0,0 +1,407 @@
    +    /*$this->assertRaw(t('Facet %name has been created.', ['%name' => $facet_name]));
    +    $this->assertUrl('admin/config/search/facets/' . $facet_id . '/display');
    +
    +    $this->drupalGet('admin/config/search/facets');*/
    

    Does this not work?

  9. +++ b/core_search_facets/src/Tests/IntegrationTest.php
    @@ -0,0 +1,407 @@
    +
    

    Unneeded blank line

  10. +++ b/core_search_facets/src/Tests/IntegrationTest.php
    @@ -0,0 +1,407 @@
    +    // @TODO Missing this text on local test. Not sure why.
    +    // $this->assertText($this->t('Are you sure you want to delete the facet'));
    +    // Actually submit the confirmation form.
    

    Can we figure out why this is, are the dependencies not calculated as expected?

  11. +++ b/core_search_facets/src/Tests/WebTestBase.php
    @@ -32,11 +32,15 @@ abstract class WebTestBase extends SimpletestWebTestBase {
    +    'facets_query_processor',
    

    We don't need this module, I think.

  12. +++ b/core_search_facets/src/Tests/WebTestBase.php
    @@ -61,18 +65,33 @@ abstract class WebTestBase extends SimpletestWebTestBase {
    +    // Adding 5 pages.
    

    Either change the comment or the number here, because it's not correct.

  13. +++ b/core_search_facets/src/Tests/WebTestBase.php
    @@ -61,18 +65,33 @@ abstract class WebTestBase extends SimpletestWebTestBase {
    +    for($i = 0;$i < 10 ; $i++) {
    

    Coding standards, should be for ($i =0; $i < 10; $i++) {

  14. +++ b/core_search_facets/src/Tests/WebTestBase.php
    @@ -61,18 +65,33 @@ abstract class WebTestBase extends SimpletestWebTestBase {
    +        'title' => 'foo bar',
    +        'body' => 'test page',
    

    Can we add the iterator's value to the title / body?
    'title' => 'foo bar: ' . $i

  15. +++ b/core_search_facets/src/Tests/WebTestBase.php
    @@ -61,18 +65,33 @@ abstract class WebTestBase extends SimpletestWebTestBase {
    +    // Adding 5 articles.
    +    for($i = 0;$i < 10; $i++) {
    +      $this->drupalCreateNode(array(
    +        'title' => 'foo baz',
    +        'body' => 'test article',
    +        'type' => 'article',
    +      ));
    +    }
    

    see above.

borisson_’s picture

Resolved my own issues, if this is green we can commit this and do the followup in #2655414: Avoid dependency on VIEWS module.

  • borisson_ committed 4a6552f on 8.x-1.x authored by marthinal
    Issue #2613186 by marthinal, borisson_: Untangle the integration test...
borisson_’s picture

Status: Needs review » Fixed

Thanks again for your contribution, committed. See you in the related issues for follow-ups.

Status: Fixed » Closed (fixed)

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