Skip to content

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

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
+239 −78
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f27e1e6
[SLING-12493] Progressive cleanup of test cases
scottyuancoc Nov 22, 2024
554dac5
[SLING-12493] Migrate test cases to JUnit 5 with code clean-ups.
yuansc Nov 24, 2024
0ac1edf
[SLING-12493] Ensure proper closure of JCR sessions after each test c…
scottyuancoc Nov 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 36 additions & 13 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
<parent>
<groupId>org.apache.sling</groupId>
<artifactId>sling-bundle-parent</artifactId>
<version>41</version>
<version>48</version>
<relativePath />
</parent>

Expand Down Expand Up @@ -61,6 +61,18 @@
</plugins>
</reporting>

<dependencyManagement>
<dependencies>
<dependency>
<groupId>org.junit</groupId>
<artifactId>junit-bom</artifactId>
<version>5.11.3</version>
<type>pom</type>
<scope>import</scope>
</dependency>
</dependencies>
</dependencyManagement>

<dependencies>
<dependency>
<groupId>org.osgi</groupId>
Expand Down Expand Up @@ -104,7 +116,7 @@
<dependency>
<groupId>org.apache.sling</groupId>
<artifactId>org.apache.sling.commons.osgi</artifactId>
<version>2.1.0</version>
<version>2.4.0</version>
<scope>provided</scope>
</dependency>
<dependency>
Expand Down Expand Up @@ -142,16 +154,21 @@
<scope>provided</scope>
</dependency>
<!-- Testing -->
<dependency>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

<groupId>org.junit.jupiter</groupId>
<artifactId>junit-jupiter</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.apache.sling</groupId>
<artifactId>org.apache.sling.jcr.resource</artifactId>
<version>2.3.8</version>
<version>3.3.2</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.apache.sling</groupId>
<artifactId>org.apache.sling.commons.testing</artifactId>
<version>2.1.0</version>
<version>2.1.2</version>
<scope>test</scope>
<exclusions>
<exclusion>
Expand All @@ -162,32 +179,38 @@
</dependency>
<dependency>
<groupId>org.apache.sling</groupId>
<artifactId>org.apache.sling.testing.osgi-mock</artifactId>
<version>2.3.4</version>
<artifactId>org.apache.sling.testing.osgi-mock.junit5</artifactId>
<version>3.5.0</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.apache.sling</groupId>
<artifactId>org.apache.sling.testing.sling-mock.junit5</artifactId>
<version>3.5.0</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-all</artifactId>
<version>1.10.19</version>
<artifactId>mockito-core</artifactId>
<version>5.14.2</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.apache.jackrabbit</groupId>
<artifactId>jackrabbit-api</artifactId>
<version>2.19.2</version>
<scope>test</scope>
<artifactId>oak-jackrabbit-api</artifactId>
<version>1.72.0</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.apache.jackrabbit</groupId>
<artifactId>jackrabbit-jcr-commons</artifactId>
<version>2.19.4</version>
<version>2.19.6</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.apache.jackrabbit</groupId>
<artifactId>jackrabbit-core</artifactId>
<version>2.19.4</version>
<version>2.19.6 </version>
<scope>test</scope>
</dependency>
</dependencies>
Expand Down
11 changes: 10 additions & 1 deletion src/test/java/org/apache/sling/scripting/javascript/RepositoryScriptingTestBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@

import org.apache.sling.commons.testing.jcr.RepositoryTestBase;
import org.apache.sling.scripting.javascript.internal.ScriptEngineHelper;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still appears in the latest changeset.

/** Base class for tests which need a Repository
Expand All @@ -33,11 +35,18 @@ public class RepositoryScriptingTestBase extends RepositoryTestBase {
private int counter;

@Override
@BeforeEach
protected void setUp() throws Exception {
super.setUp();
script = new ScriptEngineHelper();
}


@Override
@AfterEach
protected void tearDown() throws Exception {
super.tearDown();
}

protected Node getNewNode() throws RepositoryException, NamingException {
return getTestRootNode().addNode("test-" + (++counter));
}
Expand Down
4 changes: 4 additions & 0 deletions src/test/java/org/apache/sling/scripting/javascript/TestSetupTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,21 +19,25 @@
package org.apache.sling.scripting.javascript;

import org.apache.sling.scripting.javascript.internal.ScriptEngineHelper;
import org.junit.jupiter.api.Test;


/** Verify that our test environment works */
public class TestSetupTest extends RepositoryScriptingTestBase {

/** Test our test repository setup */
@Test
public void testRootNode() throws Exception {
assertNotNull(getTestRootNode());
}

/** Test our script engine setup */
@Test
public void testScripting() throws Exception {
assertEquals("something",script.evalToString("out.print('something')"));
}

@Test
public void testScriptingWithData() throws Exception {
final ScriptEngineHelper.Data data = new ScriptEngineHelper.Data();
data.put("a", "A");
Expand Down
17 changes: 9 additions & 8 deletions ...java/org/apache/sling/scripting/javascript/internal/RhinoJavaScriptEngineFactoryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,19 +24,20 @@

import org.apache.sling.commons.classloader.DynamicClassLoaderManager;
import org.apache.sling.scripting.api.ScriptCache;
import org.apache.sling.testing.mock.osgi.junit.OsgiContext;
import org.junit.Rule;
import org.junit.Test;
import org.apache.sling.testing.mock.osgi.junit5.OsgiContext;
import org.apache.sling.testing.mock.osgi.junit5.OsgiContextExtension;
import org.junit.jupiter.api.extension.ExtendWith;
import org.junit.jupiter.api.Test;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

@ExtendWith(OsgiContextExtension.class)
public class RhinoJavaScriptEngineFactoryTest {

@Rule
public OsgiContext context = new OsgiContext();
private final OsgiContext context = new OsgiContext();

@Test
public void testRegistrationProperties() {
Expand All @@ -49,7 +50,7 @@ public void testRegistrationProperties() {
assertEquals(Arrays.asList("rhino", "Rhino", "javascript", "JavaScript", "ecmascript", "ECMAScript"), instance.getNames());
assertEquals("ECMAScript", instance.getLanguageName());
assertEquals("partial ECMAScript 2015 support", instance.getLanguageVersion());
assertTrue("Unexpected engine name", instance.getEngineName() != null && instance.getEngineName().contains("Rhino 1.7.7.1_1"));
assertTrue( instance.getEngineName() != null && instance.getEngineName().contains("Rhino 1.7.7.1_1"), "Unexpected engine name" );
}

}
13 changes: 10 additions & 3 deletions src/test/java/org/apache/sling/scripting/javascript/internal/RhinoJavaScriptEngineTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,17 @@
import org.mozilla.javascript.ImporterTopLevel;
import org.mozilla.javascript.Scriptable;

import junit.framework.TestCase;
import org.junit.jupiter.api.Test;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.fail;

public class RhinoJavaScriptEngineTest extends TestCase {
public class RhinoJavaScriptEngineTest {

private static ScriptCache scriptCache = Mockito.mock(ScriptCache.class);

@Test
public void testPreserveScopeBetweenEvals() throws ScriptException {
MockRhinoJavaScriptEngineFactory factory = new MockRhinoJavaScriptEngineFactory();
ScriptEngine engine = factory.getScriptEngine();
Expand All @@ -46,12 +51,13 @@ public void testPreserveScopeBetweenEvals() throws ScriptException {
try {
result = engine.eval("f += 1", context);
} catch (ScriptException e) {
TestCase.fail(e.getMessage());
fail(e.getMessage());
}
assertTrue(result instanceof Double);
assertEquals(2.0, result);
}

@Test
public void testNullSuppliedValue() throws ScriptException {
MockRhinoJavaScriptEngineFactory factory = new MockRhinoJavaScriptEngineFactory();
ScriptEngine engine = factory.getScriptEngine();
Expand All @@ -69,6 +75,7 @@ public void testNullSuppliedValue() throws ScriptException {
assertTrue(throwable.getMessage().contains("\"suppliedNullValue\" is not defined"));
}

@Test
public void testNotNullSuppliedValue() throws ScriptException {
MockRhinoJavaScriptEngineFactory factory = new MockRhinoJavaScriptEngineFactory();
ScriptEngine engine = factory.getScriptEngine();
Expand Down
28 changes: 20 additions & 8 deletions src/test/java/org/apache/sling/scripting/javascript/internal/ScriptEngineHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,10 @@
import org.apache.sling.commons.testing.osgi.MockBundle;
import org.apache.sling.commons.testing.osgi.MockComponentContext;
import org.apache.sling.scripting.api.ScriptCache;
import org.mockito.internal.util.reflection.Whitebox;
import org.apache.sling.testing.mock.osgi.MockOsgi;
import org.mockito.InjectMocks;
import org.mockito.Mock;
import org.mockito.MockitoAnnotations;
import org.mozilla.javascript.Context;
import org.mozilla.javascript.ScriptableObject;
import org.mozilla.javascript.Wrapper;
Expand All @@ -43,20 +46,29 @@

/** Helpers to run javascript code fragments in tests */
public class ScriptEngineHelper {

private static ScriptEngine engine;
private static ScriptCache scriptCache = mock(ScriptCache.class);

@Mock
private static ScriptCache scriptCache;

@Mock
private RhinoJavaScriptEngineFactoryConfiguration factoryConfiguration;

@InjectMocks
private RhinoJavaScriptEngineFactory factory;

public ScriptEngineHelper() {
MockitoAnnotations.openMocks(this);
}

public static class Data extends HashMap<String, Object> {
}

private static ScriptEngine getEngine() {
private ScriptEngine getEngine() {
if (engine == null) {
synchronized (ScriptEngineHelper.class) {
final RhinoMockComponentContext componentContext = new RhinoMockComponentContext();
final RhinoJavaScriptEngineFactoryConfiguration configuration = mock(RhinoJavaScriptEngineFactoryConfiguration.class);
RhinoJavaScriptEngineFactory factory = new RhinoJavaScriptEngineFactory();
Whitebox.setInternalState(factory, "scriptCache", scriptCache);
factory.activate(componentContext, configuration);
factory.activate( new RhinoMockComponentContext(), factoryConfiguration);
engine = factory.getScriptEngine();
}
}
Expand Down
7 changes: 5 additions & 2 deletions src/test/java/org/apache/sling/scripting/javascript/internal/TestPathRegexp.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,12 @@
*/
package org.apache.sling.scripting.javascript.internal;

import junit.framework.TestCase;
import org.junit.jupiter.api.Test;
import static org.junit.jupiter.api.Assertions.assertEquals;

public class TestPathRegexp {

public class TestPathRegexp extends TestCase {
@Test
public void testParentPath() {
String regexp = "([^/]*/)?[^/]*/\\.\\./";
assertEquals("math", "/../math".replaceAll(regexp, ""));
Expand All @@ -30,6 +32,7 @@ public void testParentPath() {
assertEquals("foo/math", "foo/bar/increment/../math".replaceAll(regexp, ""));
}

@Test
public void testCurrentPath() {
String regexp = "[^/]*/\\./";
assertEquals("math", "/./math".replaceAll(regexp, ""));
Expand Down
Loading