Convert web tests to browser tests for search module

Does someone mind to create a followup issue so we don't forget to the convert the remaining bits?

+++ b/core/modules/search/tests/src/Functional/SearchAdvancedSearchFormTest.php
@@ -53,14 +53,14 @@ public function testNodeType() {
-    $this->drupalPostForm('search/node', $edit, t('Advanced search'));
+    $this->drupalPostForm('search/node', $edit, 'edit-submit--2');
...
-    $this->drupalPostForm('search/node', array_merge($edit, ['type[page]' => 'page']), t('Advanced search'));
+    $this->drupalPostForm('search/node', array_merge($edit, ['type[page]' => 'page']), 'edit-submit--2');
...
-    $this->drupalPostForm('search/node', array_merge($edit, ['type[article]' => 'article']), t('Advanced search'));
+    $this->drupalPostForm('search/node', array_merge($edit, ['type[article]' => 'article']), 'edit-submit--2');

I'm wondering is this change really needed? Reading the function documentation explains that you should be able to provide a label.

Follow-ups created and the feedback from @dawehner was explained. I think this is good to got!

Thanks @GoZ!

Unrelated fail

+1 for the RTBC

Re #15 I think this makes the tests way less readable. However reading the docs for \Drupal\Tests\BrowserTestBase::submitForm() in BrowserTestBase I think we should just use that method instead and supply the $form_html_id param. Or maybe we should open an issue to add a param to drupalPostForm(). I think I prefer converting these calls to use \Drupal\Tests\BrowserTestBase::submitForm()

Created a follow-up for #21 #2887411: Add $form_html_id to drupalPostForm in BrowserTestBase and convert SearchTestBase::submitGetForm() to BrowserTestBase::drupalPostForm()

Shall we convert this particular test in follow-up and move with the tests which have minimal disruptions that way we'll have very fewer tests which are pending on postponed issues

//Naveen

We can use ::findButton and then ::press instead of ::submitForm if there are conflicts

$page = $this->getSession()->getPage();
$button = $page->findButton('edit-submit-2');
$page->fillField('some field', 'some value');
$button->press();

Unpostponed everything and converted everything.

We will not need parse() because we can do much better with the steps provided by @larowlan in #24. The existing \Drupal\Tests\search\Functional\SearchTestBase::submitGetForm method in HEAD is broken and will fatal when used. This means it is not used anywhere right now so I have little problem rewriting it to suit our needs, so I've done so.

Also the follow up in #2887411: Add $form_html_id to drupalPostForm in BrowserTestBase and convert SearchTestBase::submitGetForm() to BrowserTestBase::drupalPostForm() won't help here to address #15 because the other label is also inside that form, so the collision will still occur even when restricting to the form. So using the #id makes sense even if it reduces readability (and having two buttons with the same label might be an accessibility issue and might be better solved by making sure we have two buttons that can be distinguished between? So might be something that should be fixed outside this test.)

fixed deprecation notices

+++ b/core/modules/search/tests/src/Functional/SearchCommentTest.php
@@ -299,14 +301,12 @@ public function assertCommentAccess($assume_access, $message) {
-      $expected_node_result = $this->assertText(t('Your search yielded no results.'));
-      $expected_comment_result = $this->assertText(t('Your search yielded no results.'));
+      $this->assertSession()->pageTextContains(t('Your search yielded no results.'));
     }
-    $this->assertTrue($expected_node_result && $expected_comment_result, $message);

Looks like we're losing some coverage here.

I'm not 100% sure though, but I think that we should fix that. That's the only thing I could find though. Also unassigned this issue.

Awesome, looks great!

1. It's a shame about the button thing. I've dug into the code and this happens because we have a summary element with the role of button and the same text. For me there's a question about whether or not our submit form should find such buttons. Anyhow the fix here works.
2. \Drupal\search\Tests\SearchTestBase is already deprecated but we could do with a followup to trigger a silenced deprecation notice.

3. diff --git a/core/modules/search/tests/src/Functional/SearchCommentTest.php b/core/modules/search/tests/src/Functional/SearchCommentTest.php
   index 714366185d..c964f88f2f 100644
   --- a/core/modules/search/tests/src/Functional/SearchCommentTest.php
   +++ b/core/modules/search/tests/src/Functional/SearchCommentTest.php
   @@ -305,7 +305,7 @@ public function assertCommentAccess($assume_access, $message) {
          $this->assertSession()->pageTextContains($this->commentSubject);
        }
        else {
   -      // Not expected node or comment results.
          $this->assertSession()->pageTextContains(t('Your search yielded no results.'));
        }
      }
   

   This comment with stating with the "Not" doesn't read correctly and it looks superfluous because the test searched for says enough. let's remove it.

+++ b/core/modules/search/tests/src/Functional/SearchCommentTest.php
@@ -299,14 +301,13 @@ public function assertCommentAccess($assume_access, $message) {
-    $this->assertTrue($expected_node_result && $expected_comment_result, $message);

This means the $message is not longer used. Which is problematic because it adds context for the test.

#34.2 should be covered by #2886547: Add trigger_error to deprecated TestBase base classes/traits that have none, reference CR
#34.3 fixed
#34.4 true, how about using message as a fail text for the assertion.

Thanks @Lendude - looks great.

Committed ad90164 and pushed to 8.6.x. Thanks!