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.

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

UIFR-222: Adding Java 17 Compatability #76

Merged
merged 1 commit into from
Dec 5, 2023
Merged

UIFR-222: Adding Java 17 Compatability #76

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
66 changes: 40 additions & 26 deletions .github/workflows/maven.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,40 +1,54 @@
# this build is designed to replicate the Travis CI workflow
name: Build with Maven
name: Java CI with Maven

on:
push:
branches: [ master ]
branches: [ "master" ]
pull_request:
branches: [ master ]
workflow_dispatch:
branches: [ "master" ]

jobs:
build:
strategy:
matrix:
platform: [ ubuntu-latest ]
java-version: [ 8 ]
java-8:

runs-on: ${{ matrix.platform }}
env:
PLATFORM: ${{ matrix.platform }}
JAVA_VERSION: ${{ matrix.java-version }}
runs-on: ubuntu-latest

steps:
- uses: actions/checkout@v2
- name: Set up JDK
uses: actions/setup-java@v1
- uses: actions/checkout@v3
- name: Set up JDK 8
uses: actions/setup-java@v3
with:
java-version: ${{ matrix.java-version }}
- name: Cache local Maven repository
uses: actions/cache@v2
java-version: '8'
distribution: 'adopt'
cache: maven
- name: Build with Maven
run: mvn clean install --file pom.xml

java-11:

runs-on: ubuntu-latest

steps:
- uses: actions/checkout@v3
- name: Set up JDK 11
uses: actions/setup-java@v3
with:
path: ~/.m2/repository
key: ${{ runner.os }}-maven-${{ hashFiles('**/pom.xml') }}
restore-keys: |
${{ runner.os }}-maven-
- name: Install dependencies
run: mvn clean install -DskipTests=true -Dmaven.javadoc.skip=true --batch-mode --show-version --file pom.xml
java-version: '11'
distribution: 'adopt'
cache: maven
- name: Build with Maven
run: mvn test --batch-mode --file pom.xml
run: mvn clean install --file pom.xml

java-17:

runs-on: ubuntu-latest

steps:
- uses: actions/checkout@v3
- name: Set up JDK 17
uses: actions/setup-java@v3
with:
java-version: '17'
distribution: 'adopt'
cache: maven
- name: Build with Maven
run: mvn clean install --file pom.xml
2 changes: 1 addition & 1 deletion api/src/main/java/org/openmrs/ui/framework/NameSupportCompatibility.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;
import org.openmrs.PersonName;
import org.openmrs.layout.web.name.NameSupport;
import org.openmrs.layout.name.NameSupport;
import org.openmrs.util.OpenmrsClassLoader;

import java.lang.reflect.Method;
Expand Down
17 changes: 0 additions & 17 deletions api/src/main/java/org/openmrs/ui/framework/converter/StringToPatientProblemConverter.java
View file
Open in desktop

This file was deleted.

