[SLING-12493] Progressive cleanup of test cases #4

yuansc · 2024-11-20T01:15:18Z

Updated sling-bundle-parent to version 48.
Upgraded test dependencies to the latest compatible versions.
Migrated unit tests to use Mockito 5.

joerghoh · 2024-11-20T09:42:26Z

src/test/java/org/apache/sling/scripting/javascript/internal/ScriptEngineHelper.java

-                RhinoJavaScriptEngineFactory factory = new RhinoJavaScriptEngineFactory();
-                Whitebox.setInternalState(factory, "scriptCache", scriptCache);
+                // RhinoJavaScriptEngineFactory factory = new RhinoJavaScriptEngineFactory();
+                // Whitebox.setInternalState(factory, "scriptCache", scriptCache);


Please don't comment the code, but rather remove it. We have the git history to restore it if we ever need it again.

rombert

PR looks good! A couple of small notes on top of what Joerg added and we should be good to merge.

rombert · 2024-11-20T22:08:29Z

src/test/java/org/apache/sling/scripting/javascript/RepositoryScriptingTestBase.java

@@ -25,7 +25,6 @@
 import org.apache.sling.commons.testing.jcr.RepositoryTestBase;
 import org.apache.sling.scripting.javascript.internal.ScriptEngineHelper;

-


Nit: please don't no whitespace only changes, it make it harder to track history of file changes.

This still appears in the latest changeset.

rombert · 2024-11-20T22:18:45Z

pom.xml

@@ -142,16 +142,22 @@
            <scope>provided</scope>
        </dependency>
        <!-- Testing -->
+        <dependency>


Is this dependency needed? If I remove it locally your PR works just fine.

Lines #145-150: The org.apache.sling.commons.osgi 2.4.2 dependency is required to resolve java.lang.NoClassDefFoundError: org/apache/sling/commons/osgi/Order test failures encountered when building with JDK 11 + Maven 3.9.6

I don't see that when building locally with

Apache Maven 3.9.9 Maven home: /usr/share/maven Java version: 11.0.25, vendor: Oracle Corporation, runtime: /usr/lib64/jvm/java-11-openjdk-11 Default locale: en_US, platform encoding: UTF-8 OS name: "linux", version: "6.11.8-1-default", arch: "amd64", family: "unix"

but it might be caused by the fact that we now declare the dependency twice

[WARNING] [WARNING] Some problems were encountered while building the effective model for org.apache.sling:org.apache.sling.scripting.javascript:jar:3.1.5-SNAPSHOT [WARNING] 'dependencies.dependency.(groupId:artifactId:type:classifier)' must be unique: org.apache.sling:org.apache.sling.commons.osgi:jar -> version 2.1.0 vs 2.4.2 @ line 145, column 21 [WARNING] [WARNING] It is highly recommended to fix these problems because they threaten the stability of your build. [WARNING] [WARNING] For this reason, future Maven versions might no longer support building such malformed projects. [WARNING]

I suggest you bump the existing Commons OSGi dependency to the version that is needed for the tests to pass and remove this one.

rombert · 2024-11-20T22:30:07Z

@yuansc - FWIW the build should pass but the SonarCloud checks fail (project is not onboarded). I opened SLING-12496 to track that.

rombert · 2024-11-20T22:30:56Z

Oh, and the commit seems to belong to a different GitHub user - is that intended? https://github.com/scottyuancoc

yuansc · 2024-11-21T04:37:06Z

Oh, and the commit seems to belong to a different GitHub user - is that intended? https://github.com/scottyuancoc

It was unintentional. I will fix it in the updated PR.

yuansc · 2024-11-21T06:07:24Z

Hello @rombert and @joerghoh,

Thank you for reviewing and providing your feedback. The PR has been updated accordingly. Please feel free to let me know your thoughts.

Thanks!

rombert

Thanks for the quick update @yuansc - some follow-up comments.

rombert · 2024-11-21T08:14:13Z

pom.xml

@@ -142,16 +142,22 @@
            <scope>provided</scope>
        </dependency>
        <!-- Testing -->
+        <dependency>


I don't see that when building locally with

Apache Maven 3.9.9 Maven home: /usr/share/maven Java version: 11.0.25, vendor: Oracle Corporation, runtime: /usr/lib64/jvm/java-11-openjdk-11 Default locale: en_US, platform encoding: UTF-8 OS name: "linux", version: "6.11.8-1-default", arch: "amd64", family: "unix"

but it might be caused by the fact that we now declare the dependency twice

[WARNING] [WARNING] Some problems were encountered while building the effective model for org.apache.sling:org.apache.sling.scripting.javascript:jar:3.1.5-SNAPSHOT [WARNING] 'dependencies.dependency.(groupId:artifactId:type:classifier)' must be unique: org.apache.sling:org.apache.sling.commons.osgi:jar -> version 2.1.0 vs 2.4.2 @ line 145, column 21 [WARNING] [WARNING] It is highly recommended to fix these problems because they threaten the stability of your build. [WARNING] [WARNING] For this reason, future Maven versions might no longer support building such malformed projects. [WARNING]

I suggest you bump the existing Commons OSGi dependency to the version that is needed for the tests to pass and remove this one.

rombert · 2024-11-21T08:14:55Z

src/test/java/org/apache/sling/scripting/javascript/RepositoryScriptingTestBase.java

