Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feature: Introduce unit testing to framework #314

Merged
merged 58 commits into from
May 3, 2024

Conversation

scroix
Copy link
Member

@scroix scroix commented Apr 6, 2024

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 and others added 30 commits March 18, 2024 21:13
- Migrate build scripts to be compatible
- Bump Node.js version to one which supports ARM/Silicon builds
- Configure the node-gradle-plugin
- Using `gradle wrapper` command
- 'default' includes googlefonts build, but the 'build' command does not
- This tackles a tiny regression which saw us lose -dev- handled in the generated .jar filename
- We also handle the odd case of building in a git branch which includes a forward slash
- Configure Gradle build to include JUnit dependencies
- Cover SimpleNameTest with a couple of test cases
- Ensure they pass
@justparking
Copy link
Contributor

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

@scroix
Copy link
Member Author

scroix commented Apr 20, 2024

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
Copy link
Contributor

justparking commented Apr 20, 2024

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.

- Configure Gradle build to include JUnit dependencies
- Cover SimpleNameTest with a couple of test cases
- Ensure they pass
… Actions and Events

- These are in preparation for changes related to issue museumsvictoria#284
- Configure Gradle build to include JUnit dependencies
- Cover SimpleNameTest with a couple of test cases
- Ensure they pass
…odel into feature/introduce-junit-testing

# Conflicts:
#	nodel-framework/build.gradle
#	nodel-framework/src/test/java/org/nodel/SimpleNameTest.java
…scroix/nodel into feature/introduce-junit-testing"

This reverts commit 50dfbcb, reversing
changes made to b1a73ad.
@scroix
Copy link
Member Author

scroix commented Apr 20, 2024

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
Copy link
Contributor

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
Copy link
Member Author

scroix commented Apr 24, 2024

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.

scroix added a commit to scroix/nodel that referenced this pull request Apr 24, 2024
@scroix scroix changed the base branch from master to dev April 25, 2024 01:07
@scroix scroix merged commit e64d170 into museumsvictoria:dev May 3, 2024
@scroix scroix deleted the feature/introduce-junit-testing branch May 3, 2024 10:54
@scroix scroix restored the feature/introduce-junit-testing branch May 3, 2024 10:55
@scroix scroix deleted the feature/introduce-junit-testing branch May 3, 2024 10:55
scroix added a commit to scroix/nodel that referenced this pull request May 3, 2024
commit 3c3bd3e
Author: scroix <[email protected]>
Date:   Wed Apr 24 20:38:13 2024 +1000

    Fix broken versioning of nodehost builds

    - museumsvictoria#314 (comment)

commit a2a27fd
Author: scroix <[email protected]>
Date:   Wed Apr 24 19:29:40 2024 +1000

    Apply Java 11 compile patch from @justparking

commit 1d9d4ad
Author: scroix <[email protected]>
Date:   Wed Apr 24 19:18:20 2024 +1000

    Revert "Revert to Java 8 compatible logback dependency"

    This reverts commit 1b9e15b.

commit 1b9e15b
Author: scroix <[email protected]>
Date:   Fri Apr 19 18:46:43 2024 +1000

    Revert to Java 8 compatible logback dependency

commit ca5bc00
Author: scroix <[email protected]>
Date:   Sat Apr 20 12:19:25 2024 +1000

    Revert "Merge branch 'feature/introduce-junit-testing' of github.com:scroix/nodel into feature/introduce-junit-testing"

    This reverts commit 50dfbcb, reversing
    changes made to b1a73ad.

commit 50dfbcb
Merge: b1a73ad d9830bc
Author: scroix <[email protected]>
Date:   Sat Apr 20 12:15:05 2024 +1000

    Merge branch 'feature/introduce-junit-testing' of github.com:scroix/nodel into feature/introduce-junit-testing

    # Conflicts:
    #	nodel-framework/build.gradle
    #	nodel-framework/src/test/java/org/nodel/SimpleNameTest.java

commit b1a73ad
Author: scroix <[email protected]>
Date:   Thu Apr 4 17:42:25 2024 +1100

    Add first ever JUnit tests

    - Cover SimpleNameTest with a couple of test cases
    - Ensure they pass

commit 9a0ebd2
Author: scroix <[email protected]>
Date:   Thu Apr 4 17:41:21 2024 +1100

    Add JUnit as a testing framework

    - Configure Gradle build to include JUnit dependencies

commit 1198db3
Author: scroix <[email protected]>
Date:   Fri Apr 5 09:17:53 2024 +1100

    Introduce test cases to cover expected names of dynamically generated Actions and Events

    - These are in preparation for changes related to issue museumsvictoria#284

commit da01834
Author: scroix <[email protected]>
Date:   Fri Apr 5 09:16:42 2024 +1100

    Add GitHubIssue annotation to mark up tests

