assertFieldById & friends should explain how to skip checking of the field value

Thanks! This needs to be fixed in Drupal 8 on:
https://api.drupal.org/api/drupal/core!modules!simpletest!lib!Drupal!sim...
first (and on related field assertion functions), and then backported to Drupal 7.

Probably a good Novice project?

Attached a patch - wasn't sure if the function signature should be changed from '' to NULL or it was just the preceding comment.

opps, proper patch attached...

opps. thanks for the clarification. Does the new comment make sense?

gotcha - no need to apologize... rather than recreating patches, what do you think of this? pending confirmation, i'll redo the patch:

Value of the field to assert. You may pass in NULL to skip checking the actual value while still checking that the field exists.

awesome. thanks for the help. patch attached.

opps, didn't realize we had changed the status...

Thanks! The documentation change looks good.

But if we're fixing up the docs for this function, could we also fix "id" and "Id" to be "ID"? The problem is that "id" in English is a psychological term (ego, superego, id) and to say "identifier" you need to have two caps ID. That appears several times in this function's documentation.

Also, this issue is about "assertFieldById and friends". So it seems like it would be good to add this to the documentation of:
- assertFieldByXPath
- assertFieldByName

And the assertNoField* methods documentation should also be updated... that's a bit more complicated. It looks like what happens on those methods is that if you don't provide a value, you are asserting that the field itself doesn't exist, and if you do provide a value, you are asserting either that the field doesn't exist or that the value doesn't match.

no problem, i'll take a stab at those pieces as well...

This just covers the assertField* functions and id cleanup. I'll follow this with another patch that includes the assertNoField* functions tomorrow.

Hm. We regularly use "nid" and "uid" in docs (sigh) without the caps. I don't really think we should suddenly be changing them to "NID" and "UID", because they're like that all over the place, and the database field names are lower-case, as are the object property names.

I really just want "ID" to always be capitalized.

And in places like this:

    *   The following defaults are provided:
    *   - label: Random string.
-   *   - id: Random string.
+   *   - ID: Random string.

This is giving defaults for keys/values in an array. I do not think the key name is changing to "ID", so this also shouldn't be changed. Similarly:

    *   Name of field or message property to assert. Examples: subject, body,
-   *   id, ...
+   *   ID, ...

Ok, will do.

Full patch covering the updated @param $value documentation for assertField* and assertNoField* functions, as well as the requested id capitalisation cleanup.

ToDo: Needs backport to D7

Good work!

So... One thing I thought was a bit confusing. In assertNoFieldByXPath, it says:

+   *   (optional) Value of the field to assert. Asserts that the provided value
+   *   doesn't match, or that the field itself doesn't exist. If there is no
+   *   value provided, this is also asserting that the field doesn't exist.

My read of the code says that if you provide a value, you are asserting that either the field doesn't exist or the value isn't found (which is what your text says, good). But if value isn't provide, you are asserting just that the field doesn't exist, and your text says "is also asserting" -- should say "instead", right?

Then the same thing applies for assertNoFieldByName and assertNoFieldById.

Then one other thing. The ByName and ById functions, for some reason ??? have defaults of $value = '' instead of $value = NULL. This value is passed directly into the ByXpath functions, bypassing the default of NULL that is in those functions. So I think we need to make that distinction clear in the docs. In the ByXpath functions, if you just omit $value, you will not be checking the field value. But in the other functions, if you omit $value you will be checking that the field value is explicitly ''. If I'm reading the code correctly, that is? Check my logic...

Anyway, I think the "if there is no value provided" wording may need to be changed to "if you pass in NULL". Please check on that.

The rest (the "ID" fixes) looks great, thanks!

I agree with your logic conclusion and I've got to admit, I found the '' and NULL discrepancy confusing. Note also that assertNoFieldByName() has a $value = '' and assertFieldByName() has a $value = NULL.

I think I've got it right now. Thanks for your help @jhodgdon.

OK... I think most of this is correct now... but I'm not sure it is quite as clear as it could be? For instance:

+   *   (optional) Value of the field to assert. You may pass in NULL to skip
+   *   checking the actual value, while still checking that the field exists.

This seems very clear to me, and correct.

+   *   (optional) Value of the field to assert. Asserts that the provided value
+   *   doesn't match, or that the field itself doesn't exist. Providing a NULL
+   *   value or not providing a value at all, asserts that the field doesn't
+   *   exist.

This doesn't seem as clear. Would it be better to say something like "A field value to check: the assertion will pass either if the field doesn't exist, or if it exists but the field's value doesn't match the provided value. You may pass in NULL (default) to skip the value check; in this case, the assertion will only pass if the field doesn't exist." ?

I also think that some of the "No" methods got the documentation a bit backwards, such as:

+   *   Value of the field to assert. Asserts that the provided value doesn't
+   *   match, or that the field itself doesn't exist. Providing a NULL value
+   *   asserts that the field doesn't exist. Note that if no value is provided,
+   *   this asserts that the field value is ''.

because in this case, it's asserting that the field value is *not* '', right?

Updating the patch with the changes mentioned in #20. Please review.

Thanks! Now it all looks good to me. Some of the wrapping could be a little closer to 80 characters, and these functions are weird about the '' vs. NULL (we should probably file a separate issue on that), but I think the docs are clear and match the current function behavior. Good work!

Hm... I applied the patch and looked over these docs one more time to make sure.

I think we should do one more pass:

