Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#11 | interdiff-3142752-3-11.txt | 978 bytes | sja112 |
#11 | 3142752-11.patch | 14.49 KB | sja112 |
Comments
Comment #2
sja112 CreditAttribution: sja112 at Srijan | A Material+ Company for Srijan | A Material+ Company, Drupal India Association commentedComment #3
sja112 CreditAttribution: sja112 at Srijan | A Material+ Company for Srijan | A Material+ Company, Drupal India Association commentedComment #4
mondrakeThanks @sja112, this looks good to me.
Comment #5
xjmComment #6
xjmI'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:
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 wasassert(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 thein 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.
Comment #7
mondrake#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.
Comment #8
longwaveWhile 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.
Comment #9
xjmThanks @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:
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<
or whatever. Maybe we should add a comment before the first one that says: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
<
. That will also help us avoid someone "helpfully" removing the assertions in the future because they don't see the relevance.I think we should change this to "Welcome to jungle!" :) But sadly that's out of scope.
Thanks!
Comment #10
sja112 CreditAttribution: sja112 at Srijan | A Material+ Company for Srijan | A Material+ Company, Drupal India Association commentedComment #11
sja112 CreditAttribution: sja112 at Srijan | A Material+ Company for Srijan | A Material+ Company, Drupal India Association commentedAddressing #9.1
Comment #12
sja112 CreditAttribution: sja112 at Srijan | A Material+ Company for Srijan | A Material+ Company, Drupal India Association commentedFiled #3143398: WebAssert methods do not have a $message argument like AssertLegacyTrait methods
Comment #13
sja112 CreditAttribution: sja112 at Srijan | A Material+ Company for Srijan | A Material+ Company, Drupal India Association commentedComment #14
daffie CreditAttribution: daffie commentedAfter 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.
Comment #15
alexpottCommitted 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.