Fixes of Broken CI #27

MohitMaliFtechiz · 2023-03-03T10:40:18Z

Since, We have changed the folder structure, generated files destination to build folder. That's why CI is failing on testing.

Fixed the paths according to new folder structure.
We have removed the Reader, Searcher in latest libkiwix, so in this PR we have refactored the test cases as well according to the latest libkiwix.

lib/build.gradle

MohitMaliFtechiz · 2023-03-14T05:46:50Z

@mgautierfr ,

Most of the test cases i have refactored and they are running successfully.
but in two test case i'm facing error.

 String s = getTextFileContent("small_zimfile_data/main.html");
        String c = archive.getEntryByPath("A/main.html").getItem(true).getData().getData();
        assertEquals(s, c);

when i'm comparing main page data of zim file. 2 methods i'm using
i) getTextFileContent() in this method we are getting fileContent as string.
ii) from Blob object which i have get from Archive class.

in both methods we have difference of spaces that's why test cases are failing on this.

when i'm getting Book from Library through library.getBookById(bookid) then it's giving compile time error

/media/hp-pc03/D/java-libkiwix/lib/src/test/../main/cpp/utils.h:71: void setPtr(JNIEnv*, jobject, std::shared_ptr<_Tp>&&, const char*) [with T = const kiwix::Book; JNIEnv = JNIEnv_; jobject = _jobject*]: Assertion `env->GetLongField(thisObj, fieldId) == 0' failed.

Edited

I'm adding new test cases for Searcher to test the search functionality when i'm creating the Query object to pass in searcher it's giving UnsatisfiedLinkError error. But i have include all .so and java files in test cases, am i missing something here? Can you please help me out from this.

java.lang.UnsatisfiedLinkError: 'long org.kiwix.libzim.Query.setNativeQuery(java.lang.String)'
	at org.kiwix.libzim.Query.setNativeQuery(Native Method)
	at org.kiwix.libzim.Query.<init>(Query.java:25)
	at test.testSearcher(test.java:196)

mgautierfr · 2023-03-14T12:38:13Z

in both methods we have difference of spaces that's why test cases are failing on this.

You have modified the spaces in small_zimfile_data/main.html (https://github.com/kiwix/java-libkiwix/pull/27/files#diff-8f490b9cc9fd6bb56c1ac0d26c4da20d9bab94d312b34f3953fce3ff1cafc5a5R1-R11)
So your test probably compare the modified main.html with what is the zim file (probably create with the unmodified file)

MohitMaliFtechiz · 2023-03-14T12:48:15Z

You have modified the spaces in small_zimfile_data/main.html (https://github.com/kiwix/java-libkiwix/pull/27/files#diff-8f490b9cc9fd6bb56c1ac0d26c4da20d9bab94d312b34f3953fce3ff1cafc5a5R1-R11)
So your test probably compare the modified main.html with what is the zim file (probably create with the unmodified file)

For now i have modify the the main.html to pass the test cases (to write new test cases and match 100% code coverage report), without modifying the main.html the spaces error is ouurcing.

mgautierfr · 2023-03-14T13:00:20Z

I've just tested small.zim and there is no extra space in the content. So I suspect the zim file has been created with a content without space.

I suggest you modify main.html has you have already done. But be sure to put this change in a specific commit with a commit message explicating why we do this change. Especially that we have tested the zim file itself and that the wrapper is not in cause here.

…re is no extra space in it.

MohitMaliFtechiz · 2023-03-15T05:46:14Z

Thanks, I have done the changes in main.html as you suggested.

when i'm getting Book from Library through library.getBookById(bookid) then it's giving compile time error
Edited
I'm adding new test cases for Searcher to test the search functionality when i'm creating the Query object to pass in searcher it's giving UnsatisfiedLinkError error. But i have include all .so and java files in test cases, am i missing something here? Can you please help me out from this.

can you please help me out from these error so i can move forward to right more new test cases for library and searcher.

kelson42 · 2023-03-15T14:42:04Z

@mgautierfr ?

lib/src/test/CMakeLists.txt

mgautierfr · 2023-03-15T15:17:54Z

I cannot compile the test on my computer :

./gradlew createCodeCoverageReport

> Task :lib:createCodeCoverageReport
-- JNI_INCLUDE_DIRS=/usr/lib/jvm/java/include;/usr/lib/jvm/java/include/linux;/usr/lib/jvm/java/include
-- JNI_LIBRARIES=/usr/lib/jvm/java/lib/libjawt.so;/usr/lib/jvm/java/lib/server/libjvm.so
-- Configuring done
-- Generating done
-- Build files have been written to: /home/mgautier/Project/KIWIX/JAVA/java-libkiwix2/lib/src/test
[  4%] Building CXX object CMakeFiles/buildkiwix.dir/home/mgautier/Project/KIWIX/JAVA/java-libkiwix2/lib/src/main/cpp/libkiwix/book.cpp.o
/home/mgautier/Project/KIWIX/JAVA/java-libkiwix2/lib/src/main/cpp/libkiwix/book.cpp:21:10: erreur fatale: jni.h : Aucun fichier ou dossier de ce type
   21 | #include <jni.h>
      |          ^~~~~~~
compilation terminée.
make[2]: *** [CMakeFiles/buildkiwix.dir/build.make:76 : CMakeFiles/buildkiwix.dir/home/mgautier/Project/KIWIX/JAVA/java-libkiwix2/lib/src/main/cpp/libkiwix/book.cpp.o] Erreur 1
make[1]: *** [CMakeFiles/Makefile2:83 : CMakeFiles/buildkiwix.dir/all] Erreur 2
make: *** [Makefile:91 : all] Erreur 2
Starting a Gradle Daemon, 1 busy and 2 stopped Daemons could not be reused, use --status for details
> Task :lib:copyBuildKiwixSoFile NO-SOURCE

BUILD SUCCESSFUL in 4s
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
JUnit version 4.13
Exception in thread "main" java.lang.UnsatisfiedLinkError: no buildkiwix in java.library.path: [/home/mgautier/Project/KIWIX/JAVA/java-libkiwix2/lib/build]
        at java.base/java.lang.ClassLoader.loadLibrary(ClassLoader.java:2673)
        at java.base/java.lang.Runtime.loadLibrary0(Runtime.java:830)
        at java.base/java.lang.System.loadLibrary(System.java:1873)
        at test.<clinit>(test.java:15)
        at java.base/java.lang.Class.forName0(Native Method)
        at java.base/java.lang.Class.forName(Class.java:398)
        at org.junit.internal.Classes.getClass(Classes.java:42)
        at org.junit.internal.Classes.getClass(Classes.java:27)
        at org.junit.runner.JUnitCommandLineParseResult.parseParameters(JUnitCommandLineParseResult.java:98)
        at org.junit.runner.JUnitCommandLineParseResult.parseArgs(JUnitCommandLineParseResult.java:50)
        at org.junit.runner.JUnitCommandLineParseResult.parse(JUnitCommandLineParseResult.java:44)
        at org.junit.runner.JUnitCore.runMain(JUnitCore.java:72)
        at org.junit.runner.JUnitCore.main(JUnitCore.java:36)
!!! ERROR: Unit test failed

> Task :lib:createCodeCoverageReport FAILED

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':lib:createCodeCoverageReport'.
> Process 'command 'sh'' finished with non-zero exit value 1

* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output. Run with --scan to get full insights.

* Get more help at https://help.gradle.org

BUILD FAILED in 5s
1 actionable task: 1 executed

MohitMaliFtechiz · 2023-03-16T05:32:48Z

@mgautierfr ,

Task :lib:createCodeCoverageReport
-- JNI_INCLUDE_DIRS=/usr/lib/jvm/java/include;/usr/lib/jvm/java/include/linux;/usr/lib/jvm/java/include
-- JNI_LIBRARIES=/usr/lib/jvm/java/lib/libjawt.so;/usr/lib/jvm/java/lib/server/libjvm.so

these are the JNI_INCLUDE_DIRS of your computer, After changing these in test CMakeLists.txt it'll compile on you computer.

/usr/lib/jvm/java/include
/usr/lib/jvm/java/include/linux

Edited
I have removed the absolute path of JNI_INCLUDE_DIRS , now it'll automatically picks the JNI_INCLUDE_DIRS .

@28

Fix @28

Bookmark wrapper can be created using two ways: - From a existing (cpp) bookmark (done internally by Library wrapper code) - As a totally new one (empty) the java code will have to setup (using `set*` methods). If the `Bookmark` constructor always create an empty new cpp bookmark, when we set the wrapper to point to the existing bookmark, we will have a leak of the new created bookmark. As we want to keep the "basic" constructor as the normal java api to create an empty bookmark, we need another (private) constructor to avoid the construction of an empty bookmark. The new constructor take a handle and directly set the `nativeHandle`. On `Library.getBookmarks` we cannot use the helper `BUILD_WRAPPER` and we must use "internal" function to use the `(J)V` constructor instead of the basic `()V`.

mgautierfr · 2023-03-20T13:54:08Z

@MohitMaliFtechiz I have fixed the cpp wrapping (as far as testing points to broken cpp).

However, I have tests failing about wrong indentation in expected html. I let you fix that.

… this commit i have removed that extra spaces to pass test cases

codecov-commenter · 2023-03-21T07:15:42Z

Codecov Report

❗ No coverage uploaded for pull request base (main@4b5cc44). Click here to learn what that means.
Patch has no changes to coverable lines.

Additional details and impacted files

@@           Coverage Diff           @@
##             main      #27   +/-   ##
=======================================
  Coverage        ?   38.20%           
  Complexity      ?       15           
=======================================
  Files           ?       24           
  Lines           ?       89           
  Branches        ?        6           
=======================================
  Hits            ?       34           
  Misses          ?       51           
  Partials        ?        4

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

kelson42 · 2023-03-21T21:01:27Z

I'm in favour of merging this ASAP. extending testing and code coverage can be done in furthet PRs.

MohitMaliFtechiz · 2023-03-22T05:47:11Z

I'm in favour of merging this ASAP. extending testing and code coverage can be done in furthet PRs.

@kelson42 Ok, I have create a ticket for adding more new test cases for latest wrapper #29 . @mgautierfr can you please review this PR.

MohitMaliFtechiz · 2023-03-22T09:16:26Z

when i'm getting Book from Library through library.getBookById(bookid) then it's giving compile time error
/media/hp-pc03/D/java-libkiwix/lib/src/test/../main/cpp/utils.h:71: void setPtr(JNIEnv*, jobject, std::shared_ptr<_Tp>&&, const char*) [with T = const kiwix::Book; JNIEnv = JNIEnv_; jobject = _jobject*]: Assertion `env->GetLongField(thisObj, fieldId) == 0' failed.
Edited