4 changes: 2 additions & 2 deletions api/src/test/java/org/openmrs/ui/framework/FormatterImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ public void testFormattingConceptNumeric() throws Exception {
ConceptDatatype numericDatatype = new ConceptDatatype();
numericDatatype.setHl7Abbreviation("NM");
conceptNumeric.setDatatype(numericDatatype);
conceptNumeric.setPrecise(true);
conceptNumeric.setAllowDecimal(true);

Obs numericObs = new Obs();
numericObs.setConcept(conceptNumeric);
Expand All @@ -214,7 +214,7 @@ public void testFormattingConceptNumeric_shouldNotFailIfNoUnits() throws Excepti
ConceptDatatype numericDatatype = new ConceptDatatype();
numericDatatype.setHl7Abbreviation("NM");
conceptNumeric.setDatatype(numericDatatype);
conceptNumeric.setPrecise(true);
conceptNumeric.setAllowDecimal(true);

Obs numericObs = new Obs();
numericObs.setConcept(conceptNumeric);
Expand Down
2 changes: 0 additions & 2 deletions api/src/test/java/org/openmrs/ui/framework/SimpleObjectTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
import org.openmrs.ui.framework.formatter.FormatterService;
import org.openmrs.ui.framework.fragment.FragmentActionUiUtils;
import org.springframework.context.MessageSource;
import org.springframework.test.annotation.DirtiesContext;

import java.util.Map;

Expand All @@ -20,7 +19,6 @@
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

@DirtiesContext
public class SimpleObjectTest {

private UiUtils ui;
Expand Down
2 changes: 1 addition & 1 deletion ...src/test/java/org/openmrs/ui/framework/converter/StringToGlobalPropertyConverterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ public class StringToGlobalPropertyConverterTest extends BaseModuleContextSensit
public void convert_shouldConvertStringToGlobalProperty() {
StringToGlobalPropertyConverter converter = new StringToGlobalPropertyConverter();
GlobalProperty prop = converter.convert("locale.allowed.list");
Assert.assertEquals("en", prop.getPropertyValue());
Assert.assertEquals("en_GB", prop.getPropertyValue());
Copy link
Member

Choose a reason for hiding this comment

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

Did you change this accidentally?

Copy link
Member Author

Choose a reason for hiding this comment

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

After updating to platform 2.0.0 this test failed with the following error:

junit.framework.ComparisonFailure: expected:<en[]> but was:<en[_GB]>
	at org.openmrs.ui.framework.converter.StringToGlobalPropertyConverterTest.convert_shouldConvertStringToGlobalProperty

That's why I updated the test.

Copy link
Member

Choose a reason for hiding this comment

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

OK. That makes sense. 😊

}

@Test
Expand Down
14 changes: 5 additions & 9 deletions api/src/test/java/org/openmrs/ui/framework/formatter/FormatterServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.context.MessageSource;
import org.springframework.core.Ordered;
import org.springframework.test.annotation.DirtiesContext;

import java.util.Locale;

Expand All @@ -24,7 +23,6 @@ public class FormatterServiceTest extends BaseModuleContextSensitiveTest {
private FormatterService formatterService;

@Test
@DirtiesContext
public void testFormatting() throws Exception {
HandlebarsFormatterFactory classFormatter = new HandlebarsFormatterFactory();
classFormatter.setForClass("org.openmrs.Obs");
Expand All @@ -49,25 +47,23 @@ public String toString() {
}

@Test
@DirtiesContext
public void testMessage() throws Exception {
MessageSource messageSource = mock(MessageSource.class);

FormatterService messageFormatterService = new FormatterService();
messageFormatterService.setMessageSource(messageSource);
HandlebarsFormatterFactory classFormatter = new HandlebarsFormatterFactory();
classFormatter.setForClass("org.openmrs.Obs");
classFormatter.setTemplate("{{ message 'testing.123.testing' }} something");
formatterService.addClassFormatter(classFormatter);
Copy link
Member

Choose a reason for hiding this comment

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

Why did you get rid of the above?

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't. I just changed the variable. From formatterService to messageFormatService

Copy link
Member

Choose a reason for hiding this comment

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

It is not easy to tell what exactly changed or what was necessary to make the test pass.
For instance, why do we now instantiate the message format service like FormatterService messageFormatterService = new FormatterService()

Copy link
Member Author

Choose a reason for hiding this comment

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

The error I mentioned above happens because of the test leakage from the formatterService. That's why I added the method variable for it.

Copy link
Member

Choose a reason for hiding this comment

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

Can you explain more what you mean by the test leackage?

Copy link
Member Author

@wikumChamith wikumChamith Dec 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Because we removed @DirtiesContext, changes happen to the formatterService on the other two tests passed on to this test. This causes the test leakage. That's why I decided to use a separate FormatterService for this test.

Copy link
Member

Choose a reason for hiding this comment

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

Makes perfect sense! Thanks for catching that. 👍

messageFormatterService.addClassFormatter(classFormatter);

Context.setLocale(Locale.ENGLISH);
Formatter formatter = formatterService.getFormatter();
formatterService.setMessageSource(messageSource);
Formatter formatter = messageFormatterService.getFormatter();

String result = formatter.format(new Obs(), Locale.ENGLISH);
formatter.format(new Obs(), Locale.ENGLISH);
verify(messageSource).getMessage("testing.123.testing", null, Locale.ENGLISH);
Copy link
Member

Choose a reason for hiding this comment

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

Are these really the same?

Copy link
Member Author

Choose a reason for hiding this comment

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

With this, I got an error. So that's why I changed the test.

[ERROR] org.openmrs.ui.framework.formatter.FormatterServiceTest.testMessage -- Time elapsed: 0.021 s <<< FAILURE!
Wanted but not invoked:
messageSource.getMessage(
    "testing.123.testing",
    null,
    en
);
-> at org.openmrs.ui.framework.formatter.FormatterServiceTest.testMessage(FormatterServiceTest.java:65)
Actually, there were zero interactions with this mock.

	at org.openmrs.ui.framework.formatter.FormatterServiceTest.testMessage(FormatterServiceTest.java:65)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:47)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:44)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
	at org.springframework.test.context.junit4.statements.RunBeforeTestMethodCallbacks.evaluate(RunBeforeTestMethodCallbacks.java:73)
	at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
	at org.springframework.test.context.junit4.statements.RunAfterTestMethodCallbacks.evaluate(RunAfterTestMethodCallbacks.java:82)
	at org.springframework.test.context.junit4.statements.SpringRepeat.evaluate(SpringRepeat.java:73)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:271)
	at org.springframework.test.context.junit4.SpringJUnit4ClassRunner.runChild(SpringJUnit4ClassRunner.java:217)
	at org.springframework.test.context.junit4.SpringJUnit4ClassRunner.runChild(SpringJUnit4ClassRunner.java:83)
	at org.junit.runners.ParentRunner$3.run(ParentRunner.java:238)
	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:63)
	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:236)
	at org.junit.runners.ParentRunner.access$000(ParentRunner.java:53)
	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:229)
	at org.springframework.test.context.junit4.statements.RunBeforeTestClassCallbacks.evaluate(RunBeforeTestClassCallbacks.java:61)
	at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
	at org.springframework.test.context.junit4.statements.RunAfterTestClassCallbacks.evaluate(RunAfterTestClassCallbacks.java:68)
	at org.junit.runners.ParentRunner.run(ParentRunner.java:309)
	at org.springframework.test.context.junit4.SpringJUnit4ClassRunner.run(SpringJUnit4ClassRunner.java:163)
	at org.apache.maven.surefire.junit4.JUnit4Provider.execute(JUnit4Provider.java:316)
	at org.apache.maven.surefire.junit4.JUnit4Provider.executeWithRerun(JUnit4Provider.java:240)
	at org.apache.maven.surefire.junit4.JUnit4Provider.executeTestSet(JUnit4Provider.java:214)
	at org.apache.maven.surefire.junit4.JUnit4Provider.invoke(JUnit4Provider.java:155)
	at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:385)
	at org.apache.maven.surefire.booter.ForkedBooter.execute(ForkedBooter.java:162)
	at org.apache.maven.surefire.booter.ForkedBooter.run(ForkedBooter.java:507)
	at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:495)

