Add legacy assertFieldByXPath()/assertNoFieldByXPath() method for browser tests

Probably also want assertNoFieldByXpath(), too?

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

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

Mention FormattableMarkup instead?

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

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.

Thank you

Back to RTBC

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

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?

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.

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

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

This should fix the fails.

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

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

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

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

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

AH good observation!

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.

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

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

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

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.

This should cover all of the TRUE paths through assertFieldsByValue().

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.

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

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

That's still RTBC

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

#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.

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

Are we getting bit by #2605664: [Needs change record updates] Align TestDiscovery and bootstrap.php's non-permissive loading of PSR-4 namespaces for traits?

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?

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?

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

Cool, thank you!

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

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 JSWebAssert::waitOnAutocomplete.
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.

The retests passed, which is good. Phew.

Patch looks great BTW.

Yeah https://www.drupal.org/pift-ci-job/542379 is #2828559: UpdatePathTestBase tests randomly failing.

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

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".

Let the committers decide.

More #2828559: UpdatePathTestBase tests randomly failing failures...

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

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

@klausi thanks for considering and answering my questions.