While there are already some tests, coverage at the moment is minimal. Before creating a stable release, we'll definitely have to do much better there, with proper unit, kernel and functional tests. And probably even Javascript tests, I guess.

CommentFileSizeAuthor
#4 2889437-4--add_tests.patch58.83 KBdrunken monkey

Comments

drunken monkey created an issue. See original summary.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Issue tags: -Release blocker +beta blocker
drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented

Actually found a Core bug (I think) while working on this: #2895591: JSWebAssert::assertNotVisibleInViewport() does not actually check element visibility.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Active » Needs review
StatusFileSize
new 2889437-4--add_tests.patch58.83 KB

This should be plenty, for now. Can always add more later (esp. for new features or fixed bugs).

The patch depends on #2895142: Make our test classes more useful for other modules in the Search API module. I just made a new release for it – let's hope this is enough to be picked up by the test bot.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Reviewed & tested by the community 
  1. +++ b/tests/src/FunctionalJavascript/IntegrationTest.php
@@ -0,0 +1,435 @@
+  /**
+   * A normal (non-admin) user used for the tests.

    Can we say anonymous here? So: An anonymous user used for the tests. 

  2. +++ b/tests/src/FunctionalJavascript/IntegrationTest.php
@@ -0,0 +1,435 @@
+  /**
+   * Goes to the index's "Autocomplete" tab and creates/enables the test search.
+   */
+  protected function enableSearch() {

    We should only really use javascript tests when we need the javascript, and I don't think that's needed for this test-method, let's move that to a normal btb-test? 

  3. +++ b/tests/src/FunctionalJavascript/IntegrationTest.php
@@ -0,0 +1,435 @@
+    $assert_session->checkboxChecked("searches[{$this->searchId}]");
+
+    $this->click('[data-drupal-selector="edit-submit"]');
+    $this->logPageChange(NULL, 'POST');

    I think it'd make more sense to move this blank line down 1 line,

    ->checkboxChecked
->click

->logPageChange

  4. +++ b/tests/src/FunctionalJavascript/IntegrationTest.php
@@ -0,0 +1,435 @@
+  protected function configureSearch() {

    Doesn't need js either, I think.

Only nitpicks, nothing major. Don't mind being ignored on any of 'm so setting to rtbc.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Reviewed & tested by the community » Needs work

Actually, I think moving admin-tests to BrowserTestBase is a good thing to do. So I think we should do that and remove that from the javascript test. We should use the api to create an index/autocomplete in the js-tests so that they don't need to use the ui for all that creation.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented

Thanks a lot for reviewing!

Can we say anonymous here? So: An anonymous user used for the tests.

You mean "authenticated"? It's not an anonymous user.

Actually, I think moving admin-tests to BrowserTestBase is a good thing to do. So I think we should do that and remove that from the javascript test. We should use the api to create an index/autocomplete in the js-tests so that they don't need to use the ui for all that creation.

Why? If we can have a single integration test for the whole UI, why split it just because not all functionality is needed for all parts? Is there really a benefit in having a BTB test instead?

Also, part of the current test also tests JS functionality in the admin UI (showing/hiding suggester settings when they are enabled/disabled). Not critical, I guess, but we'd still actually lose some coverage when moving this to BTB.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs work » Reviewed & tested by the community

#7, If we lose coverage, we might as well keep this as js-tests.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Reviewed & tested by the community » Fixed

OK, great. Thanks again for your review and feedback!
Committed.

Status: Fixed » Closed (fixed)

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