Problem/Motivation

For better compatibility with existing Simpletests we should try to provide assertFieldByXPath(), but mark it as deprecated. That way we need to change fewer parts of a test when converting from Simpletest to BrowserTestBase.

Proposed resolution

Implement the method, test it in BrowserTestBaseTest.

Remaining tasks

Patch.

User interface changes

None.

API changes

None.

Data model changes

None.

Comments

klausi created an issue. See original summary.

klausi’s picture

Status: Active » Needs review
FileSize
4.27 KB

patch.

mpdonadio’s picture

Probably also want assertNoFieldByXpath(), too?

mpdonadio’s picture

+++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
@@ -464,6 +465,73 @@ protected function assertNoFieldChecked($id) {
+   *   (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.
+   *

Mention FormattableMarkup instead?

Otherwise looks good, but I would vote for also adding assertNoFieldByXpath(). PhpStorm reports 49 usages in 8.2.x.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
index 5c1e3f5..cd34d4b 100644
--- a/core/modules/simpletest/tests/src/Functional/BrowserTestBaseTest.php

--- a/core/modules/simpletest/tests/src/Functional/BrowserTestBaseTest.php
+++ b/core/modules/simpletest/tests/src/Functional/BrowserTestBaseTest.php

+++ b/core/modules/simpletest/tests/src/Functional/BrowserTestBaseTest.php
@@ -86,4 +86,19 @@ public function testError() {

Added a small issue: #2784677: Move core/modules/simpletest/tests/src/Functional/BrowserTestBaseTest.php to core/tests/Drupal/FunctionalTests/BrowserTestBaseTest.php

dawehner’s picture

Status: Reviewed & tested by the community » Needs work

Mention FormattableMarkup instead?

IMHO we should simply use sprintf or string concatenation. We are not creating markup here.

klausi’s picture

yes, assertNoFieldByXpath() is missing, we should do that here as well.

Ah yes, the comment for $message is just copy pasta from Simpletest. Of course you can just use string concatenation for messages.

mpdonadio’s picture

IMHO we should simply use sprintf or string concatenation. We are not creating markup here.

Getting a bit off topic here, but we aren't terribly consistent about this since all of the SafeMarkup related changes went down. I had a patch get pushed back with the request to use FormattableMarkup for a message a little while ago. Personally, I think we should either add a BTB::format() method (via a trait) or add an extra $args argument to the assertion methods that take a $message, and either would just do a simple `$message = strtr($message, $args);` I think I may have even made an issue about this, and then forgotten to follow through on it.

klausi’s picture

Status: Needs work » Needs review
FileSize
5.18 KB
3.52 KB

Added assertNoFieldByXPath() and improved comments to not even mention SafeMarkup and friends, like we do on the other AssertLegacyTrait methods.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 9: assert-field-xpath-2784537-9.patch, failed testing.

klausi’s picture

Status: Needs work » Needs review
FileSize
5.14 KB

Simple reroll, no other changes.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 12: assert-field-xpath-2784537-12.patch, failed testing.

klausi’s picture

Status: Needs work » Needs review

Random test fail, queuing that test again.

Status: Needs review » Needs work

The last submitted patch, 12: assert-field-xpath-2784537-12.patch, failed testing.

mpdonadio’s picture

Status: Needs work » Reviewed & tested by the community

Two good test runs after the update spurious. Back to RTBC per #13.

catch’s picture

Status: Reviewed & tested by the community » Needs review

These are 1-1 copies from Simpletest's assertFieldByXPath()/assertNoFieldByXPath() - could we not move them into a shared trait then use that in both AssertContentTrait and AssertLegacyTrait?

dawehner’s picture

These are 1-1 copies from Simpletest's assertFieldByXPath()/assertNoFieldByXPath() -

Yes and no.

+++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
@@ -464,6 +465,103 @@ protected function assertNoFieldChecked($id) {
+          elseif ($field->find('xpath', '//option[@value = ' . (new Escaper())->escapeLiteral($value) . ' and @selected = "selected"]')) {

This is using a different function than the simpletest alternative.

catch’s picture

Title: Add legacy assertFieldByXPath() method for browser tests » Add legacy assertFieldByXPath()/assertNoFieldByXPath()/assertNoFieldChecked() method for browser tests

Well I mentioned assertFieldByXpath and assertNoFieldByXpath() not assertNoFieldChecked() ;)

dawehner’s picture

Personally I'm not convinced that this is worth the hassle.

Status: Needs review » Needs work

The last submitted patch, 22: 2784537-22.patch, failed testing.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
8.46 KB
1.8 KB

This should fix the fails.

Status: Needs review » Needs work

The last submitted patch, 24: 2784537-24.patch, failed testing.

mpdonadio’s picture

+++ b/core/modules/simpletest/src/AssertXpathTrait.php
@@ -0,0 +1,76 @@
+          if ($field->getAttribute('value') === $value) {

The problem is that $field is a \SimpleXMLElement when run from WTB and a \Behat\Mink\Element\NodeElement when run from a BTB. Not sure if there is a common way to handle these?

dawehner’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
5.14 KB

The problem is that $field is a \SimpleXMLElement when run from WTB and a \Behat\Mink\Element\NodeElement when run from a BTB. Not sure if there is a common way to handle these?

BTB and WTB are really two separate things. Let's not try to share code for the sake of it.
Here is a reupload of #12 with a RTBC

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 27: assert-field-xpath-2784537-12.patch, failed testing.

mglaman’s picture

Requed #27. Pulled latest 8.3.x and ran \Drupal\FunctionalTests\BrowserTestBaseTest without failures.

mglaman’s picture

Status: Needs work » Reviewed & tested by the community

The test had passed. Must have been a hiccup, putting back to RTBC as dawehner had done in #27!

mpdonadio’s picture

The fail in #27 was in the new test:

Browsertestbase.Drupal\FunctionalTests\BrowserTestBaseTest
✓		- testGoTo
✓		- testForm
✓		- testError
✗	
testAssertions
fail: [Other] Line 92 of core/tests/Drupal/FunctionalTests/BrowserTestBaseTest.php:

which is a bit of a warning sign.

The attached fixes a few nits, but I don't think they should have caused this fail (since PHP method names are case insensitive for some odd reason).

dawehner’s picture

AH good observation!

mpdonadio’s picture

The PHP 5.6 fails are #2762549: Drupal\field\Tests\Update\FieldUpdateTest, Drupal\views\Tests\Update\EntityViewsDataUpdateTest and Drupal\comment\Tests\CommentFieldsTest fail on 8.1.x. I can't make BrowserTestBaseTest fail to reproduce the bot fail to see what actually happened.

klausi’s picture

Status: Needs review » Reviewed & tested by the community

So the random test fails are already covered in another issue and this issue does not affect them in any way.

Patch still looks good, back to RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 31: 2784537-31.patch, failed testing.

mpdonadio’s picture

Simpletest.Drupal\simpletest\Tests\SimpleTestBrowserTest
✓ - setUp
✓ - testInternalBrowser
✓ - testUserAgentValidation

testTestingThroughUI
fail: [Other] Line 143 of core/modules/simpletest/src/Tests/SimpleTestBrowserTest.php:
"0 fails, 0 exceptions" found
This is a random failed covered in #2476139: Fix UI test fail in SimpleTestBrowserTest and/or #2605664: [Needs change record updates] Align TestDiscovery and bootstrap.php's non-permissive loading of PSR-4 namespaces for traits

mpdonadio’s picture

Status: Needs work » Reviewed & tested by the community

Random fail covered in a different issue. Back to RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 31: 2784537-31.patch, failed testing.

klausi’s picture

Status: Needs work » Reviewed & tested by the community

Random test fail again, back to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
@@ -464,6 +465,103 @@ protected function assertNoFieldChecked($id) {
+  protected function assertFieldsByValue($fields, $value = NULL, $message = '') {

There's no test coverage of this method and given it is the most complex it probably requires it the most.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
5.57 KB

This needs a reroll b/c #2773733: Fix AssertLegacyTrait::assert(No)Text to handle html encoded strings (part 3).

While I did the reroll, I split testLegacyAsserts() so it wouldn't become a mega-method (the main merge conflict was here), and also fixed some small case problems.

Will see if I can address #40 next. It is implicitly tested since assertFieldsByValue is called by assertFieldByXPath(), but see if I canl make this more explicit.

klausi’s picture

Title: Add legacy assertFieldByXPath()/assertNoFieldByXPath()/assertNoFieldChecked() method for browser tests » Add legacy assertFieldByXPath()/assertNoFieldByXPath() method for browser tests
Status: Needs review » Reviewed & tested by the community
Issue tags: +Dublin2016

Thank you, the changes look good!

We already have assertNoFieldChecked(), removing from the title.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 42: 2784537-42.patch, failed testing.

klausi’s picture

I have no idea why the patch failed testing. Works as expected locally for me. Retesting.

klausi’s picture

Status: Needs work » Reviewed & tested by the community

Now working as advertised on the testbot, back to RTBC.

klausi’s picture

Status: Reviewed & tested by the community » Needs work

So this issue introduces a random test fail, it just happened again in #2770921: Feb 21st: Convert chunk of WTB to BTB by just moving classes, changing use statements adding traits. We need to find out what exactly goes wrong here.

The last submitted patch, 42: 2784537-42.patch, failed testing.

mpdonadio’s picture

I did

../vendor/bin/phpunit --stop-on-failure --repeat 5000 --verbose --debug tests/Drupal/FunctionalTests/BrowserTestBaseTest.php

and it ran overnight w/o a fail. So, we either have an environment problem on the bots or something weird is going on with this test and concurrency.

klausi’s picture

Status: Needs review » Needs work

The last submitted patch, 50: assert-field-xpath-2784537-50.patch, failed testing.

klausi’s picture

dawehner’s picture

+++ b/core/modules/simpletest/src/Tests/SimpleTestBrowserTest.php
@@ -131,7 +131,7 @@ public function testTestingThroughUI() {
+      'Drupal\FunctionalTests\Breadcrumb\Breadcrumb404Test',

IMHO The right thing is to add its own dedicated test, just for that. I think there should be a dedicated rule for tests, that there should be test fixtures, instead of reusing other code, as you know, tests for example aren't an API, so you should not rely on their existence.

IMHO this change is also unrelated.

klausi’s picture

The change is related to this issue because we are making BrowserTestBaseTest longer, which causes random test fails in SimpleTestBrowserTest. Agreed that SimpleTestBrowserTest should use a dedicated test fixture, let's do that in a separate issue.

Removing the debug statements from the test, let's see if we still get random fails.

klausi’s picture

Yay, no random test failures anymore. I'll queue another round just to make sure.

Opened #2820442: SimpleTestBrowserTest has random fails when BrowserTestBaseTest gets larger.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Sorry for maybe being a bit annoying here :(

The change is related to this issue because we are making BrowserTestBaseTest longer, which causes random test fails in SimpleTestBrowserTest. Agreed that SimpleTestBrowserTest should use a dedicated test fixture, let's do that in a separate issue.

It would be nice to understand where this is coming from, so let's open up a follow up for itself: #2820458: SimpleTestBrowserTest is really fragile

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 54: assert-field-xpath-2784537-54.patch, failed testing.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

That's still RTBC

catch’s picture

Looks like an unrelated random fail here: https://www.drupal.org/pift-ci-job/524001 - sent for re-test.

mpdonadio’s picture

#61, actually this is really odd and possibly a clue.

Datetime.Drupal\datetime_range\Tests\DateRangeFieldTest
✗	
testDateRangeField
fail: [Other] Line 214 of core/modules/datetime_range/src/Tests/DateRangeFieldTest.php:
Value false is TRUE.

is this assertion

$this->assertFieldByXPath('//time[@data-field-item-attr="foobar"]');

though that test is currently a WTB (the particular assertion is a recent addition, #2811725: Error when render Datetime Range field: Error: Unsupported operand types). The Mink and SimpleXML XPath guts aren't compatible (see one of the earlier patches where we tried to reuse the trait between BTB and WTB). So, I am wondering if either the classloader or opcache is having problems with the trait and using the wrong one, or of if the XML library that evenutally gets called (libxml?) isn't 100% reentrant.

catch’s picture

Status: Reviewed & tested by the community » Needs review

Hmm interesting. Moving back to CNR, this is worth some investigating, although if it's a real issue that's pretty horrible.

Status: Needs review » Needs work

The last submitted patch, 58: assert-field-xpath-2784537-58.patch, failed testing.

mpdonadio’s picture

https://www.drupal.org/pift-ci-job/525542

Browsertestbase.Drupal\FunctionalTests\BrowserTestBaseTest
✗	
rupal\FunctionalTests\BrowserTestBaseTe
fail: [Other] Line 0 of sites/default/files/simpletest/phpunit-170.xml:
PHPunit Test failed to complete; Error: PHPUnit 4.8.27 by Sebastian Bergmann and contributors.

......F

Time: 1.5 minutes, Memory: 2.75Mb

There was 1 failure:

1) Drupal\FunctionalTests\BrowserTestBaseTest::testLegacyXPathAsserts
Failed asserting that false is true.

/var/www/html/core/tests/Drupal/KernelTests/AssertLegacyTrait.php:31
/var/www/html/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php:607
/var/www/html/core/tests/Drupal/FunctionalTests/BrowserTestBaseTest.php:156

Same type of prob, but in the BTB version

$this->assertFieldsByValue($this->xpath('//table/tbody/tr[2]/td[1]/span'), $account->getAccountName());

I wonder if we can test this hypothesis by adding something like

$this->assertEquals(getClass($fields), '\Behat\Mink\Element\NodeElement');

and the SimpleXML equivalent in the appropriate places?

mpdonadio’s picture

Experiment. Going to hog a few bots. Did we dig through the logs to see if this is one particular bot, or a mix of them?

klausi’s picture

Good news: I could reproduce the BrowserTestBaseTest fail when the role name of the created user contains a pipe (|). So this is yet another instance of #2808085: Pipe char in locators break Mink and Symfony element search.

Bad news: I have no idea about the random DateTime test fail.

klausi’s picture

Status: Needs review » Needs work

Update: not the pipe character is at fault but the ordering of the user table on the /admin/people page. Sometimes the newly created user is in the first row because the table is sorted by user creation date, which can be almost at the same time as the admin user was created.

This is really my fault because I was lazy and just used an existing admin page. Instead we should create a dedicated test page with a table on it with static content for this test. Sorry for wasting everybody's time.

klausi’s picture

Testbot is all green in all environments, queuing another full round to be sure.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/modules/system/tests/modules/test_page_test/src/Form/TestForm.php
@@ -0,0 +1,66 @@
+      '#header' => array('Column 1', 'Column 2', 'Column 3'),
+      'row_1' => [
+        'col_1' => ['#plain_text' => 'foo'],
+        'col_2' => ['#plain_text' => 'bar'],
+        'col_3' => ['#plain_text' => 'baz'],
+      ],
+      'row_2' => [
+        'col_1' => ['#plain_text' => 'one'],
+        'col_2' => ['#plain_text' => 'two'],
+        'col_3' => ['#plain_text' => 'three'],
+      ],
+    ];

Deterministic and explicit test code, yeah! nitpick: There is a single array which sneaked in.

dawehner’s picture

Cool, thank you!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 73: assert-field-xpath-2784537-73.patch, failed testing.

joelpittet’s picture

Status: Needs work » Reviewed & tested by the community

Testbot failure, resetting status due to https://www.drupal.org/node/2828045

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 73: assert-field-xpath-2784537-73.patch, failed testing.

klausi’s picture

Status: Needs work » Reviewed & tested by the community

Still green, not sure what the bot complained about? Will queue all environments again just to be sure.

xjm’s picture

Wow, this issue has more than its fair share of weird test problems, which always makes me nervous for testing improvement issues. When we see unexplained test failures, we should always try to explain them (in general, but also especially when we are actually patching the testing system). Looking them over:

  1. An earlier BTB fail (I think https://www.drupal.org/pift-ci-job/509040 and https://www.drupal.org/pift-ci-job/525542?) was caused by #2808085: Pipe char in locators break Mink and Symfony element search.
  2. A bunch like https://www.drupal.org/pift-ci-job/450381 were when we broke that in HEAD back in September.
  3. https://www.drupal.org/pift-ci-job/508994 and kin were fixed in #52 I think.
  4. The French translation failures like https://www.drupal.org/pift-ci-job/531461 were #2828045: Tests failing with unrelated „Translation: The French translation is not available”.
  5. The DateRangeFieldTest fails like https://www.drupal.org/pift-ci-job/524001 and https://www.drupal.org/pift-ci-job/530666 are almost certainly #2829848: Random test failure in DateRangeFieldTest, though it's quite interesting that this issue seems to be hitting them more.
  6. The SQLite fail https://www.drupal.org/pift-ci-job/539394 is probably #2828559: UpdatePathTestBase tests randomly failing.
  7. https://www.drupal.org/pift-ci-job/508999 is the entity autocomplete failure I've seen before, #2831603: Improve assertion for testEntityReferenceAutocompleteWidget.
  8. https://www.drupal.org/pift-ci-job/509784 looks like #2579543: Random segfault in MigrateUserProfileEntityDisplayTest.
  9. https://www.drupal.org/pift-ci-job/512529 and https://www.drupal.org/pift-ci-job/530629 are #2806697: Random fail for AlreadyInstalledException.
  10. https://www.drupal.org/pift-ci-job/538686 has happened twice before in HEAD: https://www.drupal.org/pift-ci-job/498883 and https://www.drupal.org/pift-ci-job/490797

So I think that accounts for all of them. Good gracious.

I'll also retest everything again.

xjm’s picture

The retests passed, which is good. Phew.

Patch looks great BTW.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 73: assert-field-xpath-2784537-73.patch, failed testing.

xjm’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 73: assert-field-xpath-2784537-73.patch, failed testing.

klausi’s picture

Status: Needs work » Reviewed & tested by the community

SQLite is broken on HEAD also, back to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
@@ -510,6 +511,103 @@ protected function assertNoFieldChecked($id) {
+   *   Use $this->assertSession()->fieldNotExists() or
+   *   $this->assertSession()->fieldValueNotEquals() instead.
...
+   *   Iterate over the fields yourself instead and directly check the values in
+   *   the test.

I think we should include info about #2767655: Allow WebAssert to inspect also hidden fields and point to the new methods on assertSession since if you are using this methods a straight conversion using assert session's field methods might not work.

klausi’s picture

@alexpott: assertFieldByXpath() takes an XPath expression as parameter. fieldExists() and hiddenFieldExists() take id|name|label|value as parameter. You can't convert your old assertFieldByXpath() calls to those methods.

assertFieldByXpath() is a very unfortunate name in old Simpletest. It is not specific at all to form fields, it works on any HTML element. The direct replacement to assertFieldByXpath() is executing the XPath query directly in the test and then working with the resulting DOM nodes.

You are right that the comment on assertNoFieldByXPath() is misleading, brought that into sync with assertFieldByXPath().

claudiu.cristea’s picture

Looks good. Only a nit regarding the deprecation message.

+++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
@@ -510,6 +511,102 @@ protected function assertNoFieldChecked($id) {
+   * @deprecated Scheduled for removal in Drupal 9.0.0.
...
+   * @deprecated Scheduled for removal in Drupal 9.0.0.
...
+   * @deprecated Scheduled for removal in Drupal 9.0.0.

It cannot be removed exactly in 9.0.0. Either "in 9.0.x" or "before 9.0.0".

klausi’s picture

@claudiu.cristea: that deprecation message is used already in many other places, so should be fine. The method will be removed in a 9.0.x branch, but to developers only releases are interesting.

claudiu.cristea’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Dublin2016

Let the committers decide.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 87: assert-field-xpath-2784537-87.patch, failed testing.

klausi’s picture

Status: Needs work » Reviewed & tested by the community

Random test fail, already handled in #2828559: UpdatePathTestBase tests randomly failing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 87: assert-field-xpath-2784537-87.patch, failed testing.

mpdonadio’s picture

Status: Needs work » Reviewed & tested by the community
xjm’s picture

Assigned: Unassigned » alexpott

Assigning to @alexpott for review based on #86 and #87.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 87: assert-field-xpath-2784537-87.patch, failed testing.

klausi’s picture

Status: Needs work » Reviewed & tested by the community

And again the same random #2828559: UpdatePathTestBase tests randomly failing failures, back to RTBC.

alexpott’s picture

Assigned: alexpott » Unassigned
Status: Reviewed & tested by the community » Fixed

Committed 3cd6934 and pushed to 8.3.x. Thanks!

@klausi thanks for considering and answering my questions.

  • alexpott committed 3cd6934 on 8.3.x
    Issue #2784537 by klausi, mpdonadio, dawehner: Add legacy...

Status: Fixed » Closed (fixed)

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

klausi’s picture