search

kiwix / java-libkiwix

Libkiwix binding for Java & Kotlin
https://central.sonatype.com/artifact/org.kiwix/libkiwix
GNU General Public License v3.0
3 stars 4 forks source link

Fixes of Broken CI #27

Closed MohitMaliFtechiz closed 1 year ago

MohitMaliFtechiz commented 1 year ago

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

MohitMaliFtechiz commented 1 year ago

@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);

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

Screenshot from 2023-03-10 16-17-59

/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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.

MohitMaliFtechiz commented 1 year ago

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 commented 1 year ago

@mgautierfr ?

mgautierfr commented 1 year ago

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 commented 1 year ago

@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 .

mgautierfr commented 1 year ago

@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.

codecov-commenter commented 1 year ago

Codecov Report

:exclamation: 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 ```diff @@ 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](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kiwix). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kiwix)

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

kelson42 commented 1 year ago

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

MohitMaliFtechiz commented 1 year ago

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 https://github.com/kiwix/java-libkiwix/issues/29 . @mgautierfr can you please review this PR.

MohitMaliFtechiz commented 1 year ago
  • 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 commented 1 year ago
  • 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 commented 1 year ago

@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 https://github.com/kiwix/java-libkiwix/pull/30.

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

mgautierfr commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.