Problem/Motivation

Some of the current calls to AssertLegacyTrait::assert(No)Escaped() in functional tests still have a message passed in, even if the methods do not take that in. We need to remove the messages from the calls and inline them as comments where appropriate.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mondrake created an issue. See original summary.

sja112’s picture

Assigned: Unassigned » sja112
sja112’s picture

Assigned: sja112 » Unassigned
Status: Active » Needs review
FileSize
14.27 KB
mondrake’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @sja112, this looks good to me.

xjm’s picture

Title: AssertLegacyTrait::assert(No)Escaped() in functional tests still have a message passed in, » AssertLegacyTrait::assert(No)Escaped() in functional tests still have a message passed in
xjm’s picture

I've a question about the debugging information for this change.

Presumably this is a precursor to replacing this method. Does the replacement include a message about markup being escaped? If it's WebAssert::assertEscaped(), then I think the message will be:

The string "%s" was not found anywhere in the HTML response of the current page.'

That means we'll see a lot of The string <blahlablhabl long string > was not found in the page if there are fails. Which is good debugging information. And I guess that the backtrace for an assertion will indicate that it was assert(No)Escaped and so between those two pieces of information the developer can infer that the test is checking for markup escaping. But it is a slight downgrade over the

Escaped 'blahblahblah' found

in the current API.

Thoughts? It looks like responseContains() doesn't allow overriding the message at all, which is a bummer.

I think in the case of this assertion it will be OK for the reasons above, but I wonder if there are cases where we should still be overriding stuff to provide more useful debug messaging in the assertions.

Leaving RTBC because these questions apply more to the followup, and I think the followup will be OK too, but just want to validate the answers to the above.

mondrake’s picture

#6 yes this is a precursor to replace AssertLegacyTrait::assert(No)Escaped() with WebAssert::assert(No)Escaped().

Most of the methods in WebAssert do not have a $message argument, I presume because the general going lately is that those messages are not necessary under PHPUnit world order. If we want to go back on that, then I think we should have an issue to do so for all the relevant methods in WebAssert. Note that WebAssert is an extension of mink's WebAssert, whose methods also do not allow passing in a $message.

longwave’s picture

While the messages are usually not necessary, as in other issues we have determined that the additional context is sometimes useful. It looks like in this particular case we aren't losing any useful contextual information but that might not be the case in other similar issues (even if that context data isn't even printed at the moment, because the $message argument hasn't been used since the Simpletest days).

Also note that in #3133355: Introduce PHPUnit-compliant WebAssert::responseHeaderExists() method, and its negative we are introducing a new method to WebAssert that *does* take a message, because we are doing it in a more PHPUnit-compliant way than a Mink-compliant way. Whether this is the way we want to be going for all methods is up for debate, I suppose.

xjm’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs followup

Thanks @longwave and @mondrake. I'm comfortable going forward here in light of #6 - #8. In this specific case, I think there's sufficient debugging info in the default message for the replacement method + backtrace, so in that context we can go ahead with stripping the messages here even without a followup specifically to enhance the default message.

I do worry about people assuming that a message parameter works "because it always worked" but that not being the case -- this is the third issue I've seen come through the RTBC queue about this already. The DX of some assertions allowing one but others not might be a bit lumpy, especially since PHP will happily allow you to add as many arguments as you want that aren't in the signature without telling you they don't do anything. (So devs don't get warning that their "message" will never see the light of day.) Maybe this is something we could follow up about on one of the related metas.

I read over all the specific changes again and I think there's just one more place we should add a comment:

  1. +++ b/core/modules/system/tests/src/Functional/Batch/ProcessingTest.php
    @@ -54,14 +54,14 @@ public function testBatchForm() {
    -    $this->assertNoEscaped('<', 'No escaped markup is present.');
    +    $this->assertNoEscaped('<');
    

    For these we're using < as a test for "Any escaped markup" because you can't have markup without opening a single tag. However, the message will only say there's no &lt; or whatever. Maybe we should add a comment before the first one that says:

    If there is any escaped markup it will include at least an escaped '<' character, so assert on each page that there is no escaped '<' as a way of verifying that no markup is incorrectly escaped.

    We don't need to repeat that each time, just once above the first one I think, so that someone debugging this test will see that and understand why we keep making assertions about &lt;. That will also help us avoid someone "helpfully" removing the assertions in the future because they don't see the relevance.

  2. +++ b/core/modules/system/tests/src/Functional/Menu/MenuRouterTest.php
    @@ -68,7 +68,7 @@ protected function doTestHookMenuIntegration() {
    -    $this->assertEscaped("<script>alert('Welcome to the jungle!')</script>", ENT_QUOTES, 'UTF-8');
    +    $this->assertEscaped("<script>alert('Welcome to the jungle!')</script>");
    

    I think we should change this to "Welcome to jungle!" :) But sadly that's out of scope.

Thanks!

sja112’s picture

Assigned: Unassigned » sja112
sja112’s picture

Assigned: sja112 » Unassigned
Status: Needs work » Needs review
FileSize
14.49 KB
978 bytes

Addressing #9.1

sja112’s picture

Issue tags: -Needs followup
daffie’s picture

Status: Needs review » Reviewed & tested by the community

After applying the patch I could not find any occurrences of $this->assertEscaped( or $this->assertNoEscaped( where there was more then one parameter used when calling the methods.
All the points of @xjm are addressed.
The requested followup is created.
All code changes look good to me.
For me it is RTBC.

alexpott’s picture

Version: 9.1.x-dev » 8.9.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 073679e731 to 9.1.x and ec23e1c885 to 9.0.x and e12e1ce85c to 8.9.x. Thanks!

Backported to 8.9.x as this is test-only and it helps to keep the tests aligned.

  • alexpott committed 073679e on 9.1.x
    Issue #3142752 by sja112, mondrake, xjm, longwave: AssertLegacyTrait::...

  • alexpott committed ec23e1c on 9.0.x
    Issue #3142752 by sja112, mondrake, xjm, longwave: AssertLegacyTrait::...

  • alexpott committed e12e1ce on 8.9.x
    Issue #3142752 by sja112, mondrake, xjm, longwave: AssertLegacyTrait::...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.