Support from Acquia helps fund testing for Drupal Acquia logo

Comments

drunken monkey created an issue. See original summary.

idebr’s picture

The biggest change when switching to WebDriverTestBase is you can no longer inspect response status codes, see [#2857562]

The suggestion in the change record is check for the existence of elements in the UI instead. This is already the case in most of \Drupal\Tests\search_api_autocomplete\FunctionalJavascript\IntegrationTest, so many of these can be removed without losing too much data.

However, a test like \Drupal\Tests\search_api_autocomplete\FunctionalJavascript\IntegrationTest::checkAdminAccess probably better off in a Functional test:


  /**
   * Verifies that admin pages are properly protected.
   */
  protected function checkAdminAccess() {
    // Make sure anonymous and non-admin users cannot access admin pages.
    $users = [
      'non-admin' => $this->normalUser,
      'anonymous' => NULL,
    ];
    $paths = [
      'index overview' => $this->getAdminPath(),
      'search edit form' => $this->getAdminPath('edit'),
      'search delete form' => $this->getAdminPath('delete'),
    ];
    foreach ($users as $user_type => $account) {
      $this->drupalLogout();
      if ($account) {
        $this->drupalLogin($account);
      }
      foreach ($paths as $label => $path) {
        $this->drupalGet($path);
        $status_code = $this->getSession()->getStatusCode();
        $this->assertEquals(403, $status_code, "The $label is accessible for $user_type users.");
      }
    }
    $this->drupalLogin($this->adminUser);
  }
drunken monkey’s picture

Issue tags: +Drupal 8.6 dependency
drunken monkey’s picture

Maybe like this?
(Didn’t manage to install Chromedriver, so still can’t run JS tests locally – has been the same for years, though, as PhantomJS hasn’t been working for quite some time, either.)

Status: Needs review » Needs work

The last submitted patch, 4: 2986223-4--switch_javascript_test_base.patch, failed testing. View results

drunken monkey’s picture

Shoot.
Did anyone manage to get this running and would be able to help here? Would be really appreciated.

idebr’s picture

Status: Needs work » Needs review
FileSize
7.53 KB
16.25 KB

Attached patch completes the conversion to WebDriverTestBase

Note that integration with Search API Pages requires the patch from #3063684: Search block form should define a base form ID to be committed.

drunken monkey’s picture

Excellent, thanks a lot!
Just seems like something went wrong in patch creation? The attached should work instead.

Status: Needs review » Needs work

The last submitted patch, 8: 2986223-8--switch_javascript_test_base.patch, failed testing. View results

drunken monkey’s picture

Status: Needs work » Needs review

Should hopefully work now that #3063684: Search block form should define a base form ID has been committed …

Status: Needs review » Needs work

The last submitted patch, 8: 2986223-8--switch_javascript_test_base.patch, failed testing. View results

idebr’s picture

Status: Needs work » Needs review
FileSize
276 bytes
16.91 KB

The Drupal test infrastructure is still downloading the latest tagged release for drupal/search_api_page:

08:08:58 sudo -u www-data /usr/local/bin/composer require --no-interaction 'drupal/search_api_page:@dev' --prefer-stable --no-progress --no-suggest --working-dir /var/www/html
08:09:05 ./composer.json has been updated
08:09:05 > Drupal\Core\Composer\Composer::ensureComposerVersion
08:09:05 Loading composer repositories with package information
08:09:06 Updating dependencies (including require-dev)
08:09:15 Package operations: 1 install, 0 updates, 0 removals
08:09:15 - Installing drupal/search_api_page (1.0.0-beta2): Downloading (100%)

Attached patch updates the package requirement, so it downloads the dev-version instead.

idebr’s picture

FileSize
16.44 KB

I was using an outdated remote branch for 8.x-1.x. This patch should apply properly.

drunken monkey’s picture

Thanks for fixing this problem!

+++ b/search_api_autocomplete.module
@@ -162,12 +162,8 @@ function search_api_autocomplete_form_views_exposed_form_alter(array &$form, For
+  $page_id = ltrim($form_id, 'search_api_page_block_form_');

Phew, glad I still spotted this by sheer luck! (Not sure how I missed this – I guess I just trusted you and the test bot, since I couldn’t test anyways.)
This is not how ltrim() works! Let’s use substr() instead …

Looked over the rest of the code again, but that seems fine. Please test/review the attached revision and we can finally commit.

idebr’s picture

Status: Needs review » Reviewed & tested by the community

Looks good!

  • drunken monkey committed b3c83b5 on 8.x-1.x
    Issue #2986223 by drunken monkey, idebr: Switched JS tests to...
drunken monkey’s picture

Status: Reviewed & tested by the community » Fixed

Thanks for the feedback!
Committed.
Thanks a lot again for all your help here!

Status: Fixed » Closed (fixed)

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