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

LOGGING-185: fix integration tests #157

Closed
wants to merge 3 commits into from

Conversation

SingingBush
Copy link
Contributor

No description provided.

Copy link
Member

@garydgregory garydgregory left a comment

Choose a reason for hiding this comment

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

Hi @SingingBush
Thank you for your PR. Please see my comments. TY.

.github/workflows/maven.yml Outdated Show resolved Hide resolved
.github/workflows/maven.yml Outdated Show resolved Hide resolved
.github/workflows/maven.yml Outdated Show resolved Hide resolved
@@ -402,8 +402,8 @@ under the License.
<logkit>${logkit:logkit:jar}</logkit>
<servlet-api>${javax.servlet:servlet-api:jar}</servlet-api>
<commons-logging>target/${project.build.finalName}.jar</commons-logging>
<commons-logging-api>target/${project.artifactId}-api-${project.version}.jar</commons-logging-api>
<commons-logging-adapters>target/${project.artifactId}-adapters-${project.version}.jar</commons-logging-adapters>
<commons-logging-api>target/${project.artifactId}-api-${project.version}-api.jar</commons-logging-api>
Copy link
Member

Choose a reason for hiding this comment

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

That can't be right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought this was odd, could be due to changing jarName to finalName in the pom. However, jarName is supposed to just be an alias for finalName so it shouldn't have changed anything. When building I did see the jars being built in that naming format:

target/commons-logging-1.3.0-SNAPSHOT-full.jar
target/commons-logging-1.3.0-SNAPSHOT.jar
target/commons-logging-1.3.0-SNAPSHOT-sources.jar
target/commons-logging-1.3.0-SNAPSHOT-tests.jar
target/commons-logging-1.3.0-SNAPSHOT-test-sources.jar
target/commons-logging-adapters-1.3.0-SNAPSHOT-adapters.jar
target/commons-logging-api-1.3.0-SNAPSHOT-api.jar
target/commons-logging-tests.jar

To confirm, I just tried swapping finalName back to jarName but can confirm that the classifier is still appended. It is usual for maven to put the classifier at the end of the filename before the extension.

Perhaps the better thing to do is simple allow maven to do the standard thing by not specifying a value for jarName or finalName in which case it'll just create:

target/commons-logging-1.3.0-SNAPSHOT-adapters.jar
target/commons-logging-1.3.0-SNAPSHOT-api.jar

However the test code itself is looking for the existence of commons-logging-adapters-1 on the classpath.

<commons-logging-api>target/${project.artifactId}-api-${project.version}.jar</commons-logging-api>
<commons-logging-adapters>target/${project.artifactId}-adapters-${project.version}.jar</commons-logging-adapters>
<commons-logging-api>target/${project.artifactId}-api-${project.version}-api.jar</commons-logging-api>
<commons-logging-adapters>target/${project.artifactId}-adapters-${project.version}-adapters.jar</commons-logging-adapters>
Copy link
Member

Choose a reason for hiding this comment

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

That can't be right.

@@ -145,8 +143,9 @@ public void testPaths() throws Exception {
// (context, child, parent).
final ClassLoader systemLoader = ClassLoader.getSystemClassLoader();
assertNotNull("System classloader is null", systemLoader);
assertNotEquals("System classloader has unexpected type", PathableClassLoader.class.getName(),
systemLoader.getClass().getName());
assertFalse("System classloader has unexpected type",
Copy link
Member

Choose a reason for hiding this comment

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

What's the difference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in junit 4 assertTrue is part of TestCase and is available when the tests are running. assertNotEquals was coming from a static import of org.junit.Assert which was getting a class not fund exception. It could be changed to assertNotSame as that's also part of the TestCase class.

@@ -127,8 +125,8 @@ public void testAllForbidden() {
final Object factoryTable = factoryField.get(null);
assertNotNull(factoryTable);
final String ftClassName = factoryTable.getClass().getName();
assertNotEquals("Custom hashtable unexpectedly used",
CustomHashtable.class.getName(), ftClassName);
assertFalse("Custom hashtable unexpectedly used",
Copy link
Member

Choose a reason for hiding this comment

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

What's the difference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as above

@garydgregory
Copy link
Member

Note: the build is still broken.

@SingingBush
Copy link
Contributor Author

Note: the build is still broken.

This is odd and frustrating, I've been able to run this locally on both Java 8 and 11

@garydgregory
Copy link
Member

garydgregory commented Sep 2, 2023

@SingingBush
Thank you for your work on this issue. It looks like this build was ported from an Ant build looking at how it builds more than one jar. Normally this is done in a multi-module project. But I'm guessing single vs multi is not related to the issue of failing tests.

@garydgregory
Copy link
Member

@SingingBush
The convention is to have the version label at the end for normal code jar files. Javadoc and source jar files can have a different convention iirc.

@garydgregory
Copy link
Member

Closing: Fixed by #180

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants