In #2653574-142: Unable to keep nested IEF data separate with multivalue fields. we saw that any change in HTML IDs breaks our tests.
Let's try to make this robust and maintainable.
| Comment | File | Size | Author |
|---|---|---|---|
| #23 | inline_entity_form-3103609-23.patch | 56.72 KB | geek-merlin |
Comments
Comment #2
geek-merlinInput from folks with more expirience in such tests appreciated!
How does core and/or other modules do this?
Comment #3
geek-merlinLooked a bit into \Drupal\FunctionalJavascriptTests\JSWebAssert and the proposal to use XPath seems to make most sense to me.
Comment #4
spokjeGoing to make a stab at it tomorrow.
Comment #5
joachim commentedXPath can be pretty hard to read.
Would using Mink's 'named' selector here work with the HTML we have? -- see https://symfonycasts.com/screencast/behat/finding-elements#find-via-the-...
Comment #6
spokjeThanks @joachim, that's certainly worth a try!
I'm waiting until these test nitpicks land before I'm going in.
Comment #7
spokjeComment #8
geek-merlin> XPath can be pretty hard to read.
Good point. If we need them, we should at least have link name comments for maintainability.
I'd prefer the name selector (text/title/alt) too. But in our case we'll by designe have several "Title"s. Can we discriminate them? Something like
$session->getXpathFragment('/path/to/second/ief')->findName('Title')?Comment #9
geek-merlinThought and researched a bit.
Some selenium tutorials speak about "relative selectors" beginning with "//" that "start in the middle of a document". That is plain nonsense in xpath terms, as "//" is the any-descentant syntax.
Comment #10
geek-merlinBut it's quite simple to have something like
getElementByNthContextAndLabelText($cssClass, $index, $label)that spits out xpath like//*[contains(concat(" ", normalize-space(@class), " "), " {$cssClass} ")][{$index}]//*[@id=string(//label[.='{$label}']/@for)]. (Note that most of the complexity stems from the fact that i chose CSS class for context and we must teach XPath to parse CSS classes.)(The XPath is tested on a form via the inspector's find-by-XPath feature.)Comment #11
geek-merlin@Spokje, any news on this? Need any help (or just time ;-)?
Comment #12
spokje@geek-merlin
It's time (seems to be a rare resource amongst Developers somehow...).
What needs to be done is clear, how to do it also (or at least some very good suggestion were made in the above comments).
I _hope_ to tackle it during this week, but if anybody beats me to it: It would not be a problem, I haven't assigned myself to it, so the issue is open for all and everyone :)
Comment #13
spokjeA week, two months, semantics...
Anyway: I finally had the time to tackle this one.
In the end I went for a solution that's slightly less fancy than proposed, by just getting something like "The second input field with label 'Title'".
Attached patch passes locally, so let's see what TestBot thinks.
Note:
1) I only transformedHTML IDs for input fields, not buttons, This with issue #2653574 in mind, where we _really_ need HTML ID agnostic input fields
2) I didn't touch
SimpleWidgetTest checkEditAccess, because that largely is a test to see if input fields are _not_ there. With my setup it's hard/impossible to test if the second input out of three isn't there, since the third input becomes the second. (Hopes that makes some sense to somebody...)Comment #14
spokjeComment #15
spokjeNot sure why but
/inline_entity_form/tests/src/FunctionalJavascript/ComplexWidgetRevisionsTest.phpisn't triggered by TestBot, I made changes to that file as well, and they pass locally.Comment #16
geek-merlin@Spokje Impressing work!
Now i found some time to look into this. I like the approach of generating the xpath, this makes the magic less magic.
I'll play with the xpath to try make that more specific.
Comment #17
geek-merlinI took the xpath-fu of #10 and adapted it to label. Now we should get exactly the right element even in edge cases. Let's see if that holds.
Comment #18
geek-merlinCool this passes too.
I went all through this patch and everything looks good to me.
More than a hundred times pain relief like this, how hot is this...🐳
Comment #19
spokje@geek-merlin Nice stuff! I should have made that generic approach myself, but after spending hours of endless search-replace-retest on each individual test I somehow lost focus there.
All's green, let me have a quick review and if all's Ok (which it certainly looks like it is!), I'll make this RTBC
Comment #20
spokje@geek-merlin Alas, nothing is ever easy...
Currently the test-class
ComplexWidgetRevisionsTestisn't triggered by TestBot (See here).The changes do pass locally, but I think we need to fix that first.
I've created this issue for that.
If that passes green and is committed, we can reroll your patch #17, if that passes green, I'm happy to RTBC that.
(And _then_ we can finally tackle this issue, which was what I came in for, many, many moons ago)
Comment #21
spokje3118610 came back green.
Comment #22
spokjeComment #23
geek-merlin> And _then_ we can finally tackle this issue, which was what I came in for, many, many moons ago
Aah, i remember dimly... Hey, passing the marshmallow test is said to be a predictor for a good life. ;-)
I committeed the other issue, thanks for that! And here's a re-roll.
Comment #24
spokjeNow you got me wondering if that is a good life _now_, or two good lives later...
Anyway:
ComplexWidgetRevisionsTestis now included in the tests, all green: RTBC for me :)Comment #26
geek-merlinSo finally that's in. Champagne! And marshmallows. 🍾🥂