Some test assertions are inconsistent with the others

Thanks, this looks mostly really good! I was not sure why the parameter here was $other instead of the standard $group though?

+  protected function assertFieldById($id, $value = '', $message = '', $other = 'Browser') {
+    return $this->assertFieldByXPath($this->constructFieldXpath('id', $id), $value, $message ? $message : t('Found field by id @id', array('@id' => $id)), $other);

I have a feeling that was just a typo.

This one is missing a space in $message ='':

+  protected function assertMailString($field_name, $string, $email_depth, $message ='', $group = 'E-mail') {

Also, a couple of the new default $group parameters changed what had tacitly been the default before. For instance, assertMailString now has a default of 'E-mail', but it never used a group before, so it would have had assertTrue's generic default of 'Other'. I don't think this patch should probably try to change existing tacit defaults? maybe... I guess it's also good if all the mail-related asserts have the same default group? I'm undecided.

RE #5 $group params - I saw that on *some* of the functions, but others seemed to be added with 'E-Mail' as the default that didn't pass 'E-Mail' in previously (so they were defaulting to 'Other').

Wow, I don't know how the tests passed on this patch. There's a typo in assertMailString():

+      $message = format_stringt('Expected text found in @field of email message: "@expected".', array('@field' => $field_name, '@expected' => $string));

format_stringt -> format_string

I guess that assertMailString() must not actually be used anywhere in our core tests.

The only other thing I noticed was one spot where this patch removed the blank line in documentation between the @param section and the @return section. There should always be a blank line in there.

Other than that, it looks good!

Thanks! I think this is *almost* good to go.

I just applied the patch, and took a look at all of the test classes' functions that started with "assert" to see if they are all now consistently having $message and $group parameters. They're almost all great!

I found just a few more spots that I think need attention:

a) WebTestBase

   * @param $group
   *   (optional) The group this message is in, which is displayed in a column
   *   in test output. Use 'Debug' to indicate this is debugging output. Do not
   *   translate this string. Defaults to 'Other'; most tests do not override
   *   this default.
   * @param $not_exists
   *   TRUE if this text should not exist, FALSE if it should.
   * @return
   *   TRUE on pass, FALSE on fail.
   */
  protected function assertTextHelper($text, $message = '', $group, $not_exists) {

There is no default for $group but the docs say it's optional... It is a helper function, so it's probably OK that it not have a default for $group, but the docs should not say that $group is optional. I think the right fix is just to take (optional) out of the docs.

b) The same also applies for assertUniqueTextHelper().

c) assertMailString() needs a space in its signature line in the $message parameter:

 protected function assertMailString($field_name, $string, $email_depth, $message ='', $group = 'Other') {

d) TestBase::assertIdenticalObject() has a default of '' for the $group parameter, and this paramter is not even passed to assertTrue() at the bottom of the function. So it needs to be fixed in the same way you already fixed the other methods.

Yeah! Assuming the test bot agrees, this is ready to go!

Note to final reviewer/committer: The issue summary outlines the problem... The idea of this patch is that all assert functions in the test base classes will now have both $message and $group parameters, so these can be overridden by the calling tests. All of the functions had implicit defaults for these before, but in many cases they could not be overridden. Now they can.

The patch has been carefully constructed in this issue with defaults that should not change the implicit defaults that were present before, which is why some of them have $group = 'Browser' etc. rather than the usual $group = 'Other'.

Hope these notes help... thanks kbasarab for your persistence and attention to detail on this issue! Nice work!

I'm actually not sure this does need a change notification. We were very careful in this patch to preserve exactly the previous behavior of every assertion message, while adding the ability to pass $message and $group to all of the functions if desired. So in reality, there is nothing that any test class would need to do to their tests in response to this change, and their tests should behave exactly the same as they used to.

I thought we only needed change notifications if developers or someone else would need to change something (code, docs, etc.)?