Problem/Motivation
While working on #3083275: [meta] Update tests that rely on Classy to not rely on it anymore it has become clear that there are many assertions in Functionl/Functional JS tests related to status messages, and they are written many different ways. Most of them depend on Classy being the theme. For example:
$this->assertSession()->elementNotExists('xpath', '//div[contains(@class, "messages--error")]');
$xpath_err = '//div[contains(@class, "error")]';
$this->assertNotEmpty($this->xpath($xpath_err), 'Enabling translation only for entity bundles generates a form error.');
$this->assertSession()->elementNotExists('xpath', '//div[contains(@class, "messages--status")]');
$this->assertSession()
->elementContains('css', '.messages--warning', node_get_type_label($node) . ' content items were skipped as they are under moderation and may not be directly published.');
And a couple examples from Functional JS tests where we wait for messages.
$assert_session->assertNoElementAfterWait('css', '.messages--warning');
// Check that the 'content has been updated' message status appears to confirm we left the editor.
$assert_session->waitForElementVisible('css', 'messages messages--status');
Proposed resolution
Make assertions of Status Messages more consistent and manageable by introducing an AssertStatusMessageTrait
that can be used in Functional/Function JS tests.
The methods should be:
- assertStatusMessageExists
- assertStatusMessageNotExists
- assertStatusMessageVisibleAfterWait
- assertNoStatusMessageAfterWait
each accepting optional $type and $message parameters.
/**
* Whatever this method does.
*
* @param string|null $type
* The optional message type: status, error, or warning.
* @param string|null $message
* The optional message or partial message to assert.
*/
After getting the Trait into the codebase, the Trait can then be used to make #3083275: [meta] Update tests that rely on Classy to not rely on it anymore much smoother.
Another possibility, brought up in #28 is to add methods to WebAssert/JsWebAssert instead of making new traits.
Remaining tasks
Etc
User interface changes
None
API changes
New trait for tests, but essentially none
Data model changes
None
Release notes snippet
There's a new trait? Use it?
Comment | File | Size | Author |
---|---|---|---|
#51 | interdiff_38-51.txt | 9.85 KB | danflanagan8 |
#51 | 3268932-51.patch | 26.07 KB | danflanagan8 |
#38 | interdiff_36-38.txt | 960 bytes | danflanagan8 |
#38 | 3268932-38.patch | 24.75 KB | danflanagan8 |
Comments
Comment #2
danflanagan8Here's a patch with the trait and another patch that modifies some existing tests to show how the trait works and kind of prove that it works.
Comment #3
danflanagan8Comment #4
danflanagan8Let's try that again with a different typo. The typo is supposed to show the helpful exception.
Comment #6
danflanagan8The patch in #2 is ready for review.
That fail in the demonstration was intentional, by the way:
This happened when I called
$this->assertStatusMessageExists('not a valid status', 'Translation is not supported if language is always one of: Not specified, Not applicable');
I wanted to demonstrate that exception, which makes it harder for a typo to lead to a false negative. Although that would be more of an issue if asserting the message does NOT exist.
Comment #7
xjmGreat idea!
I'd suggest potentially splitting it into two traits, one that contains the BTB assertions, and a second trait that uses the first with the
waitForElementVisible()
calls (since those methods will not exist for non-BTB tests). MaybeAssertStatusMessageTrait
andAssertJavaScriptStatusMessageTrait
.All these methods should have parameter and return typehints. Looks like the parameter typehints are generally
?string
and all but the last method havevoid
return.Is there a particular reason to allow both NULL and empty string?
Comment #8
xjmI wonder if we should also switch the order of the arguments. It seems like it would be less common to leave out asserting a specific message than it would be to leave out asserting whether it were a warning, error, etc.
Comment #9
danflanagan8Thanks for the review, @xjm!
I think you're right. I'm not sure what the most common use case will be, but it would probably make sense to have the same order of arguments that
MessengerInterface
uses. FromMessengerInterface
:public function addMessage($message, $type = self::TYPE_STATUS, $repeat = FALSE);
I was thinking it was the other way round. So we should do message first, then type.
+1 to two traits
Not really. And certainly when we switch the order of arguments there's no need to be so overly accommodating.
Setting to NW so I can make those changes.
Comment #10
danflanagan8Here's a patch with the updates suggested by @xjm.
Comment #11
dwwLove this idea! Hope to review this tomorrow...
Comment #12
longwaveCan we either add some explicit test cases that prove that each of these methods works, or convert some existing tests to use these new traits?
Does $message need escaping in some way if it contains double quotes (or maybe other characters)?
Comment #13
danflanagan8That's a good question and makes me think that explicit test coverage would be a good idea. That way we can force some edge cases.
Comment #14
larowlanit's a shame this isn't 10 only because we could use an Enum
its a shame this isn't 10 only because we could use match
Agree we need some escaping on $message (and coverage for it)
Typically when we add these things, we try to include at least one usage of it. Should we do that here too?
Comment #15
danflanagan8Here's a new patch that is really, really different because once I added test coverage I discovered a ton of bugs. As suggested in #13 using
contains
to select was broken by so much as a quote.But then writing tests for the ajax side of things I learned several things about ajax messages which meant I mostly re-wrote the
AssertJavascriptStatusMessageTrait
so that it actually does what it says it does.I'm really starting to think this works. What I'm much less confident about is where all the Traits and tests go. Nobody's complained about that yet, but I'm all ears.
Thanks for all the great feedback so far!
PS.
I have not addressed that yet. I'd be happy to do it though if that's in scope.
Comment #16
danflanagan8I should also point out that I can't get escaping quotes to work when using
:contains
. Usingaddslashes($message)
to try to escape quotes in the message resulted in:I guess
:contains
isn't too robust. Anyway, that's what I found. That's why I went in a different direction with the selector.Comment #18
longwaveYou could try building XPath based selectors instead and using WebAssert::buildXPathQuery() to build them, which will help with escaping.
Comment #19
danflanagan8@longwave, you are right on the money with
WebAssert::buildXPathQuery()
!So let's act like the patch in #15 never existed. It was a step in the wrong direction. Here's a new patch that uses xpath instead of css selectors. I'm putting an interdiff relative to #10 since we have agreed the patch in #15 never existed. ;)
Comment #20
danflanagan8As suggested in #14 here's an update to the patch where each Trait is used to refactor an existing test. Each trait uses its positive and negative assertion, so it's pretty representative.
You'll have to forgive me if
Drupal\Tests\file\FunctionalJavascript\MaximumFileSizeExceededUploadTest
fails. I can't get it to complete on my local with or without my update. Hopefully it works because it certainly reads better with the new trait.Comment #22
danflanagan8Hmmm....most of those fails look random. I want to figure out what's going on with that MaxFileSize test. I'm going to just run that test. ONce again, it's not running at all locally for me.
Comment #24
danflanagan8Hmmmm....this one reverts that test to its original state but then puts
$this->assertNotEmpty()
around a wait. Will this fail? Is that element not showing up? Let's find out!Still running just the one test.
Comment #26
danflanagan8Well, there ya go. Looks like the test's existing message assertion is bogus. I guess this proves the need for this Trait!
Is that not an ajax message? I wonder if the page is reloading and it's actually a normal status message. I don't have the html output though since I can't run the test locally.
I'm setting back to NR because #19 still needs eyes.
And maybe someone has ideas about what's going on in the max file size test.
Comment #27
danflanagan8I've said it before, but now I thin I have it! :)
(The interdiff here is relative to #20. #22 and #24 were experimental patches that ran one test.)
I had re-written that AssertJavascriptStatusMessageTrait as if ajax status messages always used MessageCommand. Turns out they almost NEVER use MessageCommand, and are instead almost always using a
status_messages
element. So I've updated the selector in AssertJavascriptStatusMessageTrait yet again such that:As for
MaximumFileSizeExceededUploadTest
, now the test should pass and it will actually be asserting the presence of a status message. This was an interesting case where I discovered bugs in two things at the same time. But now I think they're both fixed.Comment #28
mondrakeWould it make sense to just add the methods to
WebAssert
?And then call
etc. and avoid proliferation of traits.
I had go over twice before I understood this was not a test method but a controller's callback, can we find a better name?
Comment #29
danflanagan8Here's a patch that goes in @mondrake's suggested direction of adding methods to WebAssert/JsWebAssert instead of making new traits. I have updated the issue title to indicate the possible change in course.
^^ I've updated the language around this as well.
Comment #30
mondrakeSome typehints and method visibility things:
return typehint
: string
return typehint
: void
return typehint
: void
You can useassertNull()
samecan this be
private
?can this be
protected
?Then, a couple of points I would have done differently. They could be seen as personal preferences so I'm not saying it should be done like this, just food for thought.
1)
I'm not a big fan of allowing explicitly passing NULL to have different execution paths. Not anymore in PHP 8.1.
I would suggest a set of methods like
IMHO this way is more clear what we are asserting (in the xpath constructor we use contains, so this is what we are doing). Similar in the JS equivalents
2)
In PHPUnit world, an assertion, i.e. a specific condition checked by the test, 'passes' or 'fails'. If we have an error in the SUT that is not explicitly being checked, we have an 'error'. WebAssert is predominantly throwing exceptions when its assertions fail, but this is wrong IMHO.
For this reason, more recent additions to WebAssert throw a PHPUnit failure instead of an exception when the assertion fails, for example
In the patch here,
converting to PHPunit failure could be achieved by using
assertNull
(see above)converting to PHPunit failure could be achieved by using try...catch
Comment #31
mondrakeops, sorry, we cannot use
assertNull
within WebAssert.We should do
Assert::assertThat($actual, Assert::isNull(), $message);
Comment #32
danflanagan8@mondrake, thanks for the extremely helpful review. That's a lot of stuff I did not know. I believe I have taken all of your suggestions into account in this new patch.
Now the public methods being added to WebAssert are:
And to JsWebAssert we are adding:
In addition to addressing your suggestions, I also added some assertions in the form or try/catch blocks to test that I could make each of these methods fail. That seemed like something that was missing in my attempt to have test coverage for these test methods. Maybe that's overkill. I'm happy to remove it!
Comment #33
danflanagan8Sorry. Here's a patch with cs problems fixed. The diff is still relative to #29. Let's act like #32 never happened!
Comment #34
mondrakeGetting there almost. Why
?string $type = NULL
and not juststring $type = NULL
? That way if you pass something, it should be not-null; if you do not pass anything, it defaults to null. Or in other words, you can't (and don't need to) pass NULL explicitly.Comment #35
danflanagan8I didn't understand that. Thank you for the free course in php8! I'll update that.
Comment #36
danflanagan8I think I have my argument types straightened out now. New patch here.
I also updated the exception message if a bad status message type gets passed. This one no longer suggests passing NULL.
Comment #37
mondrakePHPStan in the D10 branch found an issue,
I'll be bold and set sail for the WebAssert addition solution, drop the trait-based one. We'll need a CR here. When this come back green tested on both 9 and 10 I'll RTBC
Comment #38
danflanagan8Darn it! Here's a fix for that sprintf boo boo. I canceled the test on #36 since I'm going to re-run it here.
Comment #39
mondrakeThe D10 failure seems unrelated. This looks good to me. Do we need a follow up to make use of these methods more broadly?
Thanks @danflanagan8 for following my brain circles :)
Comment #40
danflanagan8I'm the one who should thank you!
Do we need a follow up to make use of these methods more broadly?
I was planning on doing as many changes as possible as part of #3083275: [meta] Update tests that rely on Classy to not rely on it anymore, which has been going module-by-module decoupling tests from Classy. That would cover a lot of ground since many message assertion are relying on Classy markup.
That said, I'm open to other ideas.
Comment #41
danflanagan8I created a change record and added some info and examples. I don't have much experience making change records and would welcome editorial advice!
Comment #42
mondrakeCR done
Comment #43
dwwThanks for all the fabulous work and progress in here!
I started from reviewing the CR. I first spotted what seemed like a simple typo where the 2nd set of examples had the before with an
elementNotExists()
while the after saysstatusMessageExists()
. I started to edit it to fix this myself. But then I looked more closely to the first example:Before:
$this->assertSession()->elementNotExists('css', '.messages--warning');
After:
$this->assertSession()->statusMessageNotExists('warning');
$type is the 2nd arg and $message is the 1st. This example doesn’t work at all. We’re just checking there’s no status message with the message ‘error’.
But now that we can’t explicitly pass NULL as the first arg, I don’t think this API lets you do the kind of check for “there should be no error messages” that this example tries to demonstrate.
I don’t want to undo the suggestions or work from the last round of feedback, but I think this needs more work before it’s RTBC.
Thanks/sorry,
-Derek
Comment #44
dwwSorry, that’s what I get fir trying to review things on my phone at 11:20pm. ;). That method only takes a type. There’s no $message at all. 😅 tee hee. API is probably fine. Maybe just the CR needs a few edits. 😉
I’ll look again after some sleep.
Comment #45
dwwhttps://www.drupal.org/node/3270424/revisions/view/12588385/12589083
CR is now fine. API looks great. I didn't closely review the whole patch, but restoring status.
Thanks/sorry,
-Derek
Comment #46
alexpottassertTrue needs a message to have something useful in test logs.
A wait should have a timeout. So callers can set it to longer or shorter.
I think this is why the $message argument is nullable on buildStatusMessageSelector() - so I think either we should change that or use NULL instead of ''. I think NULL is a more semantic why of saying we don't care about the message. Also in #34 @mondrake is wrong - https://3v4l.org/7saY2 you are still able to pass NULLs into a
string $param = NULL
parameter.string $param = NULL
is the same as?string $param = NULL
.Why are we why wrapping this in a try catch? We do plenty of $this->elementExists - it seems very unnecessary - PHP Unit will mark the test as failed and report the ExpectationException nicely without this additional work. Ah I see - this was recommended in #30. I'm really not sure that what @mondrake is proposing is necessary or even correct. There's no real difference if \PHPUnit\Framework\ExpectationFailedException is thrown or \Behat\Mink\Exception\ExpectationException. And if there is then PHPUnit is becoming an even less welcoming framework to build test suites on top of.
Comment #47
mondrake#46.4 the only difference is that in PHPUnit CLI assertion failures are reported as an 'F', and other exceptions as an 'E'. Actually in DrupalCI a test ending in error ends up being reported twice, once with the PHPUnit CLI output, and once just flagging the fact that the PHPUnit CLI exited with an error.
And, yes, that's my personal preference since
Comment #48
mondrakeFrom https://phpunit.readthedocs.io/en/9.5/textui.html:
and
Comment #49
alexpottOkay well I think that PHPUnit should have a way to register other exceptions as Assertion fails to make interoperability a bit simpler. But sure let's leave the try catches alone. Still need to fix #46.1, .2 and .3
Comment #50
mondrake#49 an option could be to change
WebAssert::assert()
implementationto use PHPUnit's
Assert::fail()
instead of throwing Mink'sExpectationException
. But that might lead to BC ripple effects?Comment #51
danflanagan8Here's a patch with 46.1, 46.2, and 46.3 addressed.
Adding the timeout also allowed me to shorten the waits on the "forced fails" being added in
MessageCommandTest::testJsStatusMessageAssertions
. That's an easy save of 36 seconds!I'll make the necessary updates to the CR too. Thanks!
Comment #52
mondrakeBack to rtbc, the D10 failure is in head too.
Comment #53
mondrakeFiled #3271214: Change BrowserTestBase to flag PHPUnit failures instead of throwing Mink exceptions to explore #50
Comment #54
alexpottCommitted and pushed e5e74fb998 to 10.0.x and 99b6843db1 to 9.4.x. Thanks!
Comment #55
alexpottLet's see if this can be backported to 9.3.x as a test only change.
Comment #58
Wim LeersGiven that
9.4.0-beta1
will be released today, marking this as and moving back to9.4.x
.