Problem/Motivation
In preparation for #2775653: JavascriptTests with webDriver we need to ensure that we have no more calls to statusCodeEquals()
and responseHeaderEquals()
.
Given that
I think we should do a hard break on statusCodeEquals and throw an exception JTB is experimental and supporting multiple versions is way more expensive. We could change drupalLogin to not use it but assert something else or just remove it since if the form fails it fails.
Proposed resolution
Add the exceptions
Remove usage in core of all response code related methods. Let the exception throwing be done by the drivers.
Remaining tasks
None.
User interface changes
API changes
No changes.
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#82 | 2827014-82.patch | 7.81 KB | Lendude |
#82 | interdiff-2827014-71-82.txt | 3.29 KB | Lendude |
#71 | 2827014-71.patch | 4.02 KB | michielnugter |
#71 | interdiff-65-71.txt | 2.21 KB | michielnugter |
Comments
Comment #2
dawehnerComment #4
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedJust to get the ball rolling here.
- Removed the status check from SessionTest. It will fail anyway on the next click if it's an error page.
- Reimplemented login/logout and conform them to the JTB way of testing.
- statusCodeEquals() and statusCodeNotEquals() now throw exceptions. Not sure yet on the doxygen and the type of Exception.
Comment #5
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedSorry, garbage of a previous patch still in this one.. New patch.
Comment #6
dawehnerIt would be kinda nice to be able to share most of the code with the parent class somehow.
One possibility could be to simply not do anything in the assertion.
Comment #7
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedThat's true but that would make it impossible to do a hard break on these methods as @alexpott suggested. It would also mean we have methods that imply a test that is not actually performed.
I don't see any way around this..
Comment #9
dawehnerYeah agreed. We also better break earlier than later!
Comment #10
cilefen CreditAttribution: cilefen commentedThank you both for working on this. I would rather have an exception than think a test passed when it did not! This issue is not specified enough for a novice to begin with, so I removed those tags.
I have two nitpicks: Lines after docblock tags are indented two spaces and docblock summaries must be less than 80 characters.
I am not sure about the "immediately" part of the deprecation—we do not use that term in any other deprecation comments. Also, we usually write "removed before". Why is the deprecation targeted to "8.8.x"? Is this is part of the test system roadmap? The issue summary does not explain and there is no change record.
In terms of developer experience, "check the DOM" isn't the most helpful tip as a replacement.
The patch does not follow its own advice of checking the DOM and simply removes a call in testSessionExpiration, which I find odd.Let's get a change record drafted.
Comment #11
cilefen CreditAttribution: cilefen commentedIf that's true, ignore that part of the review above.
Comment #12
xjmThanks @cilefen. The deprecation message should follow our normal pattern. We don't remove APIs in minors, so it should say "before 9.0.0" like other things.
Comment #13
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commented@cilefen:
8.8.x was a typo, my bad! I meant 8.4.x. The alternative to this will be different for each situation but I'll try to write a more helpful comment.
@xjm:
Because an exception is thrown it's actually an immediate bc-break as tests would fail if they still use these methods. It's why I added a comment on the immediate deprecation instead of the normal deprecation comment.
If we were to switch drivers (#2775653, it's why this issue was opened) a break is needed as Webdriver doesn't support fetching the response code.
Comment #14
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedFixed the versions in the deprecated message and conformed more to the standards. I tried to add a better suggestion for the alternative.
Would like feedback on the versions, this is my interpretation for what it should be based on the fact 8.3 is in alpha already.
Comment #15
dawehnerWell, you cannot remove it in the future, as the base class contains the implementation already, right?
Comment #16
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedJust to be sure I understand correctly: Do you mean we cannot remove this untill 9.x? If that's the case we will have to wait with the driver change for JTB tests if we want these methods to keep working as they currently do right?
Comment #17
dawehnerWell, how do you want to remove that. There is a class hierarchy involved.
Comment #18
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedWow, ok, not a proud moment for me. To be clear, I actually do know about object oriented programming ;)
So ok, maybe we can commit a deprecation warning on 8.4.x and call the parent method (keep it working). In 8.5.x we can then add the exception and break that way.
Is that workable? Should I open a separate issue for the deprecation message and upgrade the version on this issue to 8.5.x?
Other question: can we still add the deprecation to 8.3.x or is it too late for that?
Comment #19
cilefen CreditAttribution: cilefen commentedAs for the 8.3 question, here are the rules: beta phase is now, https://www.drupal.org/core/d8-allowed-changes#beta, RC begins soon https://www.drupal.org/core/d8-allowed-changes#rc
Comment #20
xjmCoding standards are all off here. Also, I think a specific typed exception would be better than
\Exception
.Comment #21
xjmAlso, I don't see the point of adding methods for one minor release; that's almost more disruptive. So I would not take that approach. Thanks for working on this!
Comment #22
cilefen CreditAttribution: cilefen commentedI do not understand what can be deprecated. An exception should be fine. Right?
Comment #23
xjmAlso, while the test API is a public API, it's also critically important for it to be easily debugged and fail explicitly. So we have a bit more leeway for new hard breaks/fails/new exceptions than we would for production APIs. And it's definitely allowed to make make failures more explicit in a minor release. Passing tests that should not pass are bad.
Comment #24
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedWe need to add the methods because we don't want to disable them for normal BrowerTestBase tests, only for JavascriptTestBase tests.
I implemented a deprecated warning because contribs might still be using these methods. I figured giving the contribs 1 minor to update their tests before throwing exceptions is the correct way to do it.
But maybe I'm thinking too complicated and we should just do als @cilefen suggested add the exception and don't mention anything about deprecation?
Comment #25
cilefen CreditAttribution: cilefen commentedOn #2775653: JavascriptTests with webDriver, klausi recommended an UnsupportedDriverActionException. It seems klausi, dawehner, and alexpott reached consensus on throwing an exception so just do that. As for the reimplementation of drupalLogin, alexpott wrote:
By "remove it", alexpott is referring to removing the call to statusCodeEquals. Therefore that would be easier than reimplementing drupalLogin/drupalLogout.
I just noticed the issue summary refers to responseHeaderEquals but that is not in the patch.
Comment #26
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedOk, now it see it, thanks for clearing it up and sorry it took me this long to get it.
I updated the patch with the following changes:
- Removed drupalLogin/drupalLogout from JavascriptTestBase
- Removed the use of the statusCode equals calls from WebTestBase
- Changed Exceptions to UnsupportedDriverActionException, makes a lot of sense.
- Throws exceptions on responseHeaderEquals and Not variant.
- Removed deprecation warnings.
On the last one I'm not 100% sure. It's nice to have your IDE point out that the use of a method is not recommeded, the @deprecated tag adds a strike-through, very handy feature. Is there any other way for this?
Comment #27
cilefen CreditAttribution: cilefen commentedThere is #2856744: [META] Add trigger_error(..., E_USER_DEPRECATED) to deprecated code. But "deprecated" implies "will be removed". That is not the case here.
Comment #28
dawehnerGreat idea to remove the asssertions from
drupalLogin
. We test that thousand times, so we will see when this fails anyway.Maybe using
@internal
communicates a similar message?Comment #29
cilefen CreditAttribution: cilefen commentedSemantically, @internal makes it seem as if the functions are useful to core itself. Hmm...
Comment #30
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedI added an @interal tag and does add the strike-through but to make phpStorm understand it, the phpdoc for assertSession() should be updated and define the actual @return type instead of {@inheritdoc}.
I think we should add the @internal. Maybe open a new issue to fix the phpdoc for assertSession?
Comment #31
cilefen CreditAttribution: cilefen commented"@internal" means "you had better know what you are doing calling this, because we may change it any time". That does not seem to apply here, which is "do not call this".
Comment #32
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedVery true, that leaves us with not adding anything. I think it's acceptable because the exception will be thrown in a test and it will be caught while in development.
Do we have to add a hint for an alternative in the phpdoc or is it ok as it is?
Comment #33
cilefen CreditAttribution: cilefen commentedYou can explain that in a change record. Start a draft.
Comment #34
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedRemoved the @internal tag and created a draft CR:
https://www.drupal.org/node/2857562
Comment #35
cilefen CreditAttribution: cilefen commentedGood! There is a field on the CR to affiliate it back to this issue, which you should do. Add something like "these will now throw exceptions if called from..." and provide example before and after usage.
Comment #36
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedI tried the link the first time but screwed the input up, fixed it.
I updated the text to mention the exception and added an example.
Comment #37
cilefen CreditAttribution: cilefen commented$this->statusCodeNotEquals(3);
Looks like some debug code is there. Also be sure to scan the files with phpcs and Drupal's coding standard.
Comment #38
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedRemoved code and updated the thrown exceptions. I have phpcs enabled (again). I fixed the line lengths for the phpdoc. I didn't fix the documentation for the params, these weren't available on the original methods in Mink and it seems like wasted effort to document them now.
Comment #39
cilefen CreditAttribution: cilefen commentedresponseHeaderNotEquals has a wrong exception message. The param docs must meet coding standards or we can't commit.
Comment #40
dawehnerComment #41
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedFixed the exception message and added param descriptions, was that what was wrong with the phpdoc?
Comment #42
droplet CreditAttribution: droplet commentedI don't know correct or not in Drupal, which may only need to drop
{@inheritdoc}
for docs.Comment #43
cilefen CreditAttribution: cilefen commentedI must check that but I think the docs generator supports @inheritdoc only if it is alone.
Comment #44
cilefen CreditAttribution: cilefen commentedYes, you cannot extend a parent method's comment with {@inheritdoc} in Drupal.
Comment #45
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedBut when we use {@inheritdoc} we cannot indicate in the phpdoc that something is going on in this method. I'm in favor of overwriting the phpdoc as it does right now.
Comment #46
droplet CreditAttribution: droplet commentedComment #47
dawehnerI think having some kind of documentation on the method is a good idea.
Comment #48
alexpottCommitted f1b5e67 and pushed to 8.4.x. Thanks!
I've credited @cilefen, @dawehner and @xjm for all making review comments that influenced the direction of the patch. @droplet thank you for your reviews. @michielnugter thank you for the patch.
Comment #50
droplet CreditAttribution: droplet commentedGreat!
It should credit me, haha. the basic patch come from mine: https://www.drupal.org/node/2775653#comment-11646973
** Just a joke, don't modify the GIT to create extra noise :p
Thanks ALL!
Comment #52
alexpottI've reverted this change because it is broken. Revealed by https://www.drupal.org/pift-ci-job/663558
The constructor for this exception is:
These all should have $this passed in.
Comment #53
alexpottSo @droplet you'll be credited when this gets in :)
Comment #54
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedRight, bad one to miss, sorry!
All fixed now!
Comment #55
dawehnerI wondered for a second whether there is any value in some sort of unit test, but I don't really believe in it.
Comment #57
dawehnerComment #58
alexpottPostponing this on #2831274: Bring Media entity module to core as Media module since that patch is very nearly and will be simple to fix in this issue once that is in.
Comment #59
marcoscanoBack to RTBC, once #2831274: Bring Media entity module to core as Media module is in.
Comment #61
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedIt needed to drop the status checks in the media module.
I checked each status check and they were all followed with assertions that only pass if the status code was as expected so no actual coverage is lost.
Comment #62
droplet CreditAttribution: droplet commentedThanks @michielnugter.
Comment #63
Wim Leerslgtm
Fixed IS.
Comment #65
marcoscanoThis probably makes #2878112: Fix Media JavaScript tests to account for changes in JavascriptTestBase obsolete.
Comment #66
phenaproxima@marcoscano's changes look legit to me. Back to RTBC we go.
Tagging this as part of the Media initiative since it touches a Media test.
Comment #67
naveenvalechaComment #68
larowlanSorry for being daft, but can someone explain why we need to embed knowledge of the driver into our web assert handler.
Mink is meant to be driver agnostic.
The driver is responsible for telling us what it does and doesn't support.
For example we don't have knowledge in BTB that you can't drag and drop or take screenshots.
If you were to call this in BTB
The CoreDriver would throw the Exception because by default it is not supported - each driver then implements as appropriate.
The same should apply here.
CoreDriver throws this exception on our behalf for statusCodeEquals (the code of which should end up calling \Behat\Mink\Driver\DriverInterface::getStatusCode).
So I understand why we're removing the asserts, because depending on the driver, they may fail.
But I don't understand why we need to embed knowledge of a driver's capabilities into JsWebAssert.
We can avoid the API break altogether if we just defer this to the driver (as per the Mink design).
Am I missing something obvious or are we using Mink wrong in so far as the driver doesn't get a chance?
In summary: I think we only need to remove the assert calls if we want *core* to support both - but I don't think we should be including knowledge of a particular driver in the JsWebAssert class.
Comment #69
larowlantl;dr I suspect that anyone using those methods in contrib will get this exception already if they are using that method and they switch drivers - i.e. we're just duplicating something that Mink already handles
Comment #70
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commented@larowlan
You're totally right. If you'd switch driver you will get the error anyway so implementing it for ourselves is incorrect. Setting to needs work, can't reroll the patch right now without the status methods, might have time later today.
Comment #71
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedAnd the patch with diff.
Comment #72
Lendudeshould we do something with
\Drupal\FunctionalTests\AssertLegacyTrait::assertResponse
? Maybe update the docblock to indicate that this probably won't work?Comment #73
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedGood catch. I searched the codebase and it's currently not used in FunctionalJavascript tests.
With the same logic as for the statusCode methods I think it's safe and good to let it be as is. Don't build expected driver logic into the general codebase.
Comment #74
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedUpdated IS to reflect the changed solution.
Do we still need a change record if we don't break the API?
Comment #75
dawehnerI'm a bit confused. Wasn't the point of adding these methods to inform users that things are deprecated, when they use JavascriptTestBase, and before we switch the driver?
Comment #76
Lendudehmm so not throw an exception but just implement the methods to add @deprecated to them and then call the parent, and still leave it to the driver to throw the exception. Something like that?
Comment #77
dawehnerWell, its more than deprecated IMHO. We really don't want to support it for longer. Deprecations would last until 9.x, but as you know, we want to use a webdriver based driver before that.
Comment #78
michielnugter CreditAttribution: michielnugter commentedI pinged @larowlan on IRC to ask if he could give his opinion on this now that it's clear again why we did the exceptions.
Comment #79
michielnugter CreditAttribution: michielnugter commentedMy 2 cents:
+1 on not deprecating then as we want to get rid of it earlier than 9.x as we have to switch off phantomJS some time.
Exception mean a hard, clear and immediate break. As we can't keep supporting them I'm in favor of this. As the driver will eventually take care of the exception I am in favor of doing a follow-up on the webdriver issue to remove the methods with exceptions again. Is that a workable solution?
Comment #80
larowlanlog of irc conversation from this morning
Comment #81
catchI'm not sure I can see exactly what #80 looks like in terms of inheritance, but the concept seems fine and better than just throwing the exception.
Comment #82
LendudeSo this is my interpretation of our IRC discussion.
Comment #83
larowlanThis looks great - thanks @Lendude
Comment #84
dawehnerThis looks perfect for me as well. I adapted the change record to be in sync with our decision.
Comment #86
larowlanUpdating issue credits, adding myself for reviews and Wim Leers for issue summary updates.
Comment #89
larowlanCommitted as 6496b03 and pushed to 8.5.x.
Cherry-picked as b1657f0 and pushed to 8.4.x.
Published the change record.
Comment #90
dawehnerYeah, thank you @larowlan!
Comment #92
dawehnerI just realized that we forgot some of the needed deprecations, see #2928522: \Drupal\FunctionalJavascriptTests\WebDriverWebAssert misses some deprecations