Copy link
Member

@dkayiwa dkayiwa Dec 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The test was verifying that this method is invoked, and with those exact parameters. Does your change do the same?

Copy link
Member Author

@wikumChamith wikumChamith Dec 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I just noticed the old test passes when run individually. It only fails when running with the other test cases. Could it be caused because I removed @DirtiesContext?

Copy link
Member

@dkayiwa dkayiwa Dec 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Of course you can always test and confirm your theory without first seeking permission from me. 😊

Copy link
Member Author

Choose a reason for hiding this comment

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

I fixed the issue.

}

@Test
@DirtiesContext
public void testOrder() throws Exception {
HandlebarsFormatterFactory wrongFormatter1 = new HandlebarsFormatterFactory();
wrongFormatter1.setForClass("org.openmrs.Obs");
Expand Down
43 changes: 32 additions & 11 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,19 +43,13 @@
</modules>

<properties>
<openmrsPlatformVersion>1.9.9</openmrsPlatformVersion>
<springVersion>3.0.5.RELEASE</springVersion>
<openmrsPlatformVersion>2.0.0</openmrsPlatformVersion>
<springVersion>4.1.4.RELEASE</springVersion>
<handlebarsVersion>1.3.1</handlebarsVersion>
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
</properties>

<dependencies>
<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-all</artifactId>
<version>1.9.0</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.hamcrest</groupId>
<artifactId>hamcrest-all</artifactId>
Expand All @@ -78,6 +72,12 @@
<artifactId>joda-convert</artifactId>
<version>1.2</version>
</dependency>
<dependency>
<groupId>com.thoughtworks.xstream</groupId>
<artifactId>xstream</artifactId>
Copy link
Member

Choose a reason for hiding this comment

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

Did you intentionally leave out scope?

Copy link
Member Author

Choose a reason for hiding this comment

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

I added the scope

<version>1.4.20</version>
<scope>test</scope>
</dependency>
</dependencies>

<dependencyManagement>
Expand Down Expand Up @@ -229,8 +229,8 @@
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-compiler-plugin</artifactId>
<configuration>
<target>1.6</target>
<source>1.6</source>
<target>1.7</target>
<source>1.7</source>
</configuration>
</plugin>
<plugin>
Expand Down Expand Up @@ -262,7 +262,7 @@
<artifactId>maven-surefire-plugin</artifactId>
<version>2.22.1</version>
<configuration>
<argLine>-Xmx512m -XX:MaxPermSize=512m -Djdk.net.URLClassPath.disableClassPathURLCheck=true</argLine>
Copy link
Member

Choose a reason for hiding this comment

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

Was this change also needed by the tests to pass?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we can keep this. Should I revert this?

<argLine>-Xmx512m -Djdk.net.URLClassPath.disableClassPathURLCheck=true</argLine>
</configuration>
</plugin>
</plugins>
Expand Down Expand Up @@ -306,4 +306,25 @@
</snapshotRepository>
</distributionManagement>

<profiles>
<profile>
<id>Java 17</id>
<activation>
<jdk>17</jdk>
</activation>
<build>
<plugins>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-surefire-plugin</artifactId>
<version>3.2.2</version>
<configuration>
<argLine>--add-opens java.base/java.lang=ALL-UNNAMED</argLine>
</configuration>
</plugin>
</plugins>
</build>
</profile>
</profiles>

</project>