search

museumsvictoria / nodel

A digital media control system for museums and galleries
nodel.io
60 stars 17 forks source link

feature: Introduce unit testing to framework #314

Closed scroix closed 4 months ago

scroix commented 5 months ago

The thinking behind this PR comes from a few places.

I think one of the big limiting factors is the lack of testing. It's rather difficult to just "try" every feature of Nodel and hope nothing breaks before you're comfortable to merge. It's also rather difficult for @justparking to assess the implications of absolutely every PR (although he does a fantastic job!).

Here is a little attempt to break the ice by introducing some basic unit tests using JUnit and Mockito. In order to demonstrate the usefulness, I've began by targeting the changes described in #284 and implemented in feat-standardise-search.

Some of the tests, which are currently @Disabled will intentionally fail on the master branch, but pass on this new proposed feature branch. Thus, hopefully, showing the value of these simple unit tests.

scroix commented 5 months ago

@justparking I've just addressed the issue where tests would fail when building with Java 8 due to a incompatibility with one of the compiled test dependencies as made apparent by the warning: Launch has been compiled by a more recent version of the Java Runtime (class file version 55.0), this version of the Java Runtime only recognizes class file versions up to 52.0 message.

justparking commented 5 months ago

I see you had to reduce the ch.qos.logback:logback-classic dependency version to 1.2.3.

I thought I might be able to avoid that by allowing the compiler to use Java 11 but forcing it to produce Java 8 (1.8) compatible binaries. I've done that in the past using these options:

compileJava {
    sourceCompatibility = '1.8'
    targetCompatibility = '1.8'
}

That caused an obscure "missing method" exception at runtime, almost identical to this - bytebuffer-and-the-dreaded-nosuchmethoderror.

It references this as a solution:

compileJava {
  options.release = 8
}

But unfortunately that failed too because of a gradle version incompatibility.

I'm going to just leave it as you left it.

I think we're going to have to deal with leaving Java 8 altogether as a separate, longer term issue worthy of a separate discussion. Unfortunately swapping out a JAR is far easier that swapping out a JAR and JVM.

justparking commented 5 months ago

Actually, I believe the compileJava { options.release = 8 } option is compatible with gradle 7.6.4.

scroix commented 4 months ago

I think we're going to have to deal with leaving Java 8 altogether as a separate, longer term issue worthy of a separate discussion.

I agree. I've got no qualms just opting for an older dependency version in order to break the ice on introducing these unit tests. The difficulties encountered here are a good reminder of why we're still using Java 8, and that there is indeed a larger task at hand to move to Java 11+

By rolling back ch.qos.logback:logback-classic I'm able to build this branch with both Java 8 and 11 👍

justparking commented 4 months ago

Compiling with 11 is something worthwhile pursuing since it keeps pace with the rest of the tooling and IDEs, etc.

I stalled last night because your "tests" branch seemed to override updates from your "gradle" updates branch and it got too late to apply a manual patch.

I'm keen to try it i.e. your "tests" with the "release=8", latest dep3ndencies and your gradle bump.

scroix commented 4 months ago

your "tests" branch seemed to override updates from your "gradle" updates branch

I'd forked this branch from #313 but then did a wonky rebase in anticipation of having the Gradle changes rejected.

I've just gone and rebased to when I created this branch from #313. This should compile with both Java 8 and Java 11. Note, I did downgrade to ch.qos.logback:logback-classic:1.2.3. I did experiment with the "release=8" options you described but found myself staring down a rabbit hole and opted to keep this PR focused on just introducing the unit tests for now.

justparking commented 4 months ago

Hey @scroix - I managed to get it to compile with Java 11 with restore the ch.qos.logback:logback-classic:1.4.14'dependency.

This was the patch:

diff --git a/nodel-framework/build.gradle b/nodel-framework/build.gradle
index edb0f27..493f9dd 100644
--- a/nodel-framework/build.gradle
+++ b/nodel-framework/build.gradle
@@ -16,6 +16,10 @@ jar {
     }
 }

+compileJava {
+  options.release = 8
+}
+
 test {
     useJUnitPlatform()
     testLogging {
@@ -35,6 +39,6 @@ dependencies {
     testImplementation 'org.junit.jupiter:junit-jupiter-params:5.7.0'
     testRuntimeOnly 'org.junit.jupiter:junit-jupiter-engine:5.7.0'
     testImplementation 'org.mockito:mockito-core:3.+'
-    testImplementation 'ch.qos.logback:logback-classic:1.2.3'
+    testImplementation 'ch.qos.logback:logback-classic:1.4.14'
 }

diff --git a/nodel-jyhost/build.gradle b/nodel-jyhost/build.gradle
index b4265d5..1209977 100644
--- a/nodel-jyhost/build.gradle
+++ b/nodel-jyhost/build.gradle
@@ -13,6 +13,10 @@ application {
     mainClass = 'org.nodel.jyhost.Launch'
 }

+compileJava {
+  options.release = 8
+}
+
 jar {
     from "$buildDir/output"
     archiveBaseName = 'nodel-jyhost'
diff --git a/nodel-webui-js/build.gradle b/nodel-webui-js/build.gradle
index 86b893b..ad2bc76 100644
--- a/nodel-webui-js/build.gradle
+++ b/nodel-webui-js/build.gradle
@@ -17,6 +17,10 @@ plugins {
     id 'com.github.node-gradle.node' version '7.0.2'
 }

+compileJava {
+  options.release = 8
+}
+
 repositories {
     mavenCentral()
 }

H:\nodel-official>java -version
openjdk version "1.8.0_412"
OpenJDK Runtime Environment Corretto-8.412.08.1 (build 1.8.0_412-b08)
OpenJDK 64-Bit Server VM Corretto-8.412.08.1 (build 25.412-b08, mixed mode)

H:\nodel-official>java -jar nodel-jyhost\build\distributions\standalone\nodelhost-dev-2.2.1-rev565.jar
Nodel [Jython] vunspecified-dev_r565 is running.

Press Enter to initiate a shutdown.

    (web interface available at http://192.168.1.116:8085)
H:\nodel-official>java -version
openjdk version "11.0.23" 2024-04-16 LTS
OpenJDK Runtime Environment Corretto-11.0.23.9.1 (build 11.0.23+9-LTS)
OpenJDK 64-Bit Server VM Corretto-11.0.23.9.1 (build 11.0.23+9-LTS, mixed mode)

H:\nodel-official>java -jar nodel-jyhost\build\distributions\standalone\nodelhost-dev-2.2.1-rev565.jar
WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by org.python.google.common.base.internal.Finalizer (file:/H:/nodel-official/nodel-jyhost/build/distributions/standalone/nodelhost-dev-2.2.1-rev565.jar) to field java.lang.Thread.inheritableThreadLocals
WARNING: Please consider reporting this to the maintainers of org.python.google.common.base.internal.Finalizer
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release
Nodel [Jython] vunspecified-dev_r565 is running.

Press Enter to initiate a shutdown.

    (web interface available at http://192.168.1.116:8085)

The only thing I notice now is the version it's reporting vunspecified-dev_r565.

I presume that's related to the gradle update?

scroix commented 4 months ago

I notice now is the version it's reporting vunspecified-dev_r565.

I've found the issue and just applied a fix.

You should now get nodelhost-introduce-junit-testing-2.2.1-rev564.jar building from my branch and see Nodel [Jython] v2.2.1-introduce-junit-testing_r564 is running. in the console when running it.

Or, were this built from the master branch, you'll see Nodel [Jython] v2.2.1-release_r564 is running. or dev branch, and you'll see Nodel [Jython] v2.2.1-dev_r565 is running. and so forth.