Discussed in #2867154: Form: Convert system functional tests to phpunit.

\Drupal\Tests\BrowserTestBase::drupalPostForm doesn't return the HTML of the rendered page.

From a BC perspective and for ease of conversion this method should probably have the same return value as \Drupal\simpletest\WebTestBase::drupalPostForm

CommentFileSizeAuthor
#20 interdiff-17-20.txt881 bytesAnonymous (not verified)
#20 2900292-20.patch2.35 KBAnonymous (not verified)
#17 interdiff-15-17.txt1.07 KBAnonymous (not verified)
#17 2900292-17.patch2.26 KBAnonymous (not verified)
#15 interdiff-12-15.txt1.93 KBAnonymous (not verified)
#15 2900292-15.patch2.36 KBAnonymous (not verified)
#12 2900292-12.patch1.55 KBshashikant_chauhan
#8 interdiff-6-8.txt726 bytesAnonymous (not verified)
#8 2900292-8-test-only.patch748 bytesAnonymous (not verified)
#6 2900292-6-test-only.patch720 bytesAnonymous (not verified)
#4 htmlrenderdpge_2900292_4.patch744 bytesMeenakshiG
#2 2900292-2.patch832 bytesManuel Garcia
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Lendude created an issue. See original summary.

Manuel Garcia’s picture

Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commented
Status: Active » Needs review
FileSize
2900292-2.patch832 bytes

So... something like this?

Lendude’s picture

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

@Manuel Garcia yeah that looks good, one nit:

+++ b/core/tests/Drupal/Tests/BrowserTestBase.php
@@ -920,6 +920,8 @@ protected function submitForm(array $edit, $submit, $form_html_id = NULL) {
+   * @return string The HTML of the rendered page.

'The HTML of the rendered page.' needs to be on the next line.

MeenakshiG’s picture

MeenakshiG CreditAttribution: MeenakshiG at OpenSense Labs commented
FileSize
htmlrenderdpge_2900292_4.patch744 bytes

@Lendude this patch contains 'The HTML of the rendered page.' on the next line . is it correct ?

MeenakshiG’s picture

MeenakshiG CreditAttribution: MeenakshiG at OpenSense Labs commented
Status: Needs work » Needs review
Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Needs review » Needs work
FileSize
2900292-6-test-only.patch720 bytes

#5: Almost, but seems, that #3 should be interpreted literally. Few examples from other places:

   * @return bool
   *   TRUE if the assertion passed; FALSE otherwise.
...
   * @return string|\Drupal\Component\Render\MarkupInterface
   *   A safe string filtered with the allowed tag list and normalized.
...
   * @return string
   *   The data type.

It may also be useful to formally fix this change via test.

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude as a volunteer and at Dx Experts commented

#6++
Yes, added test coverage is great.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Needs work » Needs review
Issue tags: +Novice
FileSize
2900292-8-test-only.patch748 bytes
interdiff-6-8.txt726 bytes

@Lendude, thank you!

We can also slightly improve the test, if check which line is returned.

Now we need to unite #2, #3 and current test (if @Lendude agrees that this variant is preferable).

@Meenakshi Gupta, do you still have the desire to help?

Status: Needs review » Needs work

The last submitted patch, 8: 2900292-8-test-only.patch, failed testing. View results

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude as a volunteer and at Dx Experts commented

@vaplas Yeah #8 looks good as a test, it's what we expect right? Not just any string but that specific string.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

Yes. You read my mind like an open book.

shashikant_chauhan’s picture

shashikant_chauhan CreditAttribution: shashikant_chauhan as a volunteer and at Iksula commented
FileSize
2900292-12.patch1.55 KB

Combined patch from #2, #3, #8.

shashikant_chauhan’s picture

shashikant_chauhan CreditAttribution: shashikant_chauhan as a volunteer and at Iksula commented
Status: Needs work » Needs review

updating status.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented 
+++ b/core/tests/Drupal/Tests/BrowserTestBase.php
@@ -920,6 +920,9 @@ protected function submitForm(array $edit, $submit, $form_html_id = NULL) {
+   *
+   * @return string
+   *   The HTML of the rendered page.

@@ -938,6 +941,8 @@ protected function drupalPostForm($path, $edit, $submit, array $options = []) {
+
+    return $this->getSession()->getPage()->getContent();
   }

Should we document here that the recommended way is to use $this->getSession()->getPage()->getContent() or actually the assertions we provide via the webAssert?

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Issue tags: -Novice
FileSize
2900292-15.patch2.36 KB
interdiff-12-15.txt1.93 KB

#12: thank you, @shashikant_chauhan! All novice tasks are addressed, so remove the Novice tag.

#14: if I correctly understood the question, then the main reason is not in the recommendation to use the return value, but only in compatibility (especially for outside test, because to change the core tests - not a big deel). I tried to add a comment about it.

Also I adjusted the description a little, so that there would be no association with the html-format of the pages + tests:

-   *   The HTML of the rendered page.
+   *   The response content after submit form.

...

+    // Test drupalPostForm() with no-html response.
+    $values = Json::decode($this->drupalPostForm('form_test/form-state-values-clean', [], t('Submit')));
+    $this->assertTrue(1000, $values['beer']);
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented 
  1. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
@@ -938,6 +941,11 @@ protected function drupalPostForm($path, $edit, $submit, array $options = []) {
+    // Returns response content for BC purposes. Feel free to use the webAssert
+    // object for your assertions.

    What do you think about moving the hint to webAssert into the function documentation itself? This would help a bit in terms of discovery, as people will most likely read the function documentation instead of the actual code of the function. 

  2. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
@@ -938,6 +941,11 @@ protected function drupalPostForm($path, $edit, $submit, array $options = []) {
+    // @see \Drupal\simpletest\WebTestBase::drupalPostForm()

    This @see feels a little bit weird ... I would have probably skipped that :)

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
FileSize
2900292-17.patch2.26 KB
interdiff-15-17.txt1.07 KB

#16 I thought about this while I waited for the review. And I am absolutely glad about these proposals!

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented
Status: Needs review » Reviewed & tested by the community

Thank you!

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Needs review

Are we going to want to deprecate the return for 9.0.x or just leave it like this?

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
FileSize
2900292-20.patch2.35 KB
interdiff-17-20.txt881 bytes

The '(deprecated)' mark definitely should help the wavering user, and will allow to eliminate the confusing variety in the future.

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude as a volunteer and at Dx Experts commented
Status: Needs review » Needs work
Issue tags: +Needs change record

Hmm interesting deprecation case. Hadn't seen the return type deprecation before.

Per deprecation policy for this, we need a CR and a link to that CR for this.

Manuel Garcia’s picture

Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commented
Status: Needs work » Needs review

Thanks all for the work on this!

I've added a draft CR, please review =) https://www.drupal.org/node/2907815

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs change record

Thank you @Manuel Garcia to create the change record. I fixed a tiny bit of it, but it looks good for me.

  • catch committed 932db61 on 8.5.x 
    Issue #2900292 by vaplas, Manuel Garcia, Meenakshi Gupta,...

  • catch committed 5961bb6 on 8.4.x 
    Issue #2900292 by vaplas, Manuel Garcia, Meenakshi Gupta,...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Yes return deprecation is hard.

Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!

Status: Fixed » Closed (fixed)

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

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

Shouldn't t('Submit') replaced with $this->t('Submit'); ?? 

    $values = Json::decode($this->drupalPostForm('form_test/form-state-values-clean', [], t('Submit')));
Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude as a volunteer and at Dx Experts commented

@JohnOku that's not needed inside a test.

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented

publish the change record