Review textContains with html new lines
Closed this issue · 22 comments
textContains behavior changed unexpectedly, now it requires taking into account generated html newlines. Review this behaviourand implement test cases to check if it is a wanted behavior
I was looking for this 👍
I've been playing a bit feel a bit frustrated, let me explain:
If you do any selection (querySelectorAll) and want to get innerText, break lines are added when that html selection contains several HTML elements which is what browser.assert.text() should do IMHO, but with textContent & browser.assert.textContains() break lines shouldn't be added, check the following example:
I've checked very quickly the code (and could be wrong), but seems like textContains uses text under the hood, that makes the change I think...
PS: Happy New Year :P
Same applies when using text-transfrom
innerText -> what we see rendered
textContent -> what is written
So, as I understand, the only method behaving unexpectedly for you is browser.assert.textContains which should not add new lines right? @ZiFFeL1992
Wouldn't it be inconsistent if textContains ignore newlines while text doesn't?
Nope, they we'll work like native API does as you can see in the image attached, the only thing is naming, could be confusing that they behave different, I would add it to docs, but it was something I didn't know until today, I don't know if people use to know that both exist... To keep consistency with native API maybe textContent is more accurate...
Keep in mind that textContent is not the same as textContains
Regardless of native API, the original idea of text/textContains is the following:
- Text checks the same exact string
- TextContains accepts any substring (so, "david" is contained in "david hasselhoff")
https://stackoverflow.com/questions/35213147/difference-between-textcontent-vs-innertext provides a nice explanation for the native API where (as far as I understand) the key difference relies on whether only visible text or also extra elements (e.g. <br> as newlines) are returned. Currently, this last option is what is used across wendigo afaik. I'm not sure if that is the correct option, but I feel the same should be used on all places unless a clear naming is given for all these different options (I agree if a new method is added, matching the native naming, the same behavior should be expected)
So I see 2 issues here:
- Is adding newlines the correct behavior for text assertions?
- Is it worth it adding new methods/options to expose both options (textContent/innerText)?
Not sure if there are more things to consider. Thoughts @ZiFFeL1992 ?
The question for me is why would you assert with break lines? They'll be added and it's harder to make assertions IMHO, why did you decide to do that change, maybe some background would help...
If you want to keep it like that I would add a modifier for both text/textContains to work with textContent/innerText, I feel like adding another two assertions will made usage harder.
The change in assertions is a side effect of modifying browser.text which returns newlines (as it should). I'm happy to change the behavior of the assertions, but it should be modified for both IMO unless there is a reasonable reason against that (so, both assert.text and assert.textContains ignore newlines) would that make sense?
Why would assertions and browser.text work different? I can imagine that you have a long paragraph and you want to break line manually for whatever reason, in that case a break line in browser.text "would" make sense, but are there any other cases where you'd like to take into account line breaks?
But I'm agree, both assertions should work with textContent.
Essentially, I think getting a text should return the most accurate depiction of the text (preferably, with newlines). For consistency, assertions work the same way.
However, I see why assertions may be messy when taking parsed newlines into account, so it makes sense to change their behavior
I'm agree with that, but check the following example:
<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="UTF-8">
<meta name="viewport" content="width=device-width, initial-scale=1.0">
<meta http-equiv="X-UA-Compatible" content="ie=edge">
<title>Document</title>
</head>
<body>
<p class="p1">
Lorem ipsum dolor, sit amet consectetur adipisicing elit. Ab dignissimos facilis aspernatur sit magnam quasi, <br>architecto ipsam natus optio numquam fugit obcaecati doloribus magni nihil minima?<br> Sint aperiam similique cum.
</p>
<p class="p2">
Lorem ipsum, <div>dolor sit amet consectetur</div> <span>adipisicing elit</span>. Quisquam impedit repellendus ipsum excepturi praesentium deleniti quidem illum voluptates vero sapiente blanditiis eligendi id, quas voluptatum eveniet dignissimos aspernatur repellat quis?
</p>
</body>
</html>
innerText cuts the text while textContent will get everything, am I missing anything?
Is fine if browser.text only takes into account br elements, but it doesn't work like that as far as I know (have tested).
Great example, could you give me the same example but using wendigo text + assert.text to make sure what the expected vs actual behavior are? It's getting slightly confusing talking in terms of native inner and content, I think I'll understand better that way
Thanks
There you go:
wendigo-text.zip
There's something I can't understand, why selector.textContent returns the whole text inside a selector (ignoring child elements) in my browser but it doesn't in puppeteer (check test text with html elements)
IMHO that test should work as it is right L28 in main.test.js, but evaluate doesn't return what I expect even using document.querySelector, and should work on L29 after fixing what we have discussed, let me know if I can help :)
Run instructions:
npm run serve in one shell and npm test in other shell.
Hi @ZiFFeL1992 I've integrated those 2 tests as are, the result I get is as you say:
-Lorem ipsum dolor sit,
+Lorem ipsum dolor sit amet consectetur adipisicing elit. Ad quisquam ex ut in quae dolore minima nulla corporis molestiae porro, voluptatum explicabo neque consequuntur eos? Itaque doloribus voluptate sapiente dicta.However, running the same commands in my browser (Chrome) I get consistent results with the test:
Then is an issue with puppeteer it self?
erm, I don't think so, as I show in the screenshot, the behavior is the same in chrome. What browser are you using?
New test cases added to cover this behavior, branch fix-text-with-newlines will be used for development of this in case you want to review the changes/contribute @ZiFFeL1992
Can you open the PR, so I can take a look? :)
It is not fixed yet, only the tests are on that branch (yay, TDD) a diff with dev should give you the details, I'll open the PR once this issue is fixed and ready to merge to dev
diff --git a/tests/dummy_server/static/lorem.html b/tests/dummy_server/static/lorem.html
index 3159343..1bfd3f2 100644
--- a/tests/dummy_server/static/lorem.html
+++ b/tests/dummy_server/static/lorem.html
@@ -3,9 +3,7 @@
<head>
<meta charset="UTF-8">
- <meta name="viewport" content="width=device-width, initial-scale=1.0">
- <meta http-equiv="X-UA-Compatible" content="ie=edge">
- <title>Document</title>
+ <title>Text / Assert Text</title>
</head>
<body>I'd also change filename, everything else looks good to me :)
Doesn't really affect tests, name is only to identify uniquely among all the .html files for testing. Feel free to make a PR with these changes (target branch being fix-text-with-newlines)
Changes addressed, these will be published in wendigo@2.11