As mentioned at #3517299-24: Convert functional tests in help module to kernel tests it'd be helpful to add xpath() and getUrl() helpers that are available to Browser tests to \Drupal\Tests\HttpKernelUiHelperTrait.
This would help with some of the tests considered at #3583587: Convert some functional tests in system module to kernel tests for an immediate example.
However, Kernel tests already have their own version of xpath(), which has a different return type to the Browser test xpath(). Therefore, the xpath() being copied over from Browser tests is being renamed to avoid a clash and to be unambiguous. A follow-on issue #3585774: rename xpath() methods in tests will rename the existing xpath() methods for DX.
Once #3583587: Convert some functional tests in system module to kernel tests is in we need to update DefaultMetatagsTest here to use the xpath function instead of this line:
$result = $this->getSession()->getPage()->findAll('xpath', '//meta');
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #3
dwwMoved the code. But now that I did, I'm unclear why we need / want to override the copy of
xpath()that lives incore/tests/Drupal/KernelTests/AssertContentTrait.phpwhich is already included for us inKernelTestBase. 😅 Could use better comments from @joachim or whoever best understands this detail. Maybe they'll be equivalent once #3582985: Add content from HttpKernelUiHelperTrait::drupalGet() to $this->content for use by AssertContentTrait is done? Should we postpone this on that and then we don't need a separatexpath()here?Comment #4
dwwAlso starting to wonder if all these separate follow-ups are a good idea, or if it'd be easier to manage them all, and hopefully backport them, if we merged all these into a single "make Kernel drupalGet() more useful" issue? 😅
In particular, merging these 3:
(since #3582769: Add clickLink() to HttpKernelUiHelperTrait is already committed - but not yet backported)
Comment #5
dwwComment #6
dwwp.s. If we merge these followups, maybe we should also move the namespace for
HttpKernelUiHelperTraittoDrupal\KernelTests(since it's specific toKerneltests) and perhaps rename the trait to something likeHttpKernelRequestTrait. We had a Slack thread in #core-development about this.Comment #7
dwwSorry, thought I already posted this to this issue. But yeah, the original commit with the alternate
xpath()failed spectacularly with:Comment #8
mstrelan commentedIt's maybe worth noting that
BrowserTestBase::xpathreturns\Behat\Mink\Element\NodeElement[]whereasAssertContentTrait::xpathreturns\SimpleXMLElement[]|false. So even with #3582985: Add content from HttpKernelUiHelperTrait::drupalGet() to $this->content for use by AssertContentTrait we still have to refactor any calls to$this->xpath().Comment #9
joachim commentedYeah, I got stung by the different behaviour of xpath() when I was working on #3582580: add checkForMetaRefresh() method to HttpKernelUiHelperTrait.
Comment #10
joachim commentedI have mixed feelings about this.
On the one hand, we should make conversion to Kernel tests as simple as possible with as few changes as possible -- for the work itself, but also for ease of reviewing, so reviewers can be confident that the test coverage is not changing.
On the other, getUrl() is only needed by the $goto parameter of AssertBreadcrumbTrait::assertBreadcrumb(). I'm not keen at all on an assertion that sneakily makes a drupalGet() call, which changes the state of the test. Requiring an explicit call to drupalGet() first would be better DX - helps readability and prevents bugs.
But that's not something we should change the behaviour of here -- having two flavours of assertBreadcrumb() floating around would be bad.
However, the two flavours of xpath() already exist, and while at the moment they're segregated in Functional / Kernel tests, if we start having both flavours in Kernel tests people's heads are going to explode.
So if we're bringing the Functional one into Kernel tests, I think we have to rename it to something unambiguous like getNodeElementByXpath(), and file a follow-on to rename the two existing xpath() methods in a similar way.
Comment #11
smustgrave commentedWith regards to #4 I’ll lean on you have been in the weeds of this issue. But will say the MR would still be small and manageable and if you need sign off I feel you would have better luck with a single ticket vs many. But think that’s a you guys call
Comment #12
joachim commentedOk, adding the xpath() method back with a distinguishing name.
Filed #3585774: rename xpath() methods in tests as a follow-up to standardize and differentiate the names of the xpath() methods.
Comment #13
mstrelan commentedComment #14
joachim commentedAdded reasons for the rename to IS.
Comment #15
smustgrave commentedper the remaining task section now that #3583587: Convert some functional tests in system module to kernel tests landed can DefaultMetatagsTest be updated. Then probably good to move this one forward I assume?
Comment #16
joachim commentedComment #17
smustgrave commentedAll tasks seem to be addressed. LGTM!
Comment #18
dwwDefaultMetatagsTestas part of this issue. The goal is to backport this (and a handful of others), perhaps all the way back to 10.6.x so that contrib can start converting their Functional to Kernel tests. See #3390193-160: Add a drupalGet() method to KernelTestBase. So this issue should just do what the title says. Conversions of tests to use this helper should happen elsewhere. Therefore, I moved @mstrelan's work over to #3588181: [pp-1] Convert DefaultMetatagsTest as Kernel test to use getNodeElementsByXpath().xpath()) got lost when that commit was reverted. Pushed another commit to restore it.That said, very happy to see
getNodeElementsByXpath()here with its new name. I think that's excellent, and totally self-documenting (even if it's a little verbose).Leaving RTBC, since these are very trivial changes I'm making since the last review.
Thanks, everyone!
-Derek
Comment #19
smustgrave commentedup to you guys as you've all been doing all the work. Only reason I liked the one conversion was it shows this function being used. But perfectly fine doing in another ticket fwiw
Comment #20
mstrelan commentedIt was asked to be done here when the system module issue was committed. To me it doesn't seem worth having its own issue, and it also wouldn't necessarily need to be backported, but not sure why it couldn't be.
Comment #21
joachim commented@dww what would you prefer as a shorter name? We need to distinguish a return of a NodeElement from a return of an XML element.
Comment #22
dwwRe: scope - Yes, I realize @catch asked for it to happen like this. I disagree. 😅 I think the scoping is better if we only add the helpers here, not changing usages. I'd love to see drupalGet() and all the helpers backported all the way to 10.6.x. I don't think we're going to backport all the test conversions that far.
Re: name - I wrote:
I think
getNodeElementsByXpath()is great as-is. No need to change it. As I've written elsewhere, I'd rather methods with longer, self-documenting names than brief and ambiguous (or worse).Thanks!
-Derek
Comment #24
catchWe'll need a new, combined, backport MR with the various changes for an 11.x backport - won't be easy or necessarily even work to individually cherry-pick multiple commits from HEAD in the correct order directly. Easy to drop any test changes as part of that process.
Also we often include one or two conversions when adding a new API. The other option was to postpone the previous test conversion on this issue.
But also I don't really mind if it's done here or a follow-up so just going ahead. Committed/pushed to main, thanks!
Comment #27
joachim commentedI thought we were backporting each issue to 11.x, and the complicated backport was going to be 11.2/10.6?
Comment #29
catchYes good point... Cherry-picked to 11.x too.