Tests for non legacy zim files #1049

Rishabhg71 · 2023-08-13T18:55:57Z

closes #1042
Sorry I messed up my commit history in my last PR so I am reopening this after fixing the issue nonetheless

Rishabhg71 · 2023-08-13T19:13:55Z

@Jaifroid I have drafted this PR to get opinions and discuss some points before proceeding

Moving zim and segregations of files to proper folders in the tests dir

    |_____ runners
    |       |____ *.runner.js
    |
    |_____specs
    |       |____ *.spec.js
    |_____zims
    |        |____ .zim* (all zim files)

maybe a better structure or should I leave it as it is
2. Instead of making a new test suite every time shouldn't we only create a test suite for one mode
current implementation:-

describe('Legacy Ray Charles test [XZ compression] ' + (mode === 'jquery' ? '[JQuery mode]' : mode === 'serviceworker' ? '[SW mode]' : ''), async function () {
    describe('Load app', function () {
        it('Load Kiwix JS and check title', async function () {
            .......
        });
    });
    describe('Switch to ' + mode + ' mode', function () {
        it('Switch to ' + mode + ' mode', async function () {
            ........
        });
    });
    describe('Load archive', function () {
        it('Load legacy Ray Charles and check index contains specified article', async function () {
            ........
        });
    });
    describe('Navigate to linked article', function () {
        it('Navigate to "This Littlge Girl of Mine"', async function () {
            .........
        });
});

proposed implementation:-

describe('Legacy Ray Charles test [XZ compression] ' + (mode === 'jquery' ? '[JQuery mode]' : mode === 'serviceworker' ? '[SW mode]' : ''), async function () {
   
    it('Load Kiwix JS and check title', async function () {
        .......
    });

    it('Switch to ' + mode + ' mode', async function () {
        ........
    });

    it('Load legacy Ray Charles and check index contains specified article', async function () {
        ........
    });

    it('Navigate to "This Littlge Girl of Mine"', async function () {
        .........
    });
});

Jaifroid · 2023-08-13T19:26:03Z

@RG7279805 I agree we can organize the tests into appropriate folders, as you suggest. I have been building up these tests over some time, and I started with just one runner and just one test, and they have grown since, necessitating some organization.

I indeed originally had the structure in your proposed implementation. Unfortunately, however, Selenium runs all the individual tests simultaneously if they are not wrapped in separate describe statements. That is the reason each one is now wrapped. Because subsequent tests depend on the outcome of previous tests, we are forced to run them sequentially.

I'm aware that on Stack Exchange, many people advise that each it (individual test) should be separate and not dependent on others, but I think that advice applies much more to Unit Tests than to UI tests, where we want to follow a workflow through to the end in sequential order.

Hence, after a LOT of experimentation, I was forced to use this slightly unwieldly structure. However, in the GitHub Actions logs, it makes for a nicely readable set of test results like below. Therefore, I think we are obliged to keep spec test structure.

Feel free to experiment with a different structure if you think it is possible to organize it better.

Legacy Ray Charles test [XZ compression] [JQuery mode]
    Load app
(node:3618) [DEP0066] DeprecationWarning: OutgoingMessage.prototype._headers is deprecated
(Use `node --trace-deprecation ...` to show where the warning was created)
      ✔ Load Kiwix JS and check title
    Switch to jquery mode
      ✔ Switch to jquery mode (6194ms)
    Load archive
      ✔ Load legacy Ray Charles and check index contains specified article
    Navigate to linked article
      ✔ Navigate to "This Little Girl of Mine"
    Initiate search and navigate
      ✔ Search for Ray Charles in title index and go to article (7918ms)

  Legacy Ray Charles test [XZ compression] [SW mode]
    Load app
      ✔ Load Kiwix JS and check title
    Switch to serviceworker mode
      ✔ Switch to serviceworker mode (6181ms)
    Load archive
      ✔ Load legacy Ray Charles and check index contains specified article
    Navigate to linked article
      ✔ Navigate to "This Little Girl of Mine"
    Initiate search and navigate
      ✔ Search for Ray Charles in title index and go to article (8[51](https://github.com/kiwix/kiwix-js/actions/runs/5849071574/job/15857013978#step:10:52)6ms)


  10 passing (43s)

Jaifroid · 2023-08-13T19:31:05Z

I have been working all day on a PR that enabled remote testing on BrowserStack. It's now finished, and I've merged it. Unfortunately, this came at the same time as your draft PR. I have made quite a few changes to the Ray Charles spec test, so you will need to use the updated file on main. I've also added quite a lot of new runners for legacy browsers on BrowserStack.

You will need to resolve the conflicts or else start again from current main. I'm sorry about that. I hope it's not too much inconvenience. The tests are more robust now.

Rishabhg71 · 2023-08-13T19:33:07Z

@Jaifroid thanks for the insight as you are suggesting i will try to look for a better approach if possible (tests issue)

Also should i refactor the folders or do I leave that upto you?

Jaifroid · 2023-08-13T19:34:38Z

If you want to have a go at re-ordering the folders, please feel free. You should probably create a separate issue and a separate PR for that, and do that one first before you start work on this one.

Jaifroid · 2023-08-14T06:10:11Z

tests/chromium.e2e.runner.js

@@ -20,3 +21,4 @@ async function loadChromiumDriver () {
 const driver_chrome = await loadChromiumDriver();

 legacyRayCharles.runTests(driver_chrome);
+nonLegacyZim.runTests(driver_chrome);


Just a quick comment on why this doesn't work. You can't use the same driver instance, because it will be quit before the second test suite is run. You need to make a new instance, e.g. const driver_chrome_test2 = await loadChromiumDriver();.

Rishabhg71 · 2023-08-14T15:41:15Z

Hi @Jaifroid,
I hope you remember me from the PR I am working on #1049
I want to discuss a small issue with you here in this comment. When I try to run tests in this manner it seems to run sequentially. Am I missing something here?
Also is ok to DM you on Slack or Github is better

describe('Legacy Ray Charles test [XZ compression] ' + (mode === 'jquery' ? '[JQuery mode]' : mode === 'serviceworker' ? '[SW mode]' : ''), async function () {
   
    it('Load Kiwix JS and check title', async function () {
        .......
    });

    it('Switch to ' + mode + ' mode', async function () {
        ........
    });

    it('Load legacy Ray Charles and check index contains specified article', async function () {
        ........
    });

    it('Navigate to "This Littlge Girl of Mine"', async function () {
        .........
    });
});

Output:

  Reset app
    ✔ Click the app reset button and accept warning

  Legacy Ray Charles test [XZ compression] [JQuery mode]
    ✔ Load Kiwix JS and check title
    ✔ Switch to jquery mode
    ✔ Load legacy Ray Charles and check index contains specified article
    ✔ Navigate to "This Littlge Girl of Mine"
    ✔ Search for Ray Charles in title index and go to article

  Legacy Ray Charles test [XZ compression] [SW mode]
    ✔ Load Kiwix JS and check title
    ✔ Switch to serviceworker mode
    ✔ Load legacy Ray Charles and check index contains specified article
    ✔ Navigate to "This Littlge Girl of Mine"
    ✔ Search for Ray Charles in title index and go to article


  11 passing (17s)

Jaifroid · 2023-08-14T16:08:01Z

Of course I remember -- we were in touch only yesterday! It's best to discuss these issues here on GitHub.

I'm not sure why I was having issues with tests running simultaneously. Maybe it was with a slightly different structure, or maybe I was calling the individual tests asynchronously. I'm afraid I can't remember. If you've found a solution that ensures they run sequentially and simplifies them, then that's great 😊

Jaifroid · 2023-08-14T16:09:17Z

I suggest that before you undertake much more work on this PR, you could do the other PR we mentioned above, which is about organizing the folder structure.

Rishabhg71 · 2023-08-14T16:13:55Z

Hi @Jaifroid, I hope you remember me from the PR I am working on #1049 I want to discuss a small issue with you here in

One clarification on this part I was thinking to send this message on Slack but then didn't so I copied that message that's why it seems unusual

I suggest that before you undertake much more work on this PR, you could do the other PR we mentioned above, which is about organizing the folder structure.

Sure, BTW I am nearly done with all tests as soon as this PR gets merged i will start working above issue (or should i leave current tests and fix folder structure first?)

Jaifroid · 2023-08-14T17:24:02Z

I thought it might be easier for you to work on the other one first, as it would give you some experience with the way the tests work, the way imports work, etc. But either order is fine by me. It's really up to you.

By the way, I think you should give your new test spec a more meaningful name. I would recommend something like gutenberg_ro.e2e.spec.js. I think we can assume that all tests are for non-legacy archives unless we specifically label a test with legacy.

Please also note that I have now made the "Reset App" part of the test run only when the test is local, i.e. when it is not running in CI. Take a look at the latest code in the legacy test to see how to run only on local. We don't need to reset the app when running in GitHub actions because it is always running as a freshly built app in a virgin environment. Bypassing it speeds up the tests.

Given the changes I made, including adding some retries, you'll need to incorporate these into the new test as well as the legacy test. That's why it might be easier to do the other PR first, and then start this one again from the latest files. I won't be working more on tests, having just fixed the last issue (for now) I had identified with them.

Jaifroid · 2023-08-14T17:27:25Z

But as I wrote, the order you do them in is completely up to you.

Rishabhg71 · 2023-08-16T13:59:09Z

[UPDATE]: It took me more time than expected to write all the tests initially I thought I finish them in 2 days
This PR is still not ready for review as the tasks below are not completed yet

Review by me
Comments explaining tests
running those tests in IE and Firefox
Refactor of unoptimized/dirty code

All the tests are done as mentioned below let me know if I missed something

Click the app reset button and accept warning
Load Kiwix JS
Switching modes
Load archive
Active content warning
Sorting books by popularity
Sorting books by name
Change Language
Primary Search Autocomplete (Global search)
Viewing the book's HTML view
Navigating back to the main page(do we have to test back, forward, and home buttons)
Author search Autocomplete
Author search Results
Download the EPUB file

Jaifroid · 2023-08-16T14:07:07Z

That's wonderful, thank you!

I notice the tests aren't actually running on each PR push. I'm not sure if that's because you've disabled them or because of the conflicts that need to be resolved. Remember that you're working off an old copy of legacy-ray_charles, so you need to get the new one and apply the changes you made manually to it.

Also, remember that clicking the app reset button should be in the test, but it is wrapped so that it won't run during CI. It's only needed when a dev is testing locally. The code in the latest version of legacy-racy_charles will show you how to do that.

You won't be able to do some of those actions you list in jQuery mode, because it doesn't run dynamic scripts in the ZIM. So some of the tests won't be relevant in that mode. However, you can still navigate to a book cover using the title search and still view the HTML and download the epub. Downloading the epub and testing that it has downloaded might be a bit challenging, so it can be optional. Or maybe you worked it out already?

Rishabhg71 · 2023-08-16T14:44:55Z

@Jaifroid

I notice the tests aren't actually running on each PR push. I'm not sure if that's because you've disabled them or because of the conflicts that need to be resolved. Remember that you're working off an old copy of legacy-ray_charles, so you need to get the new one and apply the changes you made manually to it.

Yes it was annoying to run every test for legacy Zim and then run the actual code i am writing

Also, remember that clicking the app reset button should be in the test, but it is wrapped so that it won't run during CI. It's only needed when a dev is testing locally. The code in the latest version of legacy-racy_charles will show you how to do that.

aye aye sir

You won't be able to do some of those actions you list in jQuery mode, because it doesn't run dynamic scripts in the ZIM. So some of the tests won't be relevant in that mode. However, you can still navigate to a book cover using the title search and still view the HTML and download the epub. Downloading the epub and testing that it has downloaded might be a bit challenging, so it can be optional. Or maybe you worked it out already?

I have found a way to check that on chrome and Firefox
https://www.browserstack.com/guide/download-file-using-selenium-python

Jaifroid · 2023-08-16T14:47:45Z

I have found a way to check that on chrome and Firefox
https://www.browserstack.com/guide/download-file-using-selenium-python

Kudos!

- Base Tests - Load archive - Extra buttons and Language Dropdown

- Download EPUB file - Search books and Results

- Active content warning - Viewing article HTML view - Download File and check - exporting paths from single file

- Page Navigation - Viewing HTML page content

- [ENABLED] Gutenberg test enabled for chrome

Rishabhg71 · 2023-08-20T11:56:25Z

This is an excellent PR, well done! You've solved a lot of problems by yourself, and the code is largely working -- it works for me in a Chromium browser locally (I haven't tested Firefox yet).

Thank you that meant a lot but I did take a lot of time

I'm not sure why it's not working with CI (GitHub) -- you might need to add some diagnostics and test if the WebDriver has access to the file system on GitHub, for example.. Did you import Node FS methods, or is that supposed to be built into the WebDriver? If the WebDriver or browser is blocked from testing the presence of the downloaded epub, maybe you can simply test with Node methods whether it is in the FS. One thing I did notice is that my Chromium said that the epub had failed a virus test (which is ridiculous of course). It didn't seem to block WebDriver from finding the file, though. I think it just meant that the virus test hadn't been completed.

For now I am using the node's native fs module to check file download but I will try to dig if webdriver does have access to the filesystem. I will try to keep fixing it on my own

By the way, regarding my PR, you might have noticed several missing semicolons. Do you believe the project could gain from using Prettier? I would be happy to make a PR after tests refactor

Jaifroid · 2023-08-20T13:39:57Z

@RG7279805 If you're using Node's native FS module, I wonder if you need to import it explicitly. Where I use native FS in Kiwix JS Windows (for Electron), I have to do something like:

import { open, read, close, stat, readdir } from 'fs';

(That's probably not quite right, because in fact I have to use CommonJS module, with require rather than import, for Electron).

Jaifroid · 2023-08-20T13:40:47Z

But obviously, if WebDriver provides FS methods, then it would be a better solution.

Jaifroid · 2023-08-20T13:43:53Z

By the way, regarding my PR, you might have noticed several missing semicolons. Do you believe the project could gain from using Prettier? I would be happy to make a PR after tests refactor

By all means prettify your own file, but it's best not to prettify an entire project. Instead, I prefer to use ESLint and fix the files as I work on them. It's more flexible with the rules that can be defined. I probably just need to add a rule to enforce semicolon use, in which case your IDE will warn you more obviously. In any case, it's really minor, so don't worry about that for now!

Jaifroid · 2023-08-21T09:44:21Z

Just quickly to mention that I'm doing an Internationalization PR #1061, and it will have (is already having) small knock-on effects for e2e tests because some of our tests are checking language-specific UI for asserting that a change has happened. E.g., when we switch to jQuery mode, the legacy test looks for the text in the API panel that says that the SW API is not registered or is unavailable.

We'll need to change this so that tests for such functions don't depend on the loaded language. E.g., for the mode switching, I'll change the test so that it looks instead for the class name apiAvailable or apiUnavailable, which do not depend on the specific UI language loaded.

Rishabhg71 · 2023-08-21T15:33:01Z

@Jaifroid I have fixed the CI error issue finally 🤘. It wasn't a Filesystem problem rather the download button couldn't be clicked due to some reason (don't know why) so instead of calling .click() i downloaded the .epub via a .executeScript("<getElement>.click()")

We'll need to change this so that tests for such functions don't depend on the loaded language. E.g., for the mode switching, I'll change the test so that it looks instead for the class name apiAvailable or apiUnavailable, which do not depend on the specific UI language loaded.

Understood, When can I expect the tests to be updated or maybe you can merge this PR and I will fix the tests again?

Jaifroid · 2023-08-21T22:37:34Z

This is excellent news! Well done! Yes, I sometimes found that I had to resort to doing things in JS because WebDriver would error out on it, saying it couldn't be done... (clicking elements in particular).

My Internationalization PR won't be ready for a while yet because I'm trying to include at least one full translation of the app (into Spanish), and there are several more fiddly messages to convert.

So it would be best for us to sqaush/merge your PR when you tell me it's ready.

Could I check why you could not include the iemode.runner.js with your new spec test? Surely it would just run through the parts of the test that are valid in jquery mode, and leave it at that? The runner can also restrict the test to jquery mode also simply by passing the array ['jquery'] as the second argument when calling the test. If it's not possible, so be it, but it would be ideal if we could have even some basic ZSTD testing on IE.

There is one area you can't test in this PR because you don't have access to the Repo secrets. This is running tests on BrowserStack on older browsers. However, I think rewriting those runners could be a different PR. Especially if anything fails after we merge your PR and the full suite of tests are run.

Rishabhg71 · 2023-08-22T06:23:07Z

@Jaifroid There are 2 bugs in IE mode

Download and HTML view buttons not showing (buttons are there but not showing)
Even if the clicked download will not start instead I get redirected to this page with a blob

So it would be best for us to sqaush/merge your PR when you tell me it's ready.

tests in jquery mode are passing except for download .epub (passing since we can't check the actual downloaded file on Edge)
Let's merge the PR and try to fix the issue in another PR maybe?

There is one area you can't test in this PR because you don't have access to the Repo secrets. This is running tests on BrowserStack on older browsers. However, I think rewriting those runners could be a different PR. Especially if anything fails after we merge your PR and the full suite of tests are run.

Sure, I will fix any problem I may have to learn a bit about Browser stack though

Jaifroid · 2023-08-22T06:39:43Z

OK, that sounds good. It seems the Gutenberg ZIMs are increasingly coded in a way that is no longer compatible with IE11 (inevitable, really). We don't care about IE11 for its own sake, because even users of Windows XP can download a compatible Chrome and use that instead, but it is a good proxy for other older systems we might not be able to test for.

But in this case, it's clearly the ZIM that has "moved on", and we can't code anything much to compensate for that. So I agree, we should merge. I'll do a review in case there are any last small things to catch.

Jaifroid

The code is looking great. I just have some minor mostly formatting comments for final tidying. Let me know when you've done these, and I think we'll be ready to squash/merge.

tests/gutenberg_ro.e2e.spec.js

tests/ieMode.e2e.runner.js

tests/legacy-ray_charles.e2e.spec.js

tests/paths.js

tests/gutenberg_ro.e2e.spec.js

Jaifroid

Just some tiny last things!

tests/gutenberg_ro.e2e.spec.js

tests/legacy-ray_charles.e2e.spec.js

Rishabhg71 · 2023-08-22T09:31:01Z

@Jaifroid Can you merge this PR now?

Jaifroid · 2023-08-22T10:27:28Z

Many thanks once again for this PR. Great work and really useful for the project as a whole! About to merge.

Jaifroid reviewed Aug 14, 2023

View reviewed changes

Rishabhg71 force-pushed the main branch from 2c9fc18 to 784e5de Compare August 16, 2023 13:48

Rishabhg71 force-pushed the main branch from c264e27 to cb9ab9c Compare August 18, 2023 09:17

Rishabhg71 added 5 commits August 18, 2023 14:50

[ADD] Tests for non legacy zim files

35d502d

- Base Tests - Load archive - Extra buttons and Language Dropdown

[ADD] more tests for non legacy zims

1a41feb

- Download EPUB file - Search books and Results

[ADD] more tests and path refactor

16bd8f3

- Active content warning - Viewing article HTML view - Download File and check - exporting paths from single file

[ADD] All tests done

0980b3c

- Page Navigation - Viewing HTML page content

[RENAME] non legacy test -> gutenberg_ro

8cf1ada

Rishabhg71 force-pushed the main branch from cb9ab9c to 8cf1ada Compare August 18, 2023 09:22

Rishabhg71 added 6 commits August 18, 2023 15:06

[Refactor] Minor refactor + naming changes

cf409b7

[ADD] comments for tests

581fbc3

- [REFACTOR] All it running under single unit

fff9fc6

- [ENABLED] Gutenberg test enabled for chrome

[FIX] eslint error fix

91e3d39

[FIX] CI element not interactable

bbfd084

[FIX] CI ESLINT error

96bd67f

[REFACTOR/FIX] Changes requested by @Jaifroid

4f2cc7f

[FIX] trying to fox CI

f24fccc

[ADD] gutenberg tests enabled for IE mode

60059c8

Rishabhg71 force-pushed the main branch from 9146fb8 to 60059c8 Compare August 22, 2023 06:13

Rishabhg71 requested a review from Jaifroid August 22, 2023 06:23

Jaifroid approved these changes Aug 22, 2023

View reviewed changes

Jaifroid assigned Rishabhg71 Aug 22, 2023

Jaifroid added tests build Code relating to building, publishing, or maintaining the app user interface labels Aug 22, 2023

Jaifroid added this to the v4.0 milestone Aug 22, 2023

Jaifroid reviewed Aug 22, 2023

View reviewed changes

tests/gutenberg_ro.e2e.spec.js Outdated Show resolved Hide resolved

tests/legacy-ray_charles.e2e.spec.js Outdated Show resolved Hide resolved

tests/legacy-ray_charles.e2e.spec.js Show resolved Hide resolved

Rishabhg71 force-pushed the main branch from 886341f to 9eb52fc Compare August 22, 2023 08:36

[REFACTOR] closing changes for this PR

ba27048

Rishabhg71 force-pushed the main branch from 9eb52fc to ba27048 Compare August 22, 2023 09:21

Jaifroid merged commit 6689b8a into kiwix:main Aug 22, 2023
6 checks passed

Rishabhg71 mentioned this pull request Aug 22, 2023

Folder structure for test and proper file segregation #1062

Closed

Jaifroid modified the milestones: v4.0, v3.10 Sep 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tests for non legacy zim files #1049

Tests for non legacy zim files #1049

Rishabhg71 commented Aug 13, 2023

Rishabhg71 commented Aug 13, 2023 •

edited

Jaifroid commented Aug 13, 2023

Jaifroid commented Aug 13, 2023 •

edited

Rishabhg71 commented Aug 13, 2023

Jaifroid commented Aug 13, 2023

Jaifroid Aug 14, 2023

Rishabhg71 commented Aug 14, 2023

Jaifroid commented Aug 14, 2023

Jaifroid commented Aug 14, 2023

Rishabhg71 commented Aug 14, 2023

Jaifroid commented Aug 14, 2023

Jaifroid commented Aug 14, 2023

Rishabhg71 commented Aug 16, 2023 •

edited

Jaifroid commented Aug 16, 2023

Rishabhg71 commented Aug 16, 2023

Jaifroid commented Aug 16, 2023

Rishabhg71 commented Aug 20, 2023

Jaifroid commented Aug 20, 2023

Jaifroid commented Aug 20, 2023 •

edited

Jaifroid commented Aug 20, 2023

Jaifroid commented Aug 21, 2023

Rishabhg71 commented Aug 21, 2023 •

edited

Jaifroid commented Aug 21, 2023 •

edited

Rishabhg71 commented Aug 22, 2023

Jaifroid commented Aug 22, 2023

Jaifroid left a comment

Jaifroid left a comment

Rishabhg71 commented Aug 22, 2023

Jaifroid commented Aug 22, 2023

Tests for non legacy zim files #1049

Tests for non legacy zim files #1049

Conversation

Rishabhg71 commented Aug 13, 2023

Rishabhg71 commented Aug 13, 2023 • edited

Jaifroid commented Aug 13, 2023

Jaifroid commented Aug 13, 2023 • edited

Rishabhg71 commented Aug 13, 2023

Jaifroid commented Aug 13, 2023

Jaifroid Aug 14, 2023

Choose a reason for hiding this comment

Rishabhg71 commented Aug 14, 2023

Jaifroid commented Aug 14, 2023

Jaifroid commented Aug 14, 2023

Rishabhg71 commented Aug 14, 2023

Jaifroid commented Aug 14, 2023

Jaifroid commented Aug 14, 2023

Rishabhg71 commented Aug 16, 2023 • edited

Jaifroid commented Aug 16, 2023

Rishabhg71 commented Aug 16, 2023

Jaifroid commented Aug 16, 2023

Rishabhg71 commented Aug 20, 2023

Jaifroid commented Aug 20, 2023

Jaifroid commented Aug 20, 2023 • edited

Jaifroid commented Aug 20, 2023

Jaifroid commented Aug 21, 2023

Rishabhg71 commented Aug 21, 2023 • edited

Jaifroid commented Aug 21, 2023 • edited

Rishabhg71 commented Aug 22, 2023

Jaifroid commented Aug 22, 2023

Jaifroid left a comment

Choose a reason for hiding this comment

Jaifroid left a comment

Choose a reason for hiding this comment

Rishabhg71 commented Aug 22, 2023

Jaifroid commented Aug 22, 2023

Rishabhg71 commented Aug 13, 2023 •

edited

Jaifroid commented Aug 13, 2023 •

edited

Rishabhg71 commented Aug 16, 2023 •

edited

Jaifroid commented Aug 20, 2023 •

edited

Rishabhg71 commented Aug 21, 2023 •

edited

Jaifroid commented Aug 21, 2023 •

edited