a) We should fix the wrapping in assertNoFieldByXPath (as close to 80 characters as possible)

b) assertFieldById should have the note about '' added to it.

c) In assertFieldByXPath (and other functions where NULL is the default), we should say "... pass in NULL (default)... " instead of just "pass in NULL" to make that clear. This is only done on some of the functions, not all.

d) Documentation in a @param statement should only pertain to that parameter. Documentation that pertains to the function as a whole should go outside the @param. So ... There is wording in assertFieldByXPath that is good:

   * @param $value
   *   (optional) Value of the field to assert. You may pass in NULL to skip
   *   checking the actual value, while still checking that the field exists.

But the way it is worded in assertNoFieldByXPath is bleeding into talking about the behavior of the function as a whole... The part about "the assertion will pass either if the field doesn't exist, or if it exists but the field's value doesn't match the provided value" is documenting the behavior of the method, and should be outside the @param. Then the part about "you may pass in NULL" can go in the @param, but it should probably be worded more like the assertField method, something like "You may pass in NULL to skip checking the actual value, and therefore only checking that the field doesn't exist." Same applies to the other assertNoField* methods.

e) And really... Maybe we should just suggest that people use assertField and assertNoField to check the existence without checking the value?

Done.

retry.

Thanks for the new patch! This is mostly great. A few small things to fix:

a) Our documentation standards state that every documentation block must start with a one-line, up-to-80-character summary. Additional documentation should be in following paragraphs.

So, in assertNoFieldByXPath:

   /**
-   * Asserts that a field does not exist in the current page by the given XPath.
+   * Asserts that a field does not exist in the current page by the given XPath,
+   * or if it exists but the field's value doesn't match the provided value.

This needs to be split into two paragraphs or shortened. Maybe:

Asserts that a field doesn't exist or its value doesn't match, by XPath."

would be good?

b) And then in the @param for the same method:

  * @param $value
   *   (optional) Value of the field to assert. You may pass in NULL (default)
   *   to skip checking the actual value, while still checking that the field
   *   doesn't exist.

I think this is still confusing... Maybe the first sentence could be changed to: "(optional) Value for the field, to assert that the field's value on the page doesn't match it." ? Or maybe you can think of a better wording?

c) assertFieldByName

    * @param $value
-   *   Value of the field to assert.
+   *   Value of the field to assert. You may pass in NULL (default) to skip
+   *   checking the actual value, while still checking that the field exists.

This parameter should be marked (optional).

d) Same as (c) in: assertFieldByID

e) assertNoFieldByName

   * @param $value
   *   (optional) Value of the field to assert. Providing a NULL value asserts
   *   that the field doesn't exist. Note that if no value is provided, this
   *   asserts that the field value is not ''.
 

This is OK, but I think it could be even clearer if it said:

(optional) Value for the field, to assert that the field's value on the page doesn't match it. You may pass in NULL to skip checking the value, while still checking that the field doesn't exist. However, the default value ('') asserts that the field value is not an empty string.

f) I think we should do the same update as (e) for assertNoFieldByID.

g) assertFieldByID:

   * @param $value
   *   Value of the field to assert. You may pass in NULL to skip checking the
   *   actual value, while still checking that the field exists. Note that if no
   *   value is provided, this asserts that the field value is not ''.
 

That "not" on the last line is wrong, oops! Also I think we can rewrite this as:

(optional) Value for the field to assert. You may pass in NULL to skip checking the value, while still checking that the field exists. However, the default value ('') asserts that the field value is an empty string.

Updated the patch as mentioned in #27.
Please review now.

+++ b/core/modules/simpletest/src/WebTestBase.php
@@ -3222,15 +3222,13 @@
-   *   (optional) Value of the field to assert. You may pass in NULL (default)
-   *   to skip checking the actual value, while still checking that the field
-   *   doesn't exist.

The note should probably go back in.

+++ b/core/modules/simpletest/src/WebTestBase.php
@@ -3358,9 +3358,10 @@
+   *   (optional) Value for the field, to assert that the field's value on the ¶
+   *   page doesn't match it. You may pass in NULL to skip checking the value, ¶
+   *   while still checking that the field doesn't exist. However, the default ¶
+   *   value ('') asserts that the field value is not an empty string.

should remove the blank space at the end of these lines.

updated as #29. Please review.

The new patch is missing -- uploaded the interdiff twice by mistake?

I am so fool ...ah
Upload the patch. Please review now.

Thanks!

There are still several of the assertNoField methods that didn't get the updated wording. Can you please update them too? See #27 for details. Not all of the changes I suggested there actually got made.

Updated the patch. Please review now.

This looks good! Thanks everyone! Committed to 8.x.

Now we need to backport this to 7.x, as much as possible.

Added D7 patch. Check it.

Looks good, thanks!

The only problem is that this patch is missing fixes for two methods:
- assertNoFieldByXPath
- AssertNoFieldByName

Updated the patch.
Please review now.

Thanks!

But the new wording for assertNoFieldByXPath does not match the wording we've put on the other "No" methods, such as:

+   *   (optional) Value for the field, to assert that the field's value on the
+   *   page doesn't match it. You may pass in NULL to skip checking the
+   *   value, while still checking that the field doesn't exist.

I think this wording is clearer than what is in the latest patch?

Updated patch. Please review now.

42: d7-2039449-42.patch queued for re-testing.

some bot glitch.

Thanks, this looks good now!

Thanks again everyone! Committed to 7.x.