-
-
Notifications
You must be signed in to change notification settings - Fork 146
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
Dummy PR for Adding selenium ui test #1296
base: main
Are you sure you want to change the base?
Conversation
I will be resuming my work on this issue Now. Thanks for your patience 😃 |
Unfortunately, Edge Legacy isn't fixed, the Promise code didn't work, merely masked the errors because the script ended prematurely. BrowserStack videos show that the last test still timed out. |
Oh no, I didn't noticed that. |
So I'm reverting changes tried so far and now trying to increase timeouts to solve Edge Legacy test first. Then we can look at Firefox 70, though I won't get to that till the weekend. |
No problem sir |
Maximum timeout of 300 seconds isn't sufficient. So we need now to restructure the tests so that the window isn't opened and left idling before receiving any commands. The approaches tried to make this happen so far haven't worked because runTests isn't fully async, so synchronous parts of the tests get executed immediately even when chained within Promises. Restructuring this is more involved than I can do today, so this will have to wait. In the meantime, I've disabled all BS tests except Edge Legacy so we can focus on that one. |
So, I solved the Edge Legacy tests by running them in a single driver, so that the other drivers are not hanging around waiting for tests to complete. This works fine and the pattern in https://github.com/kiwix/kiwix-js/blob/selenium-uitest/tests/e2e/runners/edge/edge18.bs.runner.js#L40 can be used in other cases where we have any timeout issue. I also added stronger prevention of the security popup in 6178c1c, so that shouldn't bother you now. You may find that the issue in Firefox 70 is now fixed. In any case, you can now go ahead and test that! Fingers crossed.... 🤞 |
tests/e2e/spec/tonedear.e2e.spec.js
Outdated
const swRegistration = await driver.executeScript('return navigator.serviceWorker.ready'); | ||
assert.ok(swRegistration, 'Service Worker is registered'); | ||
|
||
console.log('Current URL:', await driver.getCurrentUrl()); |
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.
Just a reminder to remove test logging once no longer needed.
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.
Done Sir
@Jaifroid Sir Thanks for fixing the tests, that dialog box error is fixed now in my local server. I'm commenting out the ff70's test for tonedear let's see if it passes this time or not. |
tests/e2e/spec/tonedear.e2e.spec.js
Outdated
try { | ||
const activeAlertModal = await driver.findElement(By.css('.modal[style*="display: block"]')); | ||
if (activeAlertModal) { | ||
serviceWorkerAPI = await driver.findElement(By.id('modalLabel')).getText().then(function (alertText) { | ||
return !/ServiceWorker\sAPI\snot\savailable/i.test(alertText); | ||
}); | ||
const approveButton = await driver.wait(until.elementLocated(By.id('approveConfirm'))); | ||
await approveButton.click(); | ||
} | ||
} catch (e) { | ||
// Do nothing | ||
} |
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.
@THEBOSS0369 I think you could remove these lines. Although it's not the cause of the current error, it will never now find this element, and it introduces overhead (and confusion about the cause of failure in the logs). I'll test on a local copy of FF70 to see if there's an obvious cause of the failure to navigate to the Audio app page.
Indeed, replaceAll isn't supported in FF < 77: https://caniuse.com/?search=replaceAll I can't remember, but I think we polyfill it in legacy MS browsers. Will check. |
OK, this file wombatSetup.js is loaded inside the ZIM, so we can't easily polyfill it. It's just an incompatibility with Firefox < 77 when running Zimit archives. What I suggest is that you run the test only in jQuery (Restricted) mode for the FF70 test, as we know that's passing, and we can't work around this error. Nothing to do with your test, Zimit archives will just fail in FF < 77, and users should switch to Restricted mode. I'll see if I can fix that as an automatic switch, but it's another issue. So, please don't spend more time on this, just run this tonedear test (the FF70 one) only in jQuery mode. |
100% Agree sir I was also thinking the same way but I was thinking that we should skip the Verification of Images only in ff70 as all other tests are passing in sw mode except this. Let me know whether we should skip the verification of images test specifically for sw mode or should we follow as you suggested. Also I will be able to do the changes tomorrow morning. |
No, I think we should skip the test in SW mode completely in FF70, because it's not a valid test if the app just can't navigate to any other page than the home page (which is the case). It would be more meaningful to skip SW mode in this browser (for tonedear) because it also gives a signal to devs that the browser doesn't actually support Zimit archives in SW mode. If you like you could add a comment in the code to explain why we skip it in that browser. |
@Jaifroid Got it sir, that makes sense. I have removed the extra code lines and changed the code for test to skip sw mode for tonedear. i'm waiting for the tests to pass. Once these test passes i will copy the codes in the main PR from where we started. |
This PR is a Dummy PR for Adding selenium ui test and is successor of PR #1286 created for ensuring tests passing or not in browser stack.