Comments

klausi created an issue. See original summary.

klausi’s picture

Status: Needs review » Needs work

The last submitted patch, 2: action-browsertest-2736109-2.patch, failed testing.

andypost’s picture

dawehner’s picture

  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?

klausi’s picture

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.

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

dawehner’s picture

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.

naveenvalecha’s picture

Title: Convert webt tests to browser tests for action module » Convert web tests to browser tests for action module
klausi’s picture

Status: Needs work » Needs review
FileSize
8.72 KB

Not finished yet, trying to introduce proper helper methods.

Not interdiff, because not useful.

Status: Needs review » Needs work

The last submitted patch, 9: action-browsertest-2736109-9.patch, failed testing.

klausi’s picture

Assigned: klausi » Unassigned
Status: Needs work » Needs review
FileSize
11.59 KB
5.03 KB

Completed the necessary helper methods.

Status: Needs review » Needs work

The last submitted patch, 11: action-browsertest-2736109-11.patch, failed testing.

klausi’s picture

Status: Needs work » Needs review
FileSize
11.69 KB
933 bytes

Forgot to fix the namespaces.

Status: Needs review » Needs work

The last submitted patch, 13: action-browsertest-2736109-13.patch, failed testing.

dawehner’s picture

  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?

klausi’s picture

Status: Needs work » Needs review
FileSize
13.3 KB
2.89 KB

Overriding cleanUrl() from upstream WebAssert now where we can strip testbot's base path.

Contributing back upstream might make sense, but let's wait for more conversions to see what we need.

dawehner’s picture

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?

klausi’s picture

No, it was an absolute URL in WebTestBase, so let's keep it that way. Mink also seems to like to deal with absolute URLs as with the getCurrentUrl() method.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Ah okay, I didn't knew that

alexpott’s picture

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.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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.

  • alexpott committed 58256f9 on 8.2.x
    Issue #2736109 by klausi, dawehner: Convert web tests to browser tests...
Eric_A’s picture

Version: 8.2.x-dev » 8.1.x-dev
Status: Fixed » Needs review
FileSize
13.26 KB

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.

Status: Needs review » Needs work

The last submitted patch, 23: action-browsertest-2736109-23.patch, failed testing.

Eric_A’s picture

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

Let's retest.

Eric_A’s picture

Status: Needs work » Reviewed & tested by the community

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;
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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

  • alexpott committed ac3a539 on 8.1.x
    Issue #2736109 by klausi, Eric_A, dawehner: Convert web tests to browser...
Eric_A’s picture

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.

Status: Fixed » Closed (fixed)

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