Comments

Lendude created an issue. See original summary.

Manuel Garcia’s picture

Status: Active » Needs review
FileSize
832 bytes

So... something like this?

Lendude’s picture

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.

Meenakshi Gupta’s picture

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

Meenakshi Gupta’s picture

Status: Needs work » Needs review
vaplas’s picture

Status: Needs review » Needs work
FileSize
720 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

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

vaplas’s picture

Status: Needs work » Needs review
Issue tags: +Novice
FileSize
748 bytes
726 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

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

vaplas’s picture

Yes. You read my mind like an open book.

shashikant_chauhan’s picture

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

shashikant_chauhan’s picture

Status: Needs work » Needs review

updating status.

dawehner’s picture

+++ 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?

vaplas’s picture

Issue tags: -Novice
FileSize
2.36 KB
1.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

  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 :)

vaplas’s picture

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

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you!

catch’s picture

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?

vaplas’s picture

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