commit a1c0e13
Author: scroix <[email protected]>
Date:   Fri Apr 5 09:15:51 2024 +1100

    minor: Remove public declaration from SimpleNameTest

commit c04baf2
Author: scroix <[email protected]>
Date:   Fri Apr 5 09:14:50 2024 +1100

    Add Mockito and Logback test dependencies

commit 2daa92b
Author: scroix <[email protected]>
Date:   Thu Apr 4 17:42:25 2024 +1100

    Add first ever JUnit tests

    - Cover SimpleNameTest with a couple of test cases
    - Ensure they pass

commit 14b7033
Author: scroix <[email protected]>
Date:   Thu Apr 4 17:41:21 2024 +1100

    Add JUnit as a testing framework

    - Configure Gradle build to include JUnit dependencies

commit d9830bc
Author: scroix <[email protected]>
Date:   Fri Apr 19 18:46:43 2024 +1000

    Revert to Java 8 compatible logback dependency

commit e74f409
Author: scroix <[email protected]>
Date:   Sat Apr 6 16:11:43 2024 +1100

    Revert to original parent build file

commit 128dbe2
Author: scroix <[email protected]>
Date:   Sat Apr 6 16:10:18 2024 +1100

    Re-add Gradle job for running tests

commit 6a0ed30
Author: scroix <[email protected]>
Date:   Sat Apr 6 16:08:20 2024 +1100

    Revert "minor: ignore IntelliJ IDE files"

    This reverts commit 9a23d63.

commit 08e99fe
Author: scroix <[email protected]>
Date:   Sat Apr 6 16:04:59 2024 +1100

    Manually 'rebase' in preparation for PR

commit 2ff6dda
Merge: e386b21 1440a78
Author: Julien de-Sainte-Croix <[email protected]>
Date:   Sat Apr 6 15:21:28 2024 +1100

    Merge branch 'feature/introduce-junit-testing' of github.com:scroix/nodel into feature/introduce-junit-testing

    # Conflicts:
    #	nodel-framework/build.gradle

commit e386b21
Author: scroix <[email protected]>
Date:   Fri Apr 5 09:17:53 2024 +1100

    Introduce test cases to cover expected names of dynamically generated Actions and Events

    - These are in preparation for changes related to issue museumsvictoria#284

commit b67c120
Author: scroix <[email protected]>
Date:   Fri Apr 5 09:16:42 2024 +1100

    Add GitHubIssue annotation to mark up tests

commit 9d003b7
Author: scroix <[email protected]>
Date:   Fri Apr 5 09:15:51 2024 +1100

    minor: Remove public declaration from SimpleNameTest

commit 5ebcf19
Author: scroix <[email protected]>
Date:   Fri Apr 5 09:14:50 2024 +1100

    Add Mockito and Logback test dependencies

commit 7586d24
Author: scroix <[email protected]>
Date:   Thu Apr 4 17:42:25 2024 +1100

    Add first ever JUnit tests

    - Cover SimpleNameTest with a couple of test cases
    - Ensure they pass

commit e7b7e3e
Author: scroix <[email protected]>
Date:   Thu Apr 4 17:41:21 2024 +1100

    Add JUnit as a testing framework

    - Configure Gradle build to include JUnit dependencies

commit 9a23d63
Author: scroix <[email protected]>
Date:   Thu Apr 4 17:40:05 2024 +1100

    minor: ignore IntelliJ IDE files

commit 1440a78
Author: scroix <[email protected]>
Date:   Fri Apr 5 09:17:53 2024 +1100

    Introduce test cases to cover expected names of dynamically generated Actions and Events

    - These are in preparation for changes related to issue museumsvictoria#284

commit ac56791
Author: scroix <[email protected]>
Date:   Fri Apr 5 09:16:42 2024 +1100

    Add GitHubIssue annotation to mark up tests

commit 9e53135
Author: scroix <[email protected]>
Date:   Fri Apr 5 09:15:51 2024 +1100

    minor: Remove public declaration from SimpleNameTest

commit a37d107
Author: scroix <[email protected]>
Date:   Fri Apr 5 09:14:50 2024 +1100

    Add Mockito and Logback test dependencies

commit 6114356
Author: scroix <[email protected]>
Date:   Thu Apr 4 17:42:25 2024 +1100

    Add first ever JUnit tests

    - Cover SimpleNameTest with a couple of test cases
    - Ensure they pass

commit 9328669
Author: scroix <[email protected]>
Date:   Thu Apr 4 17:41:21 2024 +1100

    Add JUnit as a testing framework

    - Configure Gradle build to include JUnit dependencies

