Convert web tests to browser tests for help module

I took a quick look at this patch. I have a few questions about the changes in the patch:

1. +++ b/core/modules/help/tests/src/Functional/HelpTest.php
   @@ -122,7 +120,7 @@ protected function verifyHelp($response = 200) {
   -        $this->assertEqual($this->cssSelect('h1.page-title')[0], t($name), format_string('%module heading was displayed', array('%module' => $module)));
   +        $this->assertEquals($name, $this->cssSelect('h1.page-title')[0]->getText(), "$module heading was displayed");
   

   I noticed this line took out the t() call -- used to be t($name). Was that intentional? Have we lost something?

2. +++ b/core/modules/help/tests/src/Functional/NoHelpTest.php
   @@ -40,10 +40,14 @@ public function testMainPageNoHelp() {
        $this->assertFalse(\Drupal::moduleHandler()->implementsHook('menu_test', 'help'), 'The menu_test module does not implement hook_help');
   -    $this->assertNoText(\Drupal::moduleHandler()->getName('menu_test'), 'Making sure the test module menu_test does not display a help link on admin/help.');
   +    // Making sure the test module menu_test does not display a help link on
   +    // admin/help.
   +    $this->assertNoText(\Drupal::moduleHandler()->getName('menu_test'));
    
        $this->drupalGet('admin/help/menu_test');
   -    $this->assertResponse(404, 'Getting a module overview help page for a module that does not implement hook_help() results in a 404.');
   +    // Getting a module overview help page for a module that does not implement
   +    // hook_help() results in a 404.
   +    $this->assertResponse(404);
   

   I don't understand why you took some of the custom messages out of assertions here, and not others. Also why that would be part of converting this to BrowserTestBase. That seems like a separate issue?

3. I guess this also needs to wait until #2735045: Convert StandardTest to BrowserTestBase by introducing a AssertLegacyTrait and new assertions on the WebAssert is finished, so that this patch can be focused only on the Help module. A bit hard to review otherwise. But we can iterate here in the meantime.

1. +++ b/core/tests/Drupal/Tests/BrowserAssertTrait.php
   @@ -0,0 +1,205 @@
   +
   +  /**
   +   * Asserts that the element with the given CSS selector is present.
   +   *
   +   * @param string $css_selector
   +   *   The CSS selector identifying the element to check.
   +   * @param string $message
   +   *   Optional message to show alongside the assertion.
   +   */
   +  protected function assertElementPresent($css_selector, $message = '') {
   +    $this->assertNotEmpty($this->getSession()->getDriver()->find($this->cssSelectToXpath($css_selector)), $message);
   +  }
   

   IMHO we should instead of reinventing those assertions rather add a \Drupal\FunctionalTests\AssertLegacyTrait which wraps the new available assertions and is marked as deprecated. This one could be $this->assertSession()->elementExists()

   You know, we want to teach good practises rather than just continue with our drupalisms.

2. +++ b/core/tests/Drupal/Tests/BrowserAssertTrait.php
   @@ -0,0 +1,205 @@
   +  protected function assertElementNotPresent($css_selector, $message = '') {
   

   elementNotExists

3. +++ b/core/tests/Drupal/Tests/BrowserAssertTrait.php
   @@ -0,0 +1,205 @@
   +  protected function assertText($text) {
   

   pageTextContains

4. +++ b/core/tests/Drupal/Tests/BrowserAssertTrait.php
   @@ -0,0 +1,205 @@
   +  protected function assertNoText($text) {
   

   pageTextNotContains

5. +++ b/core/tests/Drupal/Tests/BrowserAssertTrait.php
   @@ -0,0 +1,205 @@
   +  protected function assertResponse($code) {
   

   statusCodeEquals

6. +++ b/core/tests/Drupal/Tests/BrowserAssertTrait.php
   @@ -0,0 +1,205 @@
   +  protected function assertTitle($expected_title) {
   

   For this case where there is clearly no existing assertion, IMHO we should move the stuff to \Drupal\Tests\WebAssert

7. +++ b/core/tests/Drupal/Tests/BrowserAssertTrait.php
   @@ -0,0 +1,205 @@
   +  protected function assertFieldByName($name, $value = NULL) {
   

   Yeah this example here is pure legacy. You ideally don't want to have one function that does two different things.

8. +++ b/core/tests/Drupal/Tests/BrowserAssertTrait.php
   @@ -0,0 +1,205 @@
   +  protected function assertEqual($first, $second, $message = '') {
   +    // Cast objects implementing MarkupInterface to string instead of
   +    // relying on PHP casting them to string depending on what they are being
   +    // comparing with.
   +    $first = $this->castSafeStrings($first);
   +    $second = $this->castSafeStrings($second);
   +    $this->assertEquals($second, $first, $message);
   +  }
   ...
   +  /**
   +   * Fire an assertion that is always positive.
   +   *
   +   * @param string $message
   +   *   (optional) A message to display with the assertion. Do not translate
   +   *   messages: use \Drupal\Component\Utility\SafeMarkup::format() to embed
   +   *   variables in the message text, not t(). If left blank, a default message
   +   *   will be displayed.
   +   */
   +  protected function pass($message = '') {
   +    return $this->assertTrue(TRUE, $message);
   +  }
   

   In this case we should just leverage \Drupal\KernelTests\AssertLegacyTrait which has some of that stuff already.

9. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
   @@ -894,6 +900,90 @@ protected function submitForm(array $edit, $submit, $form_html_id = NULL) {
   +   */
   +  protected function drupalPostForm($path, array $edit, $submit, array $options = array()) {
   +    if (is_object($submit)) {
   

   Can we move this also to a legacy function or would you personally keep that?

10. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
    @@ -1402,13 +1504,13 @@ protected function refreshVariables() {
        *
    -   * @param \Drupal\user\UserInterface $account
    +   * @param \Drupal\Core\Session\AccountInterface $account
        *   The user account object to check.
        *
        * @return bool
        *   Return TRUE if the user is logged in, FALSE otherwise.
        */
    -  protected function drupalUserIsLoggedIn(UserInterface $account) {
    +  protected function drupalUserIsLoggedIn(AccountInterface $account) {
         $logged_in = FALSE;
    

    Nice

For assertEqual ... we should move this to assertEquals itself, as you have the same problem in new tests as well.

+++ b/core/modules/help/tests/src/Functional/HelpTest.php
@@ -122,7 +120,7 @@ protected function verifyHelp($response = 200) {
-        $this->assertEqual($this->cssSelect('h1.page-title')[0], t($name), format_string('%module heading was displayed', array('%module' => $module)));
+        $this->assertEquals($name, $this->cssSelect('h1.page-title')[0]->getText(), "$module heading was displayed");

I agree, t($name) really doesn't help much unless you actually test locale functionality.

This looks OK to me. Most of the patch is from the other issue, of course... so I guess this needs to be postponed until that other one is done?

unpostponing this one. Also this needs a reroll after this #2735045: Convert StandardTest to BrowserTestBase by introducing a AssertLegacyTrait and new assertions on the WebAssert

Seems reasonable now.

 git ac https://www.drupal.org/files/issues/2735199-13.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  5156  100  5156    0     0   6435      0 --:--:-- --:--:-- --:--:-- 13935
error: patch failed: core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php:149
error: core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php: patch does not apply

Rerolled after #2736109: Convert web tests to browser tests for action module

Committed 3991692 and pushed to 8.2.x. Thanks!

I'll cherry-pick to 8.1.x if tests pass there too.

diff --git a/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
index 6f7f971..d63ada6 100644
--- a/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
+++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
@@ -243,4 +243,5 @@ protected function assertUrl($path) {
   public function assertNoEscaped($raw) {
     $this->assertSession()->assertNoEscaped($raw);
   }
+
 }

Fixed on commit.

Was this actually committed? The 3991692 object ID doesn't exist and I can't find anything in the git log.

The assertNoEscaped() method that is introduced here has the wrong visibility which causes a fatal error. I created a followup for this: #2752315: Fix visibility of AssertLegacyTrait::assertNoEscaped().

Does not apply to 8.1.x, I guess we don't care and don't want to spend time on rerolling it.

We'll have a hard time porting these conversions since our recent failure to sync changes in AssertLegacyTrait, WebAssert, BrowserTestBase. The conversions are not important for 8.1.x, but the failure to sync AssertLegacyTrait, WebAssert, BrowserTestBase might come to hunt us later when cherry-picking other, not conversion related issues. Therefor I submitted a reroll for 8.1.x in #2736109-23: Convert web tests to browser tests for action module. That one turned out to be a very easy to fix conflict.

Now that #2736109: Convert web tests to browser tests for action module is in 8.1.x, this one applies again for 8.1.x. Note that the coding standards fix that was applied on commit needs to be done for 8.1.x as well and also the follow-up #2752315: Fix visibility of AssertLegacyTrait::assertNoEscaped(). Two standard cherry-picks suffice.
It would be good to have AssertLegacyTrait the same in all branches. (And the help tests.)

I tried to requeue against 8.1.x but the bot started testing against 8.2.x. Guess I'll re-upload after the 8.2.x test fails.

Re-uploaded 2735199-15.patch from #17.

When this is green we can simply cherry-pick a142151. (And then 58c3b96 from #2752315: Fix visibility of AssertLegacyTrait::assertNoEscaped())

Unrelated fails in Drupal\field\Tests\Update\FieldUpdateTest. (https://www.drupal.org/pift-ci-job/343553).
Back to RTBC as per #18/ #28.
We'll get fully synced AssertLegacyTrait and help tests if we cherry-pick a142151 here followed by 58c3b96 from #2752315: Fix visibility of AssertLegacyTrait::assertNoEscaped().

This looks also great for me!

Committed 792d444 and pushed to 8.1.x. Thanks!