Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
Not failing JavaScript test for JavaScript errors is deprecated in drupal:9.3.0 and is removed from drupal:10.0.0. See https://www.drupal.org/node/3221100
Steps to reproduce
Proposed resolution
Fail a JavaScript test when a JS error is encountered.
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Tests will now fail on JavaScript errors. Previously, any JavaScript error that occurs that did not interfere with the functionality of the test was ignored. Now those errors are surfaced to the test runner. See the change record for details.
Comment | File | Size | Author |
---|---|---|---|
#17 | interdiff_16-17.txt | 526 bytes | pooja saraah |
#17 | 3293090-17.patch | 3.66 KB | pooja saraah |
| |||
#16 | interdiff.3293090.11-16.txt | 2.43 KB | longwave |
#16 | 3293090-16.patch | 3.63 KB | longwave |
#15 | interdiff.3293090.11-15.txt | 2.25 KB | longwave |
Comments
Comment #2
longwaveSo we can convert the deprecation to a failure, but as the failure happens in
tearDown()
I don't see how to catch and expect this in a test.Comment #4
longwaveAs we are explicitly testing the
tearDown()
method I think it is OK to call that directly from the test. We also then need to clear the error log once we have retrieved it, or the test will fail for real.Comment #5
longwaveComment #6
mondrakeWould it make sense to extract part of the code in tearDown to a protected method of its own, and call that method from tearDown and from the test instead of calling tearDown from within the test (which OK, seems to work here, but who knows in the future)?
Comment #7
longwave#6 also means we can move the reset that is only required for the test into the test itself.
Comment #8
mondrakeNice.
Comment #9
alexpottI had a look at whether we got use the @after annotation but unfortunately that does not work. tearDown is called first.
I think conceptually the
if ($this->failOnJavascriptConsoleErrors) {
should wrap the call to this function in tearDown() - that way if someone does calls this function in a test it will always test the js_testing_log_test.errors regardless of that setting - so you can set to FALSE for the main check but then call this yourself and it'll work as expected.Comment #10
longwave#9 is a good suggestion, thanks. Here's a further modified approach that uses
assertPostConditions()
, which I think is intended for this sort of check, and which also means we don't need to reset at all in the test.Comment #11
longwaveFixing a missed comment.
Comment #12
alexpottMaybe it is time to start using PHPUnit's annotations. We can use @postCondition here.
So in the JavascriptErrorsTest we do:
And then in WebDriverTestBase we remove assertPostConditions and do
Yes this reverts the change in #9 but the usage of annotations is neater and we have less inheritance funkyness.
Also I think the whole flag is kinda pointless if we do it this way. A test could make failOnJavaScriptErrors a no-op and be done.
Comment #13
longwaveWhat controls the order of these postcondition hooks? We need to ensure the clear one runs before the fail one in this case. I see the PR that adds @postCondition but it's not documented at https://phpunit.readthedocs.io/en/9.5/annotations.html :(
Comment #14
longwaveAnswering my own question: it uses ReflectionClass::getMethods(), which according to docs
These are executed in order so @postCondition in the class itself should come first, then the parent class afterwards. (@preCondition is reversed, which also makes sense)
Comment #15
longwaveImplemented #12.
For now I think we should keep the flag as we have documented that that is the way to use this feature, we can consider removing it again later in the 10.x cycle I think?
Comment #16
longwaveWe also don't need @group legacy any more for this test.
Comment #17
pooja saraah CreditAttribution: pooja saraah at Srijan | A Material+ Company for Drupal India Association commentedFixed failed commands on #16
Attached interdiff
Comment #18
Gábor HojtsyAlthough I don't know a lot about JS testing, the code looks very sensible and tests the new behaviour well AFAIS based on @alexpott's suggestion.
Comment #19
Gábor HojtsyComment #21
catchThis looks good to me too including the adaptation of the existing test coverage.
Committed/pushed to 10.1.x and cherry-picked to 10.0.x, thanks!
Comment #23
phenaproximaI think this could use a release note, since it is a breaking change in our testing API.
Comment #24
bbralaComment #25
bbralaWrote a snippet and added to the draft.
Comment #26
phenaproximaLooks good to me!