-
-
Notifications
You must be signed in to change notification settings - Fork 148
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
Added an automated Selenium UI test for a small Zimit2 archive #1286
base: main
Are you sure you want to change the base?
Conversation
@Jaifroid I have not added test for android & ios page because i didn't know what test should i do, your suggestion will be appreciated 😅😃 Thanks! |
Thank you @THEBOSS0369. That's very useful. I would suggest adding a test that switches to the page that shows the Android and iOS store images, and verifying that both images have loaded correctly:
|
Hey @Jaifroid ! I have added the android and ios test as you asked me but there is a problem occurring when i am running the tests the i have tried by making the waititme 120000 , by removin cache but still this won't work. I don't understand what i am doing wrong i checked in the sw mode the image's alt is present and i have added the same in the code. Your help will be appreciated . Thanks! |
Thanks @THEBOSS0369. Sorry to hear about the error. It's tricky to get the assertions right with the timing. I'll take a look at the weekend, but in the meantime it's possibly the case that there's some error in selecting the images and checking their natural width. Adding some console logging to see what it's actually finding and what you are expecting it to find might help! |
OK, I ran this myself (in the Edge runner, adding the appropriate commands), and while watching the browser locally it's clear that the test never actually loads the page "Android & iOS App". It remains stuck on "Ear Training Practice" (see screenshot). You're not going to be able to verify the images on the "Android & iOS App" page unless you actually switch to it first! |
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.
Please see below for the steps to remedy the issue you're encountering.
@@ -27,6 +28,8 @@ async function loadFirefoxDriver () { | |||
// so we need to use the second one first | |||
const driver_for_gutenberg = await loadFirefoxDriver(); | |||
const driver_for_ray_charles = await loadFirefoxDriver(); | |||
const driver_for_tonedear = await loadFirefoxDriver(); |
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.
As the comment says above this block, the drivers need to be listed in reverse test order here, so that when the user runs the tests locally we see the Ray Charles first (which will be on top of the other windows). So please put the tonedear test above gutenberg in this list, and update the comment to reflect that there are three tests
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
aa8674e
to
66aeef5
Compare
@Jaifroid Another problem occurring , when i try to navigate to Android Page it won't, i think this is happening because i didn't know the exact path for it at first i used this then i looked into the Then i think i can do that by link but The link for the server remains same Can you help me with this path problem if we get it fixed then every tests will pass correctly. |
@THEBOSS0369 Oh no, this app doesn't support navigating to a page by URL fragment. There are two ways to get to a particular page: either click on the link that you highlight in the screenshot (easiest), or search for it using the search bar. Also, remember that the links you're clicking on are in the iframe, not at the top level, so you have to switch to the iframe before finding the link and clicking it. Take a look at the code in the Ray Charles test to see how to switch to the iframe. This explains why you said you were finding 21 images in the landing page when in fact there are none! You were finding these images in the app itself, as you hadn't switched to the iframe containing the content. |
@Jaifroid Sirrr ! it worked now its navigating to andorid page although the images are still not found now i will try to fix that |
@Jaifroid Sir all The tests have passed . Does any thing else needed to be added in the test? If yes then do let me know. The comments in this image for tonedear tests will not be visible in the main tests i have commented it out. |
@THEBOSS0369 That's very good news! I don't think we need more tests. I'll review the new code as soon as I can, but in the meantime, you might want to clean it up, remove any excessive timeouts you previously added, etc. Once the spec is ready, you should add it to all the other runners in the folders under Oh, I see GitHub tests have failed. I'll re-run those to be sure it wasn't a timeout fluke. |
@Jaifroid I think the tests are failing because when it navigate to android page it asks me to authorize the author and i used to press that maybe that's causing error. |
@Jaifroid Yes this is happening due to the reason i told above i didn't clicked the trust source this time and tests failed. I don't know how to fix this one |
This is because you erased https://github.com/kiwix/kiwix-js/blob/main/tests/e2e/spec/legacy-ray_charles.e2e.spec.js#L125 (the setting "noPrompts=true") from the URL when loading the app. This was in your previous spec, but then you did a hard reset of your branch, and you erased it, so it's no longer in the commit history. That command prevents the security prompt from showing. All you need to do is copy the appropriate code block from the ray charles version of the test. Also, in future, try to avoid rewriting history in your PRs. It makes things difficult to review as well, because when you do a force push, I have to reset my copy of your PR instead of simply pulling a new commit! (Don't worry, everyone makes these kinds of mistakes with PRs when learning.) |
@Jaifroid Done Sir! Now all tests passes well without any issue. I'm waiting for these tests to pass then i will add the tests in all the files and remove any unnecessary time waits.
My apologies Sir i didn't knew force push can do this much damage. I will be careful from next time 😅 |
sir i have reverted all changes and tets are passing in my local let's see if it passes this time or not |
@THEBOSS0369 Unfortunately this is now failing on Chrome 60. So it looks like it wasn't browser-specific. Here are the two failed runs:
I can give you write access to the Repo so that the BrowserStack tests run without my needing to intervene. However, my feeling is that the code to dismiss the security prompt may actually be the problem. As mentioned, if called correctly with the noPrompts querystring this prompt should never show, so we shouldn't need to have code dealing with it. I would try removing that, and then if we find that there is a security prompt, we can fix that with the npPrompts... |
@THEBOSS0369 An invite has been sent to you to accept write access to the Repo. Use this with extreme responsibility. It should mean the BrowserStack tests run when you push (after you've accepted the invite). I can give you access to the BrowserStack system so you can look at the videos for yourself. For that, please send me a private message on the Kiwix Slack. |
(You may need to push directly to the Repo for the BS tests to run, rather than pushing from a fork, I'm not 100% certain. Try pushing as normal, and if it doesn't work I can explain how to push directly to a branch, so the tests run.) |
Got it sir i will do the pushing with extreme care without touching anything else. |
All the tests are passing after using the code from Dummy PR #1296 . Just need to run the BS test to ensure everything is working. |
@Jaifroid Sir I have checked the Code here is same as the dummy PR. the extra code has been removed from here as well |
tests/e2e/spec/tonedear.e2e.spec.js
Outdated
return await img.isDisplayed(); | ||
})); | ||
return visibility.every((isVisible) => isVisible); | ||
}, 10000, 'No visible store images found after 30 seconds'); |
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 think this message should read "10 seconds", right?
tests/e2e/spec/tonedear.e2e.spec.js
Outdated
} | ||
|
||
modes.forEach(function (mode) { | ||
const serviceWorkerAPI = true; |
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.
You always set serviceWorkerAPI as true here, and it can't be changed (it's a const). Instead, we need to use a let here, and then change it if the serviceWorkerAPI isn't available, which is the case in some browsers.
tests/e2e/spec/tonedear.e2e.spec.js
Outdated
}, 5000); | ||
await driver.sleep(1300); | ||
|
||
if (mode === 'serviceworker' && !serviceWorkerAPI) { |
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.
This code block will never run because serviceWorkerAPI is stuck on true. You need to test whether serviceWorker API is supported or not. See the Gutenberg test to see how it's done.
}); | ||
|
||
it('Navigate to Android & iOS section', async function () { | ||
if (!serviceWorkerAPI && mode === 'serviceworker') { |
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.
This code will never run for the same reason.
}); | ||
|
||
it('Verify Android and iOS store images in ' + (mode === 'jquery' ? 'Restricted' : 'ServiceWorker') + ' mode', async function () { | ||
if (!serviceWorkerAPI && mode === 'jquery') { |
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.
Ditto, code will never run.
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.
There appear to be some issues arising from the fact that the test for the serviceWorkerAPI was removed and it was set only to true
. I think it's important to test for this, and the code is provided in Gutenberg test, so easy to add.
@Jaifroid Sir i have added the code from the gutensberg, for const issue i tried by adding |
Sir the test failed like this last time as well. If we just simply re run it then it will pass. For confirmation i will edit the code in dummy pr. |
The tests are passing in the dummy pr. So it should for this one as well this time. |
This PR Fixes #1205
Hello Everyone!
Description
In this PR i have added an automated Selenium UI test for a small Zimit2 archive (used
tonedear.com_en_2024-09.zim
)The tests i have added are as follows :
Test
I have done all the necessary test
npm test
no issuetests-e2e-firefox
-> All tests Passed Below is the ssScreenshot for tests