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

Fix bug in loadJars() #131

Merged
merged 5 commits into from
Apr 1, 2017
Merged

Fix bug in loadJars() #131

merged 5 commits into from
Apr 1, 2017

Conversation

janhoy
Copy link
Member

@janhoy janhoy commented Mar 30, 2017

DefaultPluginLoader loads my plugin just fine, it manages to add classes/ to the PluginClassLoader, but the jars in lib/ are not added.

Turn out that loadJars() refuses to add jars if the folder File object is not an absolute path.

Also adds slf4j-simple logger in test scope and convert some File usage to nio Path.

… not absolute

Add slf4j-simple logger in test scope for some logging from tests
Convert some File usage to Path
@coveralls
Copy link

coveralls commented Mar 30, 2017

Coverage Status

Coverage remained the same at 15.176% when pulling 5b2b800 on cominvent:load-jars-fix into e9126c4 on decebals:master.

Also adds a nice exception in case we try to load a plugin without a classes folder
@Test
public void testPluginLoader() throws ClassNotFoundException {
PluginManager manager = new DefaultPluginManager();
assertNotNull(manager.loadPlugin(Paths.get("src/test/resources/plugin-with-jar")));
Copy link
Member

Choose a reason for hiding this comment

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

Is this file available only on your machine?

@@ -96,27 +92,27 @@ public static boolean delete(File fileOrFolder) {
return success;
}

public static List<File> getJars(File folder) {
public static List<File> getJars(Path folder) {
Copy link
Member

Choose a reason for hiding this comment

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

I am OK with the modifications from this class. My intention is to move this code to Java 7 NIO API, and to use Files.newDirectoryStream (less code lines and probably faster).

@@ -33,6 +33,12 @@
<version>1.7.5</version>
</dependency>
<dependency>
<groupId>org.slf4j</groupId>
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this? In all my libraries projects I use slf4j-api as dependency and I use what slf4j I wish in application.

Copy link
Member Author

Choose a reason for hiding this comment

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

This added dependency is an slf4j logger implementation with a test scope so it will allow tests to output logs. The runtime jars will NOT contain this class - as you say, the user app decides what to use.

@coveralls
Copy link

Coverage Status

Coverage increased (+17.03%) to 32.204% when pulling 0f7dd56 on cominvent:load-jars-fix into e9126c4 on decebals:master.

@coveralls
Copy link

coveralls commented Mar 30, 2017

Coverage Status

Coverage increased (+17.0%) to 32.147% when pulling 0f7dd56 on cominvent:load-jars-fix into e9126c4 on decebals:master.

@janhoy
Copy link
Member Author

janhoy commented Mar 30, 2017

Wow, test coverage from 17% to 32% :-)

@decebals
Copy link
Member

Please remove PluginLoaderTest class from this PR because comes with .class files in resources and I don't like this. Maybe we will find other method to test PluginLoader (for example via mock object).

@coveralls
Copy link

coveralls commented Mar 31, 2017

Coverage Status

Coverage increased (+0.4%) to 18.287% when pulling 02effee on cominvent:load-jars-fix into c6aa1f1 on decebals:master.

@decebals decebals merged commit aef9d3a into pf4j:master Apr 1, 2017
@decebals
Copy link
Member

decebals commented Apr 1, 2017

Thanks! I prefer small PR that solves only one problem.

@janhoy janhoy deleted the load-jars-fix branch April 1, 2017 08:40
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