-
Notifications
You must be signed in to change notification settings - Fork 54
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
https://github.com/jakartaee/faces/pull/1732: Selenium tests for the ajax TCK (master branch) #1770
Conversation
Old pull request (now closed): #1732 |
(accidentally added debugging version)
* @see AjaxBehavior | ||
* @see https://github.com/eclipse-ee4j/mojarra/issues/2166 | ||
*/ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this change was unintentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes very likely a copy paste error, will fix that tomorrow.
Fixed... there was some code which was carried over from the old port, it now is back in sync with the master branch! |
…so relies on ajax!
smaller rename for the custom runners
ping, is this going to be merged? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approve in principal. We do need to make double sure this works correctly on our Eclipse CI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to run all tests locally, just Issue2162IT fails, not sure why. I will run it yet with -fae.
[INFO] Running ee.jakarta.tck.faces.test.servlet30.ajax_selenium.Issue2162IT
Jan 12, 2023 5:36:32 PM org.openqa.selenium.devtools.CdpVersionFinder findNearestMatch
WARNING: Unable to find an exact match for CDP version 108, so returning the closest version found: 107
[ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.533 s <<< FAILURE! - in ee.jakarta.tck.faces.test.servlet30.ajax_selenium.Issue2162IT
[ERROR] ee.jakarta.tck.faces.test.servlet30.ajax_selenium.Issue2162IT.testIssue2162 Time elapsed: 0.43 s <<< FAILURE!
java.lang.AssertionError
at ee.jakarta.tck.faces.test.servlet30.ajax_selenium.Issue2162IT.testIssue2162(Issue2162IT.java:52)
Thanks for report investigating it, seems like this is a mojarra issue (I have had marked this code for further investigation, but forgot to do it) The test is exactly the same as in HTMLUnit, but it seems like the render all issued in this case fails on Chrome. Here is the same test running on MyFaces and our reimplemented faces.ts scripts: [INFO] Running ee.jakarta.tck.faces.test.servlet30.ajax_new.Issue2162IT
Jan 12, 2023 1:46:47 PM org.openqa.selenium.devtools.CdpVersionFinder findNearestMatch
WARNING: Unable to find an exact match for CDP version 108, so returning the closest version found: 107
[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 1.724 s - in ee.jakarta.tck.faces.test.servlet30.ajax_new.Issue2162IT So the tests passes clearly on MyFaces with the new scripts, I suspect the problem might be somewhere on the faces.js side on Mojarra, which probably uses somewhere a construct which fails on newer browsers but passes on html unit. I will do a deeper research on it. Btw. not the only case where one of the testcases failed on Chrome, there were 2-3 tests which relied on non dom compliant usage of a Textarea as result logger, which resulted in failing tests on Chrome. Those were fixed by me. But this is not test related. I will come back later with the results of what I can find out. I also will have another look at the CDP Warning, this is ugly! Update: |
It seems the cause might be dependency on local operating system settings, I have cs_CZ lang and locale, when I was running unchanged version of this test from GlassFish, it passed (all tests).
But it also inherits this from our basic surefire+failsafe settings:
So I believe it is not a problem of this PR. |
Found the issue regarding the failing test. In fact, this is the first advantage we see here from browser testing. It indeed either is a Chrome bug or a Mojarra issue, but in fact following happens. Following code is the problem: var copyChildNodes = function copyChildNodes(nodeFrom, nodeTo) {
...
} else {
var ownerDoc = nodeTo.nodeType == Node.DOCUMENT_NODE ? nodeTo : nodeTo.ownerDocument;
var i;
if (typeof (ownerDoc.importNode) != "undefined") {
for (i = 0; i < nodes.length; i++) {
nodeTo.appendChild(ownerDoc.importNode(nodes[i], true))
}
}
...
}; What happens here is that: the ownerDoc.importNode swallows text nodes and drops them. MyFaces does not run into this issue, because my code does not use importNode for the dom element replacement. Here you can see the browser console output regarding what happens. I will file a bug report on this on the RI side. I have filed an issue for this under: #1776 |
Re the CDP Warning, this seems to be a Selenium issue. The Chrome driver and the underlying Selenium version seem to be in a slight mis-alignment regarding the Chromium Debug tools version (which I use internally to provide some functionality, which HTMLUnit has per default, but the chrome driver does not) It is a warning, but nevertheless I will look into it, how I can fix this. |
Awesome analysis and explanation! |
…st selenium webdrivers
I have fixed the CSP warning in a way that it is displayed only once. Note... Selenium atm only supports chrome until 108, so a warning is expected if you trigger a newer webdriver. It does not really matter because it works nevertheless. Also we might have to do smaller adjustments on the code for ci integration because the webdrivermanager downloads the chrome webdrivers for the installed chrome automatically. This might cause problems with proxy environments, or if we do not want that. There are other ways to initialize an existing webdriver. I might add a manual driver location setup via environmental parameters on Monday! |
Re CI Integration, please collect info what you still need, then I can add it on monday. |
…auling Spec1423 so that it uses less timing and more waiting for page conditions to avoid race conditions
button.click(); | ||
page.waitReqJs(Duration.ofMillis(6000)); | ||
assertTrue(page.findElement(By.id("scriptResult")).getText().trim().equals("addedViaHead")); | ||
assertTrue(page.findElement(By.id("stylesheetResult")).getText().isEmpty()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not page.waitForCondition()?
|
||
|
||
assertTrue(page.findElement(By.id("scriptResult")).getText().trim().equals("addedViaHead")); | ||
assertTrue(page.findElement(By.id("stylesheetResult")).getText().isEmpty()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary, these are already asserted by page.waitForCondition().
button.click(); | ||
page.waitReqJs(Duration.ofMillis(6000)); | ||
assertTrue(page.findElement(By.id("scriptResult")).getText().trim().equals("addedViaBody")); | ||
assertTrue(page.findElement(By.id("stylesheetResult")).getText().trim().equals("rgb(255, 0, 0)")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not page.waitForCondition()?
|
||
page.waitReqJs(Duration.ofMillis(6000)); | ||
assertTrue(page.findElement(By.id("scriptResult")).getText().trim().equals("addedViaBody")); | ||
assertTrue(page.findElement(By.id("stylesheetResult")).getText().trim().equals("rgb(255, 0, 0)")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not page.waitForCondition()?
page.waitReqJs(Duration.ofMillis(6000)); | ||
|
||
assertTrue(page.findElement(By.id("stylesheetResult")).getText().trim().equals("rgb(0, 255, 0)")); | ||
assertTrue(page.findElement(By.id("scriptResult")).getText().trim().equals("addedProgrammatically")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not page.waitForCondition()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
waitforCondition is only possible if a state change is supposed to happen after a click.
I will recheck the code if I missed a state change, but that is the basic premise where it can be applied in this case (see also the other comments)
WaitforCondition means we wait for a change, if there is none, we still have to use normale timed waits, not ideal, but there is no other way. I have added waitForCondition wherever it made sense for this test! |
assertTrue(page.findElement(By.id("scriptResult")).getText().trim().equals("addedViaBody")); | ||
assertTrue(page.findElement(By.id("stylesheetResult")).getText().trim().equals("rgb(255, 0, 0)")); | ||
|
||
button = page.findElement(By.id("form2:addViaInclude")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probably better if page was reloaded so that the script/style results of previous test step are guaranteed to be cleared out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes that would mean a complete test rewrite, also this would miss the double include check. (Aka if a script already is loaded it should not be executed a second time)
This test is rather complicated in a sense that it tests for several things at once., first whether the elements are updated, and seondly, that the page does not run into an illegal update state by retriggering certain update patterns over and over again in various combinations.
Btw generally spoken, it would make sense to replace the waits with waitForCondition in the other tests as well, but for this one it was the most pressing.
The main difference and that is a huge one, between HtmlUnit and Selenium is, that with Selenium the browser runs in a separate process.
Hence there is no real synchronisation, between the driver and the browser. The best bet is to wait until the dom is updated into the condition you want to assert, if the browser does a long running or asynchronous operation, otherwise you might run into unwanted race conditions if you set your wait windows too tight!
}); | ||
|
||
|
||
button = page.findElement(By.id("form1:addViaHead")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probably better if page was reloaded so that the script/style results of previous test step are guaranteed to be cleared out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above for my comment on this! Not possible!
Hi, I noticed that at least one test is different (and I can look for others): Was this intentional? @werpu |
No not intentional, this was a regression on my behalf, could have been an accidental copy paste issue. Have you found others? |
I just performed a quick diff on the contents of the ajax and ajax_selenium folders for each of the tcks. I didn't see any other problems. It looks good |
I have re-ported the Ajax tests again to Selenium but this time properly in master.
Main differences: Latest framework base version (note this is under ASL2 license)
tests now can be started via -Dtest.selenium=true this will skip the HTMLUnitTests and run the selenium tests
if you leave that out the tests behave as before.
I will close the old pull request, but link it in.