Follow-up to #2863842: Convert web tests to browser tests for search module

CommentFileSizeAuthor
#12 interdiff-11-12.txt731 bytesAnonymous (not verified)
#12 2887411-12.patch9.47 KBAnonymous (not verified)
#12 2887411-12-test-only.patch1.39 KBAnonymous (not verified)
#11 interdiff-9-11.txt3.38 KBAnonymous (not verified)
#11 2887411-11.patch9.45 KBAnonymous (not verified)
#11 2887411-11-test-only.patch1.37 KBAnonymous (not verified)
#9 interdiff-2-9.txt7.14 KBAnonymous (not verified)
#9 2887411-9.patch7.89 KBAnonymous (not verified)
#2 2887411-2.patch988 bytesnaveenvalecha
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

naveenvalecha created an issue. See original summary.

naveenvalecha’s picture

naveenvalecha
He/Him
Primary language Hindi
CreditAttribution: naveenvalecha as a volunteer commented
Component: search.module » phpunit
Issue tags: -DevDaysSeville +phpunit initiative
Parent issue: #2863842: Convert web tests to browser tests for search module »
Related issues: +#2863842: Convert web tests to browser tests for search module
FileSize
2887411-2.patch988 bytes
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Needs review » Needs work

Can we add a usage please so we have implicit coverage.

Thanks

naveenvalecha’s picture

naveenvalecha
He/Him
Primary language Hindi
CreditAttribution: naveenvalecha as a volunteer commented
Title: Add $form_html_id to drupalPostForm in BrowserTestBase » Add $form_html_id to drupalPostForm in BrowserTestBase and convert SearchAdvancedSearchFormTest from WTB to BTB
Parent issue: » #2863842: Convert web tests to browser tests for search module
Related issues: -#2863842: Convert web tests to browser tests for search module +#2870595: Convert postponed WTB to BTB for search module SearchBlockTest and SearchConfigSettingsFormTest

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented 
--- a/core/tests/Drupal/Tests/BrowserTestBase.php
+++ b/core/tests/Drupal/Tests/BrowserTestBase.php

+++ b/core/tests/Drupal/Tests/BrowserTestBase.php
@@ -921,7 +921,7 @@ protected function submitForm(array $edit, $submit, $form_html_id = NULL) {

@@ -921,7 +921,7 @@ protected function submitForm(array $edit, $submit, $form_html_id = NULL) {
    * @param array $options
    *   Options to be forwarded to the url generator.
    */
-  protected function drupalPostForm($path, $edit, $submit, array $options = []) {
+  protected function drupalPostForm($path, $edit, $submit, array $options = [], $form_html_id = NULL) {
     if (is_object($submit)) {

Let's also expand the documentation: We could copy it from \Drupal\simpletest\WebTestBase::drupalPostForm

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

After #2863842: Convert web tests to browser tests for search module we can replace all SearchTestBase::submitGetForm() to BTB::drupalPostForm(). This will give an explicit usage cases.

Also BTB:drupalPostForm() sends requests in the way that is specified in the action attribute of the form (proof from BTB::submitForm). We could also reflect this in the documentation. "Drupal Post From" like verb "send", not like type "POST".

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Needs work » Needs review
FileSize
2887411-9.patch7.89 KB
interdiff-2-9.txt7.14 KB

#7: Done.
#8: Done.

Case with $form_html_id:

+++ b/core/modules/search/tests/src/Functional/SearchBlockTest.php
@@ -92,16 +92,16 @@ public function testSearchFormBlock() {
-    $this->submitGetForm(NULL, ['keys' => $this->randomMachineName()], t('Search'), 'search-block-form');
+    $this->drupalPostForm(NULL, ['keys' => $this->randomMachineName()], t('Search'), [], 'search-block-form');

also thanks to the form_html_id detected nit defect during conversion WTB/BTB:

+++ b/core/modules/search/tests/src/Functional/SearchBlockTest.php
@@ -92,16 +92,16 @@ public function testSearchFormBlock() {
-    $this->drupalPostForm(NULL, ['keys' => $this->randomMachineName()], t('Search'), [], [], 'search-form');
+    $this->drupalPostForm(NULL, ['keys' => $this->randomMachineName()], t('Search'), [], 'search-form');
Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude at Dx Experts commented
Status: Needs review » Needs work

@vaplas nice!

couple of things:

+++ b/core/modules/search/tests/src/Functional/SearchTestBase.php
@@ -40,27 +40,10 @@ protected function setUp() {
   protected function submitGetForm($path, $edit, $submit, $form_html_id = NULL) {
...
+    @trigger_error(__CLASS__ . '::' . __FUNCTION__ . '() is deprecated in Drupal 8.6.x, for removal before the Drupal 9.0.0 release. Use \Drupal\Tests\BrowserTestBase::drupalPostForm instead.', E_USER_DEPRECATED);

I think we need a @deprecated here and a CR to @see to, right?

Its nice to have some use in the wild, but I think we should still add some dedicated coverage to \Drupal\FunctionalTests\BrowserTestBaseTest::testForm for this.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Needs work » Needs review
FileSize
2887411-11-test-only.patch1.37 KB
2887411-11.patch9.45 KB
interdiff-9-11.txt3.38 KB

@Lendude, thanks!

I did not do СR before, deciding that it's not worth it. But if you offer, there are no problems.

Added dedicated coverage by your hint. At first glance it may seem a bit complicated, but it's much better than my first version of this coverage, which I will not even tell :)

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
FileSize
2887411-12-test-only.patch1.39 KB
2887411-12.patch9.47 KB
interdiff-11-12.txt731 bytes 
+++ b/core/tests/Drupal/FunctionalTests/BrowserTestBaseTest.php
@@ -111,6 +111,15 @@ public function testForm() {
+    $this->assertTextHelper('Page form2 has been created.');

Who added assertTextHelper to my patch?)

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented
Status: Needs review » Reviewed & tested by the community

This looks very solid, I can't find anything to nitpick or change about this patch. Great work.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott as a volunteer and at Acro Commerce, Thunder commented
Status: Reviewed & tested by the community » Fixed

Committed 396936c and pushed to 8.6.x. Thanks!

Can we get a followup to remove \Drupal\Tests\search\Functional\SearchTestBase - the abstraction to just enable search and create content types if not standard looks unnecessary. I think it could use the same CR.

  • alexpott committed 396936c on 8.6.x 
    Issue #2887411 by vaplas, naveenvalecha, larowlan, dawehner, Lendude,...
Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

@borisson_, @alexpott, thank you!

#14: Done #2980107: Replace Functional\SearchTestBase on BrowserTestBase?

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
CreditAttribution: kim.pepper at PreviousNext commented

The change record needs updating:

Method \Drupal\Tests\BrowserTestBase::drupalPostForm() is deprecated. Use \Drupal\Tests\BrowserTestBase::drupalPostForm() instead.

Should be:

Method \Drupal\Tests\search\Functional\SearchTestBase::submitGetForm() is deprecated. Use \Drupal\Tests\BrowserTestBase::drupalPostForm() instead.

Also the title:

have been deprecated

should be

has been deprecated

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Title: Add $form_html_id to drupalPostForm in BrowserTestBase and convert SearchAdvancedSearchFormTest from WTB to BTB » Add $form_html_id to drupalPostForm in BrowserTestBase and convert SearchTestBase::submitGetForm() to BrowserTestBase::drupalPostForm()

#17: thanks, fixed.

Also added info about deprecated SearchTestBase. But this is relevant after #2980107: Replace Functional\SearchTestBase on BrowserTestBase? only.

Status: Fixed » Closed (fixed)

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

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented

Publish change record