[DX] `assertFieldChecked` and `assertNoFieldChecked` also apply to radios, but they only account for checkboxes

[klonos]

The checked property applies to both type="checkbox" and type="radio", yet BackdropWebTestCase::assertFieldChecked and BackdropWebTestCase::assertFieldChecked only mention checkboxes.

The docblock description for these functions says:

  /**
   * Asserts that a checkbox field in the current page is checked.
   *

And the default messages for these assertions are Checkbox field @id is checked. and Checkbox field @id is not checked..

We should change that to account/mention both radios and checkboxes.

[klonos]

...while at it, let's also add group as an optional parameter (with the default set to what it is now), so that we allow it to be altered (instead of hard-coding it to the returned assertTrue).

[klonos]

...and also, as per the #5753 meta., we should remove t() from the messages in these functions:

• The $message parameter of SimpleTest assertions (e.g., DrupalTestCase::assertTrue()) is a string displayed only in the administrative UI or on the commandline following test runs, and on testbot.
• This parameter is never translated, so using t() on it is needless overhead.
• We have other instances in core where string function parameters should not be wrapped in t(), e.g. watchdog().
• When the assertion message is a plain string, t() should simply be omitted.
• When the assertion message contains placeholders for variables, format_string() should be used instead.

[indigoxela]

@klonos your PR produces loads of "Array to string conversion" with check_plain(), because $this->xpath() outputs an array, but you handle its output as string in format_string.

Personally, I'd not recommend to mix unrelated topics in one issue and PR, BTW.

And ... have you tried to run phpcs on it? :wink: Your changes would probably fail validation (only assuming, we don't run phpcs via GHA yet.)

[klonos]

Thanks @indigoxela ...yes, I saw the failures and meant to return to this, but got into other rabbit holes in the meantime and started having fun 😅

[klonos]

And ... have you tried to run phpcs on it? 😉

Nope. ...I just filed this PR by directly editing the file via the GitHub web UI, because it seemed "simple" - I didn't even set anything up on my local to begin with. I did now though ...and I pushed changes that fixed things.

[indigoxela]

Out of curiosity: that $group (like $group = 'Browser') - does it have any actual use? If so, what does it do?

[klonos]

It seems to be used to group assertions. All docblocks after https://github.com/backdrop/backdrop/blob/1.x/core/modules/simpletest/backdrop_web_test_case.php#L358 say:

   * @param $group
   *   The type of assertion - examples are "Browser", "PHP".

My understanding is that it's similar to the $type parameter in watchdog_deprecated_function().

[indigoxela]

My understanding is that it's similar to the $type parameter in watchdog_deprecated_function()

I can see the type of a watchdog_deprecated_function (like drupal or taxonomy) on the dblog and details pages, but I've no clue, where these assertion groups could get used.

EDIT, never mind, it's obvious where it get's used. :laughing: On the test result page. (Honestly, I never paid attention to that table column.)

[klonos]

Yeah, my understanding is that the "Browser" and "PHP" that are suggested by the docblocks are being used to distinguish for example when using assertEqual() if the two values compared are php variables (and don't say that everything is a php variable 😅 ) or browser/HTML elements that have been acquired via xpath. But since the parameter is free text, it can be anything that helps developers really.

Anyway, the way it was being used in assertFieldChecked() and assertNoFieldChecked() was hard-coded. So I followed the pattern being used in other assertions, and provided a default value instead. That allows people to customize the message returned by these two functions if needed, whereas before they couldn't.

PS: I discovered all this while working on the tests for #3237.