commit 36a59f3
Author: scroix <[email protected]>
Date:   Thu Apr 4 17:40:05 2024 +1100

    minor: ignore IntelliJ IDE files

commit de67ed4
Author: scroix <[email protected]>
Date:   Thu Apr 4 14:51:47 2024 +1100

    Rename branch in support of .jar generation

    - This tackles a tiny regression which saw us lose -dev- handled in the generated .jar filename
    - We also handle the odd case of building in a git branch which includes a forward slash

commit 138c477
Author: scroix <[email protected]>
Date:   Thu Apr 4 07:29:27 2024 +1100

    Stop targetting specific Gruntfile command

    - 'default' includes googlefonts build, but the 'build' command does not

commit 997b677
Author: scroix <[email protected]>
Date:   Sat Mar 30 13:46:36 2024 +1100

    Revert "Update gradle-wrapper.jar"

    This reverts commit 058a407.

commit 058a407
Author: scroix <[email protected]>
Date:   Sat Mar 30 13:46:17 2024 +1100

    Update gradle-wrapper.jar

commit d23abeb
Author: scroix <[email protected]>
Date:   Sat Mar 30 13:41:50 2024 +1100

    Switch to Gradle bin

commit 90c5066
Author: scroix <[email protected]>
Date:   Sat Mar 30 13:36:28 2024 +1100

    Update gradlew files

    - Using `gradle wrapper` command

commit 9018716
Author: scroix <[email protected]>
Date:   Sat Mar 30 13:24:58 2024 +1100

    Move project info variables to top-level build file

commit 0cfb6fd
Author: scroix <[email protected]>
Date:   Sat Mar 30 13:24:33 2024 +1100

    Revert to original devDependencies

commit 162d58d
Author: scroix <[email protected]>
Date:   Sat Mar 30 12:03:12 2024 +1100

    Revert node version to simplify grunt issue

commit 1b0b706
Author: scroix <[email protected]>
Date:   Sat Mar 30 08:13:58 2024 +1100

    Add a couple more optimisations

commit d65be71
Author: scroix <[email protected]>
Date:   Sat Mar 30 07:42:54 2024 +1100

    Move the feature to check for file changes into a class

commit 027a129
Author: scroix <[email protected]>
Date:   Sat Mar 30 07:18:03 2024 +1100

    Remove unneccesary dir check during Gradle webui tasks

commit 1a766c2
Author: scroix <[email protected]>
Date:   Sat Mar 30 07:17:27 2024 +1100

    Remove Gradle optimisation flags

commit 2fd735b
Author: Julien de-Sainte-Croix <[email protected]>
Date:   Fri Mar 29 22:26:53 2024 +1100

    Maybe this is working now

commit 820dba9
Author: Julien de-Sainte-Croix <[email protected]>
Date:   Fri Mar 29 21:57:48 2024 +1100

    Remove the grunt script from the package.json

commit 3e66086
Author: Julien de-Sainte-Croix <[email protected]>
Date:   Fri Mar 29 21:57:37 2024 +1100

    Further attempt to cache the grunt task

commit 3121adf
Author: Julien de-Sainte-Croix <[email protected]>
Date:   Fri Mar 29 20:51:49 2024 +1100

    Add hack for "cache" of grunt tasks

commit 48c0b59
Author: scroix <[email protected]>
Date:   Fri Mar 29 19:43:22 2024 +1100

    Attempt to cache grunt

commit 8a93e80
Author: scroix <[email protected]>
Date:   Fri Mar 29 19:42:47 2024 +1100

    Enable some speed-ups

commit d151cda
Author: scroix <[email protected]>
Date:   Fri Mar 29 16:29:48 2024 +1100

    Bump node-gradle plugin version to 7.0.2

commit 2c8f6f5
Author: scroix <[email protected]>
Date:   Fri Mar 29 16:29:22 2024 +1100

    Resolve minor warnings

commit 0fcc59e
Author: scroix <[email protected]>
Date:   Fri Mar 29 16:28:48 2024 +1100

    Replace deprecated jcenter

commit c10d6ef
Author: Julien de-Sainte-Croix <[email protected]>
Date:   Fri Mar 29 15:10:53 2024 +1100

    Bump Gradle version to 7.6.4

commit 9e05637
Author: Julien de-Sainte-Croix <[email protected]>
Date:   Mon Mar 18 21:13:46 2024 +1100

    Upgrade Gradle to 7.4 and Node.js to 16.x

    - Migrate build scripts to be compatible
    - Bump Node.js version to one which supports ARM/Silicon builds
    - Configure the node-gradle-plugin
justparking pushed a commit that referenced this pull request May 4, 2024
- Java 11 now required for building
- compilation binaries remain compatible with Java 8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal A feature or update proposal where feedback is sought.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants