Convert web tests to browser tests for action module

1. +++ b/core/modules/action/tests/src/Functional/ActionUninstallTest.php
   @@ -27,7 +27,7 @@ public function testActionUninstall() {
   -    $this->assertTrue($storage->load('user_block_user_action'), 'Configuration entity \'user_block_user_action\' still exists after uninstalling action module.' );
   +    $this->assertNotNull($storage->load('user_block_user_action'), 'Configuration entity \'user_block_user_action\' still exists after uninstalling action module.' );
   
   +++ b/core/modules/action/tests/src/Functional/BulkFormTest.php
   @@ -47,7 +47,7 @@ public function testBulkForm() {
   -    $this->assertTrue($first_form_element, 'The views form edit header appears first.');
   +    $this->assertNotEmpty($first_form_element, 'The views form edit header appears first.');
   

   I'm wondering whether we should use assertInstanceOf to be a little bit more strict. In general +1 for the general idea to be more semantic than just assertTrue

2. +++ b/core/modules/action/tests/src/Functional/BulkFormTest.php
   @@ -123,7 +123,7 @@ public function testBulkForm() {
   -    $this->assertEqual('With selection', (string) $result[0]);
   +    $this->assertEqual('With selection', $result[0]->getText());
   
   @@ -133,7 +133,7 @@ public function testBulkForm() {
   -    $this->assertEqual('Test title', (string) $result[0]);
   +    $this->assertEqual('Test title', $result[0]->getText());
   

   Now that assertEquals would deal with it, do you think we should keep this change?

I had to replace assertTrue() because phpunit does a "===" comparison, so the test was failing :) I think we should use assertNotNull() and assertNotEmpty() here since that was the intention when the test was written.

Fair point.

assertEquals() does not deal with mink node elements (they don't have a __toString()), so we need the ->getText() call here.

Ah right, we have a different object now!

So yeah IMHO this is "RTBC" beside the necessary reroll.

1. +++ b/core/modules/action/tests/src/Functional/BulkFormTest.php
   @@ -123,7 +123,7 @@ public function testBulkForm() {
   -    $this->assertEqual('With selection', (string) $result[0]);
   +    $this->assertEqual('With selection', $result[0]->getText());
   
   @@ -133,7 +133,7 @@ public function testBulkForm() {
   -    $this->assertEqual('Test title', (string) $result[0]);
   +    $this->assertEqual('Test title', $result[0]->getText());
   

   I love that!

2. +++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
   @@ -149,4 +186,48 @@ protected function assertLink($label, $index = 0) {
   +  protected function assertUrl($path) {
   +    $path = "/$path";
   +    $this->assertSession()->addressEquals($path);
   +  }
   

   This is not the right way to be honest. We need to take into account the subfolder in which drupal is installed. Actually I think providing our own implementation of addressEquals would be a good idea in general.

3. +++ b/core/tests/Drupal/Tests/WebAssert.php
   @@ -67,6 +67,71 @@ public function selectExists($select, TraversableElement $container = NULL) {
   +  public function optionExists($select, $option, TraversableElement $container = NULL) {
   ...
   +  public function optionNotExists($select, $option, TraversableElement $container = NULL) {
   

   I'm wondering actually whether we maybe want to try to move some of the stuff upstream?

Oh, thats quite nice!

+++ b/core/tests/Drupal/Tests/BrowserTestBase.php
@@ -1762,6 +1766,16 @@ protected function drupalGetHeader($name) {
   /**
+   * Get the current URL from the browser.
+   *
+   * @return string
+   *   The current URL.
+   */
+  protected function getUrl() {
+    return $this->getSession()->getCurrentUrl();
+  }
+

Should we convert that URL back to something without the base path?

Ah okay, I didn't knew that

We need to fix the Simpletest UI before doing this :( - see #2575725: Run PHPunit tests one at a time via UI to avoid exception when selecting too many - it tries to run all phpunit tests outside of the batch.

Committed 58256f9 and pushed to 8.2.x. Thanks!

Couldn't be applied to 8.1.x. In general it's nice when tests stay in sync but where there are conflicts commit to 8.2.x and moving on seems sensible.

If we don't port anything at all here, we could encounter issues in the near future when cherry-picking non-action fixes and their tests, because the commit here introduced changes/additions/fixes to WebAssert, BrowserTestBase, AssertLegacyTrait in 8.2.x, apart from the actual conversion of action tests.
Porting would also be very nice for contrib, but the possible near-future core issues make it worthwhile to fix one conflict now, instead of possibly many later.

The conflict is just one line of context, a use statement introduced by #2723521: Remove entity_load* usage for action entity type. Here's a 8.1.x patch.

testUpdateImportSourceRemote
fail: [Other] Line 139 of core/modules/locale/src/Tests/LocaleUpdateTest.php:
Updates for Contrib module one

Let's retest.

It's the exact same patch as the one committed six days ago, apart from the one line of context that is not in 8.1.x due to the unrelated entity_load() cleanup in 8.2.x.

 use Drupal\system\Entity\Action;

It's nice to have tests in all current branches the same so committed ac3a539 and pushed to 8.1.x. Thanks!

Thanks!

It's nice to have tests in all current branches the same so committed

Now that this is in we can cherry-pick a142151 from #2735199: Convert web tests to browser tests for help module and 58c3b96 from #2752315: Fix visibility of AssertLegacyTrait::assertNoEscaped() and then we've fully synced AssertLegacyTrait and help tests.