Converted Q Unit tests to Mocha Tests #1301

THEBOSS0369 · 2025-01-14T06:32:32Z

This PR Fixes #1187

Description

In this PR i have converted Q Unit tests to Mocha Tests.

Tests Converted

All the tests from unit/spec/test.js file are converted into mocha tests. It can be found in unit/spec/mocha.test.js

Note:
In this PR i haven't deleted the test.js file as once other files are approved we can continue with deleting that file

Commands to Run the test:

npm run test-unit : This command runs the unit tests for the application to ensure that individual units of code (such as functions or methods) are working as expected.

npm run test-unit-watch : This command allows us to continuously run tests while we work on our code. If we change a test file or the code it tests, Mocha will automatically re-run the tests.

npm run test-unit-coverage : This command runs the unit tests with code coverage reporting using NYC, which is a tool for generating code coverage reports.

Test

I have done all the necessary test

Checked in both "Restricted" and "ServiceWorker" Everything is working fine.
Unit tests npm test no issue
End-to-end (e2e) tests-e2e-firefox-> All tests Passed.
extension versions with production code tested
Browser Test -> Microsoft Edge.

Screenshot for converted tests Passing

THEBOSS0369 · 2025-01-14T06:42:09Z

These tests are falling because of the changes done in init.js file, I am fixing that right now

Signed-off-by: THEBOSS0369 <[email protected]>

…ix-js into con-mocha-backup

Jaifroid · 2025-01-15T07:22:21Z

@THEBOSS0369 Thank you very much for this, that's great! I'll take a look as soon as I can. Looks like a great improvement. @audiodude maybe you have some comments?

Jaifroid · 2025-01-18T12:53:21Z

I've done some basic tests by switching to the PR/branch, running npm install, and npm run test-unit and test-unit-coverage. Congratulations, this is very impressive conversion work!

Just quickly, I noticed that running these commands create a bunch of temporary files in the Repo that have not been correctly ignored (see screenshot). If the coverage files are overwritten on each coverage run, then they can remain, but you should .gitignore the coverage folder. Regarding the .nyc_output folder, it should also be .gitignore'd, but these look like files with a PID, and so I imagine will be created every run and could become a nuisance in that folder. Ignoring the folder may not be enough. Please check if there is a way to prevent them being generated, or whether they get auto-cleaned up (which would be fine).

Jaifroid · 2025-01-18T12:53:35Z

I haven't yet reviewed the code itself.

THEBOSS0369 · 2025-01-18T13:22:16Z

@Jaifroid Sir the temp files that you are seeing is not from the code instead it is due to the command npm run test-unit-coverage which is present in package.json file. The json files are the report files. The report gets updated when we re run the command and there is a change in files.
I didn't thought of that it would install the files in the main repo itself. However if you want i can delete the command so that these files wouldn't get downloaded and once this pr passes all the ticks we can add the command in the docs so if anyone wants report they can directly copy paste the command. Let me know what you think

This is a blog for a reference https://zinserjan.github.io/mocha-webpack/docs/guides/code-coverage.html

Jaifroid · 2025-01-18T14:29:16Z

Yes, I was aware that these reports were from running the command, but we don't want them being accidentally pushed to the server by a dev, so the folders containing them should at least be added to .gitignore. I don't think it's necessary to remove the command to invoke the code coverage test. I noticed the coverage hadn't been fully set up, it doesn't list any files and coverage currently.

THEBOSS0369 · 2025-01-18T16:34:15Z

Yes, I was aware that these reports were from running the command, but we don't want them being accidentally pushed to the server by a dev, so the folders containing them should at least be added to .gitignore. I don't think it's necessary to remove the command to invoke the code coverage test. I noticed the coverage hadn't been fully set up, it doesn't list any files and coverage currently.

Got it sir i will do it, and yes right now it just shows the table's heading and empty value I wanted to confirm whether this feature is needed or not so i created as an example. Once conversion work of test is approved, I will make it working.

THEBOSS0369 · 2025-02-03T06:31:03Z

Hey @Jaifroid sir let me know what to do further in this pr, when you get some time.

Jaifroid · 2025-02-03T07:25:14Z

Thanks! I noticed fallback errors in the test run due to Fetch not being supported in the test environment (see screenshot):

