Skip to content

Add ClassLoaderUtils and log location from which an engine was loaded #788

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Add ClassLoaderUtils and log location from which an engine was loaded #788

wants to merge 1 commit into from

Conversation

sormuras
Copy link
Member

@sormuras sormuras commented Apr 10, 2017 •
edited
Loading

Overview

This commit enhances the configuration-level log statement with the location URL from where the classloader loaded the engine class. This commit also introduces dedicated ClassLoaderUtils class that encapsulates static class-related helper methods.

Addresses part of #775

I hereby agree to the terms of the JUnit Contributor License Agreement.

@sormuras sormuras added this to the 5.0 M5 milestone Apr 10, 2017
@sormuras sormuras self-assigned this Apr 10, 2017
@sormuras sormuras mentioned this pull request Apr 10, 2017
Closed
6 tasks
sbrannen
sbrannen requested changes Apr 10, 2017
View reviewed changes
documentation/src/docs/asciidoc/release-notes-5.0.0-M5.adoc Outdated
@@ -47,7 +47,9 @@ on GitHub.

===== New Features and Improvements

* ❓
* Added `getSourceLocation()` method in `TestEngine` interface to provide information
about the location from where the engine class was loaded from. When location URL is
Copy link
Member

Choose a reason for hiding this comment

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

"the location from which the engine class was loaded."

Copy link
Member

Choose a reason for hiding this comment

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

"When the location URL is available..."

junit-platform-commons/src/main/java/org/junit/platform/commons/util/ClassLoaderUtils.java Outdated
///CLOVER:ON

/**
* Get the location from where this objects underlying class was loaded from.
Copy link
Member

Choose a reason for hiding this comment

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

"Get the location from which the supplied object's class was loaded."

junit-platform-commons/src/main/java/org/junit/platform/commons/util/ClassLoaderUtils.java Outdated
/**
* Get the location from where this objects underlying class was loaded from.
*
* @param object to find the location its class was loaded from for
Copy link
Member

Choose a reason for hiding this comment

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

This is a tough one, grammatically speaking, but something like the following is more suitable:

@param object the object for whose class the location should be retrieved

In general, we always follow the pattern @param object the object....

junit-platform-commons/src/main/java/org/junit/platform/commons/util/ClassLoaderUtils.java Outdated
}
if (loader != null) {
String name = object.getClass().getCanonicalName();
name = name.replace(".", "/") + ".class";
Copy link
Member

Choose a reason for hiding this comment

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

Don't we have this kind of "replace" call in multiple places in the code base now?

If so, I think that would warrant the introduction of a new ClassUtils.

Copy link
Member Author

Choose a reason for hiding this comment

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

"Find in path..." revealed only this occurance of replace(".", "/") -- so no need for a ClassUtils collection for this specific use-case.

On the other hand, what happens when the class is a nested one, that is separated with a $-sign? Going to write a test for that.

junit-platform-commons/src/main/java/org/junit/platform/commons/util/ClassLoaderUtils.java
*/
@API(Internal)
public final class ClassLoaderUtils {

Copy link
Member

Choose a reason for hiding this comment

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

Please relocate org.junit.platform.commons.util.ReflectionUtils.getDefaultClassLoader() to this class as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Relocating in a separate commit to gather all 14 related file changes in a single place.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done via 89c9834

Copy link
Member

Choose a reason for hiding this comment

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

👍

junit-platform-commons/src/main/java/org/junit/platform/commons/util/ClassLoaderUtils.java Outdated
}
return Optional.empty();
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if we shouldn't wrap the meat of getLocation() in a try-catch block just in case....

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense.

We could also try the .getProtectionDomain().getCodeSource().getLocation() as a last resort here.

junit-platform-engine/src/main/java/org/junit/platform/engine/TestEngine.java Outdated
@@ -175,4 +177,16 @@
PackageUtils.getAttribute(getClass(), Package::getImplementationVersion).orElse("DEVELOPMENT"));
}

/**
* Get the location from where this engine was loaded from.
Copy link
Member

Choose a reason for hiding this comment

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

"Get the location from which this engine was loaded."

platform-tests/src/test/java/org/junit/platform/commons/util/ClassLoaderUtilsTests.java Outdated
import org.junit.jupiter.api.Test;

/**
* Unit tests for {@link PackageUtils}.
Copy link
Member

Choose a reason for hiding this comment

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

PackageUtils? 😜

@sormuras sormuras force-pushed the location_of_loaded_engine branch 2 times, most recently from b94af6e to b75b5bb Compare April 12, 2017 18:41
@sormuras
Log the location from which an engine class was loaded
455f911 
This commit enhances the configuration-level log statement with the
location URL from where the classloader loaded the engine class.

Also introduce dedicated `ClassLoaderUtils` class that encapsulates
static class-related helper methods. As a first action, this commit
moves `getDefaultClassLoader()` to `ClassLoaderUtils` helper.
@sormuras sormuras force-pushed the location_of_loaded_engine branch from b75b5bb to 455f911 Compare April 13, 2017 06:23
@sormuras sormuras changed the title Introduce getSourceLocation() in TestEngine interface Add ClassLoaderUtils and log location from which an engine was loaded Apr 13, 2017
@sormuras
Copy link
Member Author

Failing "pr" build: https://travis-ci.org/junit-team/junit5/builds/221362686
But working "push" build: https://travis-ci.org/junit-team/junit5/builds/221362671

?

Re-creating this PR.

@sormuras sormuras closed this Apr 13, 2017
@sormuras sormuras deleted the location_of_loaded_engine branch April 13, 2017 06:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbrannen sbrannen sbrannen approved these changes

Assignees

@sormuras sormuras

Labels
component: Platform type: enhancement
Projects
None yet
Milestone
5.0 M5
Development

Successfully merging this pull request may close these issues.
2 participants
@sormuras @sbrannen