@@ -25,7 +25,6 @@
 import org.apache.sling.commons.testing.jcr.RepositoryTestBase;
 import org.apache.sling.scripting.javascript.internal.ScriptEngineHelper;

-


This still appears in the latest changeset.

yuansc · 2024-11-21T17:27:12Z

Thanks for the quick update @yuansc - some follow-up comments.

Hi @rombert,

Thank you for your prompt response and feedback. The differing compilation results with Maven 3.9.9 and JDK 11.0.25 on Linux are indeed interesting. I will try the recommended approach and update the PR once the local build issue with Maven 3.9.6 and Oracle JDK 11.0.22 on Windows is resolved.

Thanks!
@yuansc

* Updated sling-bundle-parent to version 48. * Upgraded test dependencies to the latest compatible versions. * Migrated unit tests to use Mockito 5.

yuansc · 2024-11-22T05:37:13Z

Hi @rombert,

The existing Commons OSGi has been updated to version 2.4.0, and the missing empty line changes were also addressed in the last PR update. Could you kindly review it?

Thanks!
@yuansc #

* Removed usage of deprecated initMocks() method. * Resolved local variable shadowing object field declarations. * Eliminated usage of deprecated MockResourceResolver.

yuansc · 2024-11-27T01:33:46Z

Hello Everyone,

To also address the issues reported by SonarQube Quality Gateway, I've push an additional commit to clean up the issue migrate all test cases to JUnit 5 discussed in https://issues.apache.org/jira/browse/SLING-12493. Hopefully, this additional commit would help with an cleaner start work to getting JDK 17 and JDK 21 in place through MOCK_JCR.

Best Regards,
@yuansc

…ase.

yuansc · 2024-11-27T05:32:50Z

Hello @rombert and @joerghoh,

I’ve added another commit to ensure proper closure of the JCR session upon completion of each test. With this latest commit, the JDK 17 and JDK 21 builds have been successful and clean.

Could you kindly review and merge this pull request? If there are any additional improvements that could further enhance the codebase, please let me know, and I’ll be happy to make adjustments.

Thank you very much!

Best Regards,
@yuansc

sonarqubecloud · 2024-11-27T06:42:54Z

Quality Gate passed

Issues
88 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

rombert · 2024-11-27T12:49:01Z

@yuansc - thanks for the follow-up. I would prefer a single Jira issue/PR for a single topic, so I would use one for fixing the tests, one for the JUnit 5 migration, etc. I will try to cherry-pick your initial commit to a separate PR and merge it directly, then we can iterate on the JUnit 5 fixes and other improvements. Hope that works for you.

rombert · 2024-11-27T12:58:14Z

@yuansc - I merged your initial changes, please rebase on top of those (it's your exact first commit) and we can follow-up on the JUnit 5 migration. Thanks for the contribution!

joerghoh reviewed Nov 20, 2024

View reviewed changes

rombert requested changes Nov 20, 2024

View reviewed changes

yuansc force-pushed the bugfix/SLING-12493 branch 2 times, most recently from 3ac3691 to 6baa827 Compare November 21, 2024 05:58

yuansc requested a review from rombert November 21, 2024 06:04

rombert requested changes Nov 21, 2024

View reviewed changes

yuansc force-pushed the bugfix/SLING-12493 branch from 6baa827 to bc66b2e Compare November 22, 2024 05:03

[SLING-12493] Progressive cleanup of test cases

f27e1e6

* Updated sling-bundle-parent to version 48. * Upgraded test dependencies to the latest compatible versions. * Migrated unit tests to use Mockito 5.

yuansc force-pushed the bugfix/SLING-12493 branch from bc66b2e to f27e1e6 Compare November 22, 2024 05:23

yuansc requested a review from rombert November 22, 2024 05:37

[SLING-12493] Migrate test cases to JUnit 5 with code clean-ups.

554dac5

* Removed usage of deprecated initMocks() method. * Resolved local variable shadowing object field declarations. * Eliminated usage of deprecated MockResourceResolver.

[SLING-12493] Ensure proper closure of JCR sessions after each test c…

0ac1edf

…ase.

yuansc requested a review from joerghoh November 27, 2024 05:33

rombert mentioned this pull request Nov 27, 2024

SLING-12493 - Apache Sling Scripting JavaScript 3.1.4+ test failed with Java 17 and Java 21 #5

Merged

		@@ -25,7 +25,6 @@
		import org.apache.sling.commons.testing.jcr.RepositoryTestBase;
		import org.apache.sling.scripting.javascript.internal.ScriptEngineHelper;

[SLING-12493] Progressive cleanup of test cases #4

Are you sure you want to change the base?

[SLING-12493] Progressive cleanup of test cases #4

Conversation

yuansc commented Nov 20, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rombert left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rombert commented Nov 20, 2024

Uh oh!

rombert commented Nov 20, 2024

Uh oh!

yuansc commented Nov 21, 2024

Uh oh!

yuansc commented Nov 21, 2024

Uh oh!

rombert left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yuansc commented Nov 21, 2024

Uh oh!

yuansc commented Nov 22, 2024

Uh oh!

yuansc commented Nov 27, 2024

Uh oh!

yuansc commented Nov 27, 2024

Uh oh!

sonarqubecloud bot commented Nov 27, 2024

Quality Gate passed

Uh oh!

rombert commented Nov 27, 2024

Uh oh!

rombert commented Nov 27, 2024

Uh oh!

Uh oh!