To avoid this, we should add a test for 'Fetch' support in the files xzdec_wrapper.js and zstddec_wrapper.js. It is sufficient to add this in each file where we test for WebAssembly support:

if ('WebAssembly' in self && 'Fetch' in self) { (the new code is && 'Fetch' in self).

Add this specifically on these lines:

Jaifroid

@THEBOSS0369 It's looking good. Just some minor suggestions to clean up the output display.

Jaifroid · 2025-02-03T07:28:11Z

tests/unit/spec/mocha.test.js

+});
+
+describe('Environment', function () {
+    it('Mocha Test should be properly configured', function () {


I think it's otiose to write "should" in every test. More usual would be to write simply Configure Mocha Test. If it has a tick, then it passed, if it has a red cross, it failed.

Jaifroid · 2025-02-03T07:28:38Z

tests/unit/spec/mocha.test.js

+        expect('test').to.equal('test');
+    });
+
+    it('should have loaded archive files', function () {


Similarly here, simply Load archive files...

Jaifroid · 2025-02-03T07:28:59Z

tests/unit/spec/mocha.test.js

+});
+
+describe('Utils', function () {
+    it('should correctly read an IEEE_754 float from 4 bytes', function () {


Here: Correctly read an .....

Jaifroid · 2025-02-03T07:29:22Z

tests/unit/spec/mocha.test.js

+        expect(float).to.equal(-118.625);
+    });
+
+    it('should handle upper/lower case variations', function () {


Here Handle upper/lower case variation

Jaifroid · 2025-02-03T07:31:02Z

tests/unit/spec/mocha.test.js

+            { input: 'téléphone', expected: 'Téléphone', description: 'The first letter should be uppercase' },
+            { input: 'Paris', expected: 'paris', description: 'The first letter should be lowercase' },
+            { input: 'le Couvre-chef Est sur le porte-manteaux', expected: 'Le Couvre-Chef Est Sur Le Porte-Manteaux', description: 'The first letter of every word should be uppercase' },
+            { input: 'épée', expected: 'Épée', description: 'The first letter should be uppercase (with accent)' },
+            { input: '$￥€"«xριστός» †¡Ἀνέστη!"', expected: '$￥€"«Xριστός» †¡ἀνέστη!"', description: 'First non-punctuation/non-currency Unicode letter should be uppercase, second (with breath mark) lowercase' },
+            { input: 'Καλά Νερά Μαγνησία žižek', expected: 'ΚΑΛΆ ΝΕΡΆ ΜΑΓΝΗΣΊΑ ŽIŽEK', options: 'full', description: 'All Unicode letters should be uppercase' }


In these cases, it's OK to keep "should".

Jaifroid · 2025-02-03T07:31:26Z

tests/unit/spec/mocha.test.js

+        });
+    });
+
+    it('should remove parameters from URLs', function () {


Just Remove parameters from URLs

Jaifroid · 2025-02-03T07:32:37Z

tests/unit/spec/mocha.test.js

+});
+
+describe('ZIM initialization', function () {
+    it('should set archive as ready', function () {


For this and all other "should" below, just change to, e.g., Set archive as ready (etc.).

Signed-off-by: THEBOSS0369 <[email protected]>

THEBOSS0369 · 2025-02-03T12:33:35Z

Thanks! I noticed fallback errors in the test run due to Fetch not being supported in the test environment (see screenshot):

To avoid this, we should add a test for 'Fetch' support in the files xzdec_wrapper.js and zstddec_wrapper.js. It is sufficient to add this in each file where we test for WebAssembly support:

if ('WebAssembly' in self && 'Fetch' in self) { (the new code is && 'Fetch' in self).

Add this specifically on these lines:

https://github.com/kiwix/kiwix-js/blob/main/www/js/lib/xzdec_wrapper.js#L34

https://github.com/kiwix/kiwix-js/blob/main/www/js/lib/zstddec_wrapper.js#L 98

@Jaifroid Thanks you for this sir , this was such a help i was trying to fix this in files zstddec_asm.js and wasm and similar types of files and was just about to ask you about this. I have fixed it now and other typos as you suggested. please take a look

THEBOSS0369 · 2025-02-05T07:29:23Z

Progress status: I'm currently trying to fix npm run test-unit-coverage isse. Its taking longer than i expected it to plus my winter's holiday has ended 2 weeks ago so i am not getting enough time to focus only on fixing it but i will try to fix it till this weekend, if i ain't able to make it, i will probably won't add the feature in this pr. would that be fine sir? @Jaifroid

Jaifroid · 2025-02-05T08:57:37Z

Progress status: I'm currently trying to fix npm run test-unit-coverage isse. Its taking longer than i expected it to plus my winter's holiday has ended 2 weeks ago so i am not getting enough time to focus only on fixing it but i will try to fix it till this weekend, if i ain't able to make it, i will probably won't add the feature in this pr. would that be fine sir? @Jaifroid

Thanks for the update. See what you can do this weekend, and if it's too complicated, maybe you could just make it into a separate issue to "Add Unit test coverage".

Signed-off-by: THEBOSS0369 <[email protected]>

THEBOSS0369 · 2025-02-08T11:51:21Z

Hello @Jaifroid sir i have fixed it but its creating so many problems in other workflow of the repo's.

First is that the main test are failing if i add the functionality it shows error message in files like urlutils.js , settingstore.js... and when i fix them it disturb the main workflow
second is the loaders which is used for testing will most probably be removed in future which will most probably disturb the workflow of repo, and when i use the instead option showing in below ss, it does not suite with other things.

example is this

coverage report are showing but with error messages

I am not very confident with this so i am just commiting the code to show you. I am just confirming again that i should remove this functionality from this pr and go ahead with the creating an issue or should i work on it more.

Its just keep getting complicated & more complicated and i don't want to mess with the codebase. Let me know what to do thanks.

THEBOSS0369 · 2025-02-08T11:53:46Z

I will revert the recent commit once you have taken a look

Jaifroid · 2025-02-08T13:16:55Z

tests/unit/coverage-setup.cjs

@@ -0,0 +1,26 @@
+const { createInstrumenter } = require('istanbul-lib-instrument');
+const { resolve } = require('path');
+const { readFileSync } = require('fs');


@THEBOSS0369 We're not using CommonJS modules. You should only use the ES Module import statements.

Jaifroid · 2025-02-08T13:17:49Z

.babelrc

@@ -0,0 +1,4 @@
+{
+  "presets": ["@babel/preset-env"],
+  "plugins": ["@babel/plugin-transform-modules-commonjs"]


@THEBOSS0369 Don't transform modules to CJS format, that will lead to loads of issues.

Jaifroid · 2025-02-08T13:23:11Z

@THEBOSS0369 I didn't review the whole of the latest commit, but I immediately noticed you're using the wrong type of modules. We use ES6 Modules, with import statements, not CommonJS modules with require statements. If ever you are copying code from Stack Exchange that has require in it, then it won't work with our project. It took a lot of work to get rid of the old moduling system and modernize it to ES6!

Overall, my view is that you should revert the code coverage commit and make it into a separate issue. It's enough that you've converted our ancient tests to a modern format. We know the coverage is extremely poor currently, and we need to work on adding more tests for basic units of code. Being told on each run that our coverage is poor won't really help the situation, even though it's a good idea in principle! Many thanks!

This reverts commit 444e215.

THEBOSS0369 · 2025-02-08T13:57:10Z

@THEBOSS0369 I didn't review the whole of the latest commit, but I immediately noticed you're using the wrong type of modules. We use ES6 Modules, with import statements, not CommonJS modules with require statements. If ever you are copying code from Stack Exchange that has require in it, then it won't work with our project. It took a lot of work to get rid of the old moduling system and modernize it to ES6!

Overall, my view is that you should revert the code coverage commit and make it into a separate issue. It's enough that you've converted our ancient tests to a modern format. We know the coverage is extremely poor currently, and we need to work on adding more tests for basic units of code. Being told on each run that our coverage is poor won't really help the situation, even though it's a good idea in principle! Many thanks!

Got it sir i have reverted the changes and yess i have been searching out the internet to make it work from last 2 weeks as i have never worked on something like this before 😅🫠.
I think the work is done now so do i need to do anything else on this pr? if not then i will move to different issue.

Jaifroid · 2025-02-09T10:46:37Z

Thanks, @THEBOSS0369 🙏. I shall review ASAP.

THEBOSS0369 added 2 commits January 14, 2025 11:48

Added Files

eca978c

Merge branch 'main' into con-mocha-backup

d799c48

THEBOSS0369 added 3 commits January 14, 2025 13:10

Fixed the init file

e775e9c

Signed-off-by: THEBOSS0369 <[email protected]>

Merge branch 'con-mocha-backup' of https://github.com/THEBOSS0369/kiw…

6a0a5d9

…ix-js into con-mocha-backup

Trying again

8ccbdd5

Jaifroid assigned THEBOSS0369 Jan 15, 2025

Jaifroid added enhancement tests labels Jan 15, 2025

Jaifroid added this to the v4.2 milestone Jan 15, 2025

Jaifroid added build Code relating to building, publishing, or maintaining the app task and removed enhancement labels Jan 15, 2025

Merge branch 'main' into con-mocha-backup

3982fc8

Adding coverage folders in gitignore

7bcbe82

Jaifroid self-requested a review January 26, 2025 11:34

THEBOSS0369 added 2 commits January 26, 2025 17:07

Merge branch 'main' into con-mocha-backup

93b98a6

Merge branch 'main' into con-mocha-backup

01d35a6

Jaifroid requested changes Feb 3, 2025

View reviewed changes

Fixed fallback errors & some grammatical checks

2a5cb69

Signed-off-by: THEBOSS0369 <[email protected]>

Added unit test coverage

444e215

Signed-off-by: THEBOSS0369 <[email protected]>

Jaifroid reviewed Feb 8, 2025

View reviewed changes

Revert "Added unit test coverage"

7cba4e3

This reverts commit 444e215.

THEBOSS0369 mentioned this pull request Feb 9, 2025

Add Unit test coverage #1308

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Converted Q Unit tests to Mocha Tests #1301

Converted Q Unit tests to Mocha Tests #1301

THEBOSS0369 commented Jan 14, 2025 •

edited

Loading

THEBOSS0369 commented Jan 14, 2025

Jaifroid commented Jan 15, 2025

Jaifroid commented Jan 18, 2025

Jaifroid commented Jan 18, 2025

THEBOSS0369 commented Jan 18, 2025

Jaifroid commented Jan 18, 2025

THEBOSS0369 commented Jan 18, 2025

THEBOSS0369 commented Feb 3, 2025

Jaifroid commented Feb 3, 2025

Jaifroid left a comment

Jaifroid Feb 3, 2025

Jaifroid Feb 3, 2025

Jaifroid Feb 3, 2025

Jaifroid Feb 3, 2025

Jaifroid Feb 3, 2025

Jaifroid Feb 3, 2025

Jaifroid Feb 3, 2025

THEBOSS0369 commented Feb 3, 2025

THEBOSS0369 commented Feb 5, 2025

Jaifroid commented Feb 5, 2025

THEBOSS0369 commented Feb 8, 2025

THEBOSS0369 commented Feb 8, 2025 •

edited

Loading

Jaifroid Feb 8, 2025

Jaifroid Feb 8, 2025

Jaifroid commented Feb 8, 2025 •

edited

Loading

THEBOSS0369 commented Feb 8, 2025

Jaifroid commented Feb 9, 2025

Converted Q Unit tests to Mocha Tests #1301

Are you sure you want to change the base?

Converted Q Unit tests to Mocha Tests #1301

Conversation

THEBOSS0369 commented Jan 14, 2025 • edited Loading

Description

Tests Converted

Test

Screenshot for converted tests Passing

THEBOSS0369 commented Jan 14, 2025

Jaifroid commented Jan 15, 2025

Jaifroid commented Jan 18, 2025

Jaifroid commented Jan 18, 2025

THEBOSS0369 commented Jan 18, 2025

Jaifroid commented Jan 18, 2025

THEBOSS0369 commented Jan 18, 2025

THEBOSS0369 commented Feb 3, 2025

Jaifroid commented Feb 3, 2025

Jaifroid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

THEBOSS0369 commented Feb 3, 2025

THEBOSS0369 commented Feb 5, 2025

Jaifroid commented Feb 5, 2025

THEBOSS0369 commented Feb 8, 2025

THEBOSS0369 commented Feb 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jaifroid commented Feb 8, 2025 • edited Loading

THEBOSS0369 commented Feb 8, 2025

Jaifroid commented Feb 9, 2025

THEBOSS0369 commented Jan 14, 2025 •

edited

Loading

THEBOSS0369 commented Feb 8, 2025 •

edited

Loading

Jaifroid commented Feb 8, 2025 •

edited

Loading