Modernize Drupal\KernelTests\AssertContentTrait

+++ b/core/tests/Drupal/KernelTests/AssertContentTrait.php
@@ -285,24 +285,13 @@ protected function getAllOptions(\SimpleXMLElement $element) {
+    $this->assertArrayHasKey($index, $links, $message ?: "Link with label $label found.");

@@ -311,25 +300,13 @@ protected function assertLink($label, $index = 0, $message = '', $group = 'Other
+    $this->assertEmpty($links, $message ?: "Link with label $label not found.");

@@ -340,23 +317,11 @@ protected function assertNoLink($label, $message = '', $group = 'Other') {
+    $this->assertArrayHasKey($index, $links, $message ?: "Link containing href $href found.");

@@ -365,23 +330,11 @@ protected function assertLinkByHref($href, $index = 0, $message = '', $group = '
+    $this->assertEmpty($links, $message ?: "No link containing href $href found.");

@@ -390,23 +343,11 @@ protected function assertNoLinkByHref($href, $message = '', $group = 'Other') {
+    $this->assertEmpty($links, $message ?: "No link containing href $href found.");

@@ -1048,34 +753,23 @@ protected function assertNoFieldByXPath($xpath, $value = NULL, $message = '', $g
+        $message = strtr('Found field with name @name', [
...
+        $message = strtr('Found field with name @name and value @value', [

@@ -1089,21 +783,10 @@ protected function assertFieldByName($name, $value = NULL, $message = NULL, $gro
+    $this->assertNoFieldByXPath($this->constructFieldXpath('name', $name), $value, $message ?: "Did not find field by name $name");

@@ -1116,27 +799,16 @@ protected function assertNoFieldByName($name, $value = '', $message = '', $group
+    $this->assertFieldByXPath($this->constructFieldXpath('id', $id), $value, $message ?: "Found field by id $id");

@@ -1150,21 +822,10 @@ protected function assertFieldById($id, $value = '', $message = '', $group = 'Br
+    $this->assertNoFieldByXPath($this->constructFieldXpath('id', $id), $value, $message ?: "Did not find field by id $id");

@@ -1173,22 +834,11 @@ protected function assertNoFieldById($id, $value = '', $message = '', $group = '
+    $this->assertTrue(isset($elements[0]) && !empty($elements[0]['checked']), $message ?: "Checkbox field $id is checked.");

@@ -1197,22 +847,11 @@ protected function assertFieldChecked($id, $message = '', $group = 'Browser') {
+    $this->assertTrue(isset($elements[0]) && empty($elements[0]['checked']), $message ?: "Checkbox field $id is not checked.");

@@ -1223,22 +862,11 @@ protected function assertNoFieldChecked($id, $message = '', $group = 'Browser')
+    $this->assertArrayHasKey(0, $options, $message ?: "Option $option for field $id exists.");

@@ -1250,13 +878,10 @@ protected function assertOption($id, $option, $message = '', $group = 'Browser')
+    $this->assertArrayHasKey(0, $options, $message ?: "Option with text label $text for select field $id exists.");

@@ -1267,22 +892,11 @@ protected function assertOptionByText($id, $text, $message = '') {
+    $this->assertArrayHasKey(0, $options, $message ?: "Option $option for field $drupal_selector exists.");

@@ -1293,23 +907,12 @@ protected function assertOptionWithDrupalSelector($drupal_selector, $option, $me
+    $this->assertTrue(isset($selects[0]) && !isset($options[0]), $message ?: "Option $option for field $id does not exist.");

@@ -1320,24 +923,13 @@ protected function assertNoOption($id, $option, $message = '', $group = 'Browser
+    $this->assertTrue(isset($elements[0]) && !empty($elements[0]['selected']), $message ?: "Option $option for field $id is selected.");

@@ -1348,24 +940,13 @@ protected function assertOptionSelected($id, $option, $message = '', $group = 'B
+    $this->assertTrue(isset($elements[0]) && !empty($elements[0]['selected']), $message ?: "Option $option for field $drupal_selector is selected.");

@@ -1376,22 +957,11 @@ protected function assertOptionSelectedWithDrupalSelector($drupal_selector, $opt
+    $this->assertTrue(isset($elements[0]) && empty($elements[0]['selected']), $message ?: "Option $option for field $id is not selected.");

In PHPUnit, the messages should explain the reason of the failure. These messages seem to be inherited from Simpletest, where they were describing what was being tested. So they should be negated IMHO. Normally I wouldn't bother, but in this case it's a trait used extensively and I think here they should be corrected.

Looks good to me

Looks like it needs a re-roll. Thanks!

Also, the changes in this patch do not look very BC. Can we deprecate the extra parameter and such, rather than changing the signatures? Etc.

Rerolled. IMHO it does not make any sense to go for deprecations here, at least for D9 - what's being removed are Simpletest-isms. They should have been deprecated before Simpletest was removed. But Simpletest has been removed already, and there's no point to do that now.

If this patch will ever be backported to D8 (and I see no strict need for that), then we may want to make some deprecation dance there and then; besides, this patch is introducing usage of PHPUnit-only assertions: if the trait is used also by Simpletest in D8, then we would need to ensure we will not be breaking anything.

We would definitely not backport this to D8. But the minor upgrade path from 9.0 -> 9.1 should be free of BC breaks, even for internal code. Even though SimpleTest isn't in core anymore, this stuff is.

Tagging for RM review.

1. Alright, given that WebAssert::assert() and AssertLegacyTrait::assert() don't actually even take $group anymore, and that assertTextHelper() also isn't doing anything with it, I was wrong and this is BC. If custom code is using the $group parameter, they'd be doing so with func_get_args(), which will continue to work.

   However, what we're missing is notifying custom code that the $group parameter is unused. So if they're expecting it to put the test in a different group, it doesn't. Therefore, I still think we should @trigger_error() if anything other than a NULL value of $group is passed -- especially for assertTextHelper(), but also for the other methods. This should have been done in the first place when it became dead code, but we should definitely fix it before silently removing the parameter from the signature.

   It might also make sense to scope this issue into one patch that deals with dead code $group (maybe beyond the scope of just this trait) and another that deals with improving the not-dead code in the methods.

2. +++ b/core/tests/Drupal/KernelTests/AssertContentTrait.php
   @@ -531,23 +428,12 @@ protected function assertNoEscaped($raw, $message = '', $group = 'Other') {
   -  protected function assertText($text, $message = '', $group = 'Other') {
   -    return $this->assertTextHelper($text, $message, $group, FALSE);
   +  protected function assertText($text, $message = '') {
   +    $this->assertStringContainsString((string) $text, $this->getTextContent(), $message);
   
   @@ -559,23 +445,12 @@ protected function assertText($text, $message = '', $group = 'Other') {
   -  protected function assertNoText($text, $message = '', $group = 'Other') {
   -    return $this->assertTextHelper($text, $message, $group, TRUE);
   +  protected function assertNoText($text, $message = '') {
   +    $this->assertStringNotContainsString((string) $text, $this->getTextContent(), $message);
   

   I was going to say we should deprecate assertTextHelper(), but looks like it already is.

3. +++ b/core/tests/Drupal/KernelTests/AssertContentTrait.php
   @@ -365,23 +330,11 @@ protected function assertLinkByHref($href, $index = 0, $message = '', $group = '
   -   *   (optional) A message to display with the assertion. Do not translate
   -   *   messages: use \Drupal\Component\Render\FormattableMarkup to embed
   -   *   variables in the message text, not t(). If left blank, a default message
   -   *   will be displayed.
   ...
   +   *   (optional) A message to display with the assertion.
   

   Why are we deleting all these docs? AFAIK we still don't hav a PHPCS sniff to ensure the message parameter is not translated, so these are still relevant. (I guess "half-relevant" since we've also moved away from FormattableMarkup for them as well.) The bit about "a default message will be displayed" is also still true.

Thanks!

+1 for deprecating unused methods. This trait was introduced back in Simpletest times, I feel like most of the methods listed in #20 probably make sense in the context of functional tests, but there we have WebAssert now. So let's deprecate to remove in D10, and then if some have merit to stay someone will hopefully speak up.