I'm adding new test cases for Searcher to test the search functionality when i'm creating the Query object to pass in searcher it's giving UnsatisfiedLinkError error. But i have include all .so and java files in test cases, am i missing something here? Can you please help me out from this.
java.lang.UnsatisfiedLinkError: 'long org.kiwix.libzim.Query.setNativeQuery(java.lang.String)'
	at org.kiwix.libzim.Query.setNativeQuery(Native Method)
	at org.kiwix.libzim.Query.<init>(Query.java:25)
	at test.testSearcher(test.java:196)

@mgautierfr , have you find something on these error?
Other Library functions are working fine but 2 methods are giving error first one is getBookById() which i have mentioned earlier and another one is getArchiveById() .

kelson42 · 2023-03-22T09:45:35Z

when i'm getting Book from Library through library.getBookById(bookid) then it's giving compile time error
/media/hp-pc03/D/java-libkiwix/lib/src/test/../main/cpp/utils.h:71: void setPtr(JNIEnv*, jobject, std::shared_ptr<_Tp>&&, const char*) [with T = const kiwix::Book; JNIEnv = JNIEnv_; jobject = _jobject*]: Assertion `env->GetLongField(thisObj, fieldId) == 0' failed.
Edited
I'm adding new test cases for Searcher to test the search functionality when i'm creating the Query object to pass in searcher it's giving UnsatisfiedLinkError error. But i have include all .so and java files in test cases, am i missing something here? Can you please help me out from this.
java.lang.UnsatisfiedLinkError: 'long org.kiwix.libzim.Query.setNativeQuery(java.lang.String)'
	at org.kiwix.libzim.Query.setNativeQuery(Native Method)
	at org.kiwix.libzim.Query.<init>(Query.java:25)
	at test.testSearcher(test.java:196)
@mgautierfr , have you find something on these error? Other Library functions are working fine but 2 methods are giving error first one is getBookById() which i have mentioned earlier and another one is getArchiveById() .

@MohitMaliDeveloper Why do we have a green CI if there an error? Please move everything which is not mandatory and still NOK to #30. This PR has to be ready to review = perfect.

MohitMaliFtechiz · 2023-03-22T09:56:04Z

@MohitMaliDeveloper Why do we have a green CI if there an error?

@kelson42 , we haven't included the failing test cases in this PR for writing new test cases we commented that part of code.

Please move everything which is not mandatory and still NOK to #30.

Okay, I'm removing the commented code from here and moving these test cases to #30 .

mgautierfr

Few things to change on the code itself.

More generally, we have to test the wrapper. Meanning we have to run all functions and check they are not broken. Testing the behavior (returned values) of such functions is nice but it is not what we want to test. The behavior of libkiwix is tested in libkiwix side.

The same way, you have fixed existing tests to make it pass with the new api. But it would be better to fully adapt the tests to the new api and not being stuck with the old logic.

(And see my answers to the discussion in the PR comments)

mgautierfr · 2023-03-21T18:09:33Z

lib/src/test/CMakeLists.txt

+SHARED
+${PROJECT_SOURCE_DIR}/../main/cpp/libkiwix/book.cpp
+${PROJECT_SOURCE_DIR}/../main/cpp/libkiwix/bookmark.cpp
+${PROJECT_SOURCE_DIR}/../main/cpp/libkiwix/filter.cpp
+${PROJECT_SOURCE_DIR}/../main/cpp/libkiwix/illustration.cpp
+${PROJECT_SOURCE_DIR}/../main/cpp/libkiwix/kiwixicu.cpp
+${PROJECT_SOURCE_DIR}/../main/cpp/libkiwix/kiwixserver.cpp
+${PROJECT_SOURCE_DIR}/../main/cpp/libkiwix/library.cpp
+${PROJECT_SOURCE_DIR}/../main/cpp/libkiwix/manager.cpp
+${PROJECT_SOURCE_DIR}/../main/cpp/libzim/archive.cpp
+${PROJECT_SOURCE_DIR}/../main/cpp/libzim/blob.cpp
+${PROJECT_SOURCE_DIR}/../main/cpp/libzim/entry.cpp
+${PROJECT_SOURCE_DIR}/../main/cpp/libzim/entry_iterator.cpp
+${PROJECT_SOURCE_DIR}/../main/cpp/libzim/item.cpp
+${PROJECT_SOURCE_DIR}/../main/cpp/libzim/query.cpp
+${PROJECT_SOURCE_DIR}/../main/cpp/libzim/search.cpp
+${PROJECT_SOURCE_DIR}/../main/cpp/libzim/search_iterator.cpp
+${PROJECT_SOURCE_DIR}/../main/cpp/libzim/searcher.cpp
+${PROJECT_SOURCE_DIR}/../main/cpp/libzim/suggestion_item.cpp
+${PROJECT_SOURCE_DIR}/../main/cpp/libzim/suggestion_iterator.cpp
+${PROJECT_SOURCE_DIR}/../main/cpp/libzim/suggestion_search.cpp
+${PROJECT_SOURCE_DIR}/../main/cpp/libzim/suggestion_searcher.cpp
+${PROJECT_SOURCE_DIR}/../main/cpp/utils.h


Tests should not recompile the wrapper.
It should use the already compiled wrapper (with build target) as kiwix-android will use it.

We are not running test cases on android architecture its linux variant upon which test needs to run .

lib/src/test/test.java

mgautierfr · 2023-03-22T10:05:28Z

@mgautierfr , have you find something on these error?

I have fixed all cpp errors I can found by running the test on my computer (and the CI). As @kelson42 said, the tests must NOT pass if there is error. This is the purpose of tests.

However, I agree with @kelson42, if this pr is good enough to have tests running (and it is), we can merge it right now and fix things later.

MohitMaliFtechiz · 2023-03-22T11:31:25Z

Few things to change on the code itself.

More generally, we have to test the wrapper. Meanning we have to run all functions and check they are not broken. Testing the behavior (returned values) of such functions is nice but it is not what we want to test. The behavior of libkiwix is tested in libkiwix side.

The same way, you have fixed existing tests to make it pass with the new api. But it would be better to fully adapt the tests to the new api and not being stuck with the old logic.

(And see my answers to the discussion in the PR comments)

Let me conclude what i understood by your comment , We have to test the wrapper changes functionality on android side so after building the wrapper , we have compile kiwix-android replace the .aar file and run the test as we are running it for android by this way we can ensure wrapper is working fine. let me know if this is making sense.

mgautierfr · 2023-03-22T12:16:40Z

Let me conclude what i understood by your comment , We have to test the wrapper changes functionality on android side so after building the wrapper , we have compile kiwix-android replace the .aar file and run the test as we are running it for android by this way we can ensure wrapper is working fine. let me know if this is making sense.

Not exactly.
kiwix-android will use a precompiled java-libkiwix .aar file. It will not compile it nor include the wrapper source in it own build scripts. It will just "import" libkiwix and use it.
The testing of java-libkiwix must do the same. We build the java-libkiwix aar with the gradle build target and the testing use that aar. We may make the test (or generateCodecoverage) target depends of build target is testing depends of build. But the testing build script itself must not compile the wrapper or include the cpp sources.

mgautierfr · 2023-03-22T16:49:38Z

We have merged this PR as it is technically working. But few things still have to be improved. I have open the issue #31 for that.

MohitMaliDeveloper added 4 commits March 3, 2023 16:08

Include new wrapper files in test CmakeLists file

49de5c4

Removed wrapper.app from testing

e14a123

Fix path of compiled so file

f7e4a5b

Fix path according to new folder structure

9b7067d

mgautierfr reviewed Mar 6, 2023

View reviewed changes

lib/build.gradle Outdated Show resolved Hide resolved

MohitMaliDeveloper added 2 commits March 6, 2023 15:47

Reverting minSdk,targetSdk version

37618f2

Fix cpp error while compiling

f427d53

mgautierfr mentioned this pull request Mar 8, 2023

[WIP] Working on test.. #26

Closed

MohitMaliFtechiz marked this pull request as draft March 9, 2023 09:45

MohitMaliDeveloper added 3 commits March 10, 2023 16:37

Refactored test cases

38043b0

Fixed jni directory in test cases

306916f

Fixed generated jar path

9d962a3

MohitMaliFtechiz force-pushed the FixBrokenCi branch from cb30ff0 to 9d962a3 Compare March 13, 2023 13:10

MohitMaliDeveloper added 2 commits March 13, 2023 18:55

Fixed jacoco report path

45f3e86

Re-enable failed test cases

7d07392

MohitMaliFtechiz mentioned this pull request Mar 14, 2023

addBookmark function of Library class is not saving the Bookmark in latest libkiwix #28

Closed

MohitMaliDeveloper added 4 commits March 14, 2023 18:52

Added test cases for libkiwix package

09539e2

Removed extra spaces from main.html, we have tested small.zim and the…

9a2573e

…re is no extra space in it.

Added searcher test cases

97f2b4d

Add book to library for bookmark test

441a154

MohitMaliFtechiz force-pushed the FixBrokenCi branch from f009122 to 441a154 Compare March 14, 2023 13:30

mgautierfr reviewed Mar 15, 2023

View reviewed changes

lib/src/test/CMakeLists.txt Outdated Show resolved Hide resolved

Removed absolute path of JNI_INCLUDE_DIR

c5bf83a

MohitMaliFtechiz force-pushed the FixBrokenCi branch from 088cf82 to c5bf83a Compare March 16, 2023 05:56

mgautierfr added 2 commits March 20, 2023 14:28

Fix typo in Library.getBookmarks

3073712

Fix @28

main.html has extra spaces, which small.zim file not containing so in…

ee9a47d

… this commit i have removed that extra spaces to pass test cases

MohitMaliFtechiz added 2 commits March 21, 2023 18:21

Remove bookmark test cases added

57dd239

Added new test cases for bookmark

4ee1e7f

MohitMaliFtechiz mentioned this pull request Mar 22, 2023

Add new test cases for latest wrapper #29

Closed

MohitMaliFtechiz marked this pull request as ready for review March 22, 2023 05:47

MohitMaliFtechiz requested a review from mgautierfr March 22, 2023 05:48

kelson42 mentioned this pull request Mar 22, 2023

Added new test cases for latest wrapper #30

Merged

Removed commented code

33dd97d

mgautierfr requested changes Mar 22, 2023

View reviewed changes

Changes after review

160428d

MohitMaliFtechiz requested a review from mgautierfr March 22, 2023 12:10

kelson42 merged commit 66ebc4e into main Mar 22, 2023

kelson42 deleted the FixBrokenCi branch March 22, 2023 16:32

mgautierfr mentioned this pull request Mar 22, 2023

Reorganise the gradle build script #31

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes of Broken CI #27

Fixes of Broken CI #27

MohitMaliFtechiz commented Mar 3, 2023 •

edited

MohitMaliFtechiz commented Mar 14, 2023 •

edited

mgautierfr commented Mar 14, 2023

MohitMaliFtechiz commented Mar 14, 2023

mgautierfr commented Mar 14, 2023

MohitMaliFtechiz commented Mar 15, 2023

kelson42 commented Mar 15, 2023

mgautierfr commented Mar 15, 2023

MohitMaliFtechiz commented Mar 16, 2023 •

edited

mgautierfr commented Mar 20, 2023

codecov-commenter commented Mar 21, 2023

kelson42 commented Mar 21, 2023

MohitMaliFtechiz commented Mar 22, 2023

MohitMaliFtechiz commented Mar 22, 2023

kelson42 commented Mar 22, 2023 •

edited

MohitMaliFtechiz commented Mar 22, 2023

mgautierfr left a comment

mgautierfr Mar 21, 2023

MohitMaliFtechiz Mar 22, 2023

mgautierfr commented Mar 22, 2023

MohitMaliFtechiz commented Mar 22, 2023

mgautierfr commented Mar 22, 2023

mgautierfr commented Mar 22, 2023

Fixes of Broken CI #27

Fixes of Broken CI #27

Conversation

MohitMaliFtechiz commented Mar 3, 2023 • edited

MohitMaliFtechiz commented Mar 14, 2023 • edited

mgautierfr commented Mar 14, 2023

MohitMaliFtechiz commented Mar 14, 2023

mgautierfr commented Mar 14, 2023

MohitMaliFtechiz commented Mar 15, 2023

kelson42 commented Mar 15, 2023

mgautierfr commented Mar 15, 2023

MohitMaliFtechiz commented Mar 16, 2023 • edited

mgautierfr commented Mar 20, 2023

codecov-commenter commented Mar 21, 2023

Codecov Report

kelson42 commented Mar 21, 2023

MohitMaliFtechiz commented Mar 22, 2023

MohitMaliFtechiz commented Mar 22, 2023

kelson42 commented Mar 22, 2023 • edited

MohitMaliFtechiz commented Mar 22, 2023

mgautierfr left a comment

Choose a reason for hiding this comment

mgautierfr Mar 21, 2023

Choose a reason for hiding this comment

MohitMaliFtechiz Mar 22, 2023

Choose a reason for hiding this comment

mgautierfr commented Mar 22, 2023

MohitMaliFtechiz commented Mar 22, 2023

mgautierfr commented Mar 22, 2023

mgautierfr commented Mar 22, 2023

MohitMaliFtechiz commented Mar 3, 2023 •

edited

MohitMaliFtechiz commented Mar 14, 2023 •

edited

MohitMaliFtechiz commented Mar 16, 2023 •

edited

kelson42 commented Mar 22, 2023 •

edited