Solves issue #530

[mhagnumdw]

Solves #530

[coveralls]

Pull Request Test Coverage Report for Build 1102

• 31 of 37 (83.78%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.2%) to 21.908%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pippo-controller-parent/pippo-controller/src/main/java/ro/pippo/controller/util/ClassUtils.java	31	37	83.78%

Totals
Change from base Build 1095:	0.2%
Covered Lines:	1408
Relevant Lines:	6427

---

💛 - Coveralls

[decebals]

I like this solution because it does not involve adding a new dependency.

[mhagnumdw]

I'm thinking how to create a test for ClassUtils.getClasses.

[decebals]

If you think that getClasses method is too big you can split it in small pieces (getClassesFromJar, getClassesFromXYZ) if you think that this operation improve the readability.

[mhagnumdw]

Hi @decebals !

Let me know if you want me to change anything.

I needed to create dummy classes and a ultra small jar inside src/test/resources/ in order to create the test for the ClassUtils.getClasses method. Let me know if you want me to change anything, or even take another approach.

[decebals]

I am happy to see some tests 😄. I think that the PR is good. I prefer to not store the classes use din test and the jar file but this is another story. I think that we can improve this aspect in the future.
For example, in PF4J I use with success in tests, the PluginJar. If you have some time and you think that helps, you can copy the PluginJar in Pippo, change the name (something like DynamicJar or something better) and try to create the jar used in this test, in a dynamic mode, without to have Class1-Class6 and the jar file in repo. You can take a look of AbstractExtensionFinderTest to see how I create class bytes from java sources on the fly via compilation.
Again, if you have time and you like the research. Otherwise the PR is OK for merging.

[mhagnumdw]

Hi @decebals !

I agree with you! I'll take a look at what you said.

I saw that your AbstractExtensionFinderTest user com.google.testing.compile.Compiler.javac(). I have a minimalist lib that is also used here TestUtils.java.

[decebals]

I have a minimalist lib that is also used here TestUtils.java.

This means that the work is almost ready 😄. I redirected you to PF4J just to see the code. If you like something from that project, you can use that part here. If you think that it's too complicate for our needs, try to find something more simple.

[mhagnumdw]

Hi @decebals !

Today I looked at it again and I have good and bad news.

• Good

Creating the Jar and classes at test run time worked! 🥳

• Bad

As we are testing a class with static methods I need to use @RunWith(PowerMockRunner.class) and this ends up causing the compilation Compiler.javac().compile(classes) to break with the message java.lang.ClassCastException: class com.sun.tools.javac.api.JavacTool.

If I remove @RunWith(PowerMockRunner.class) I don't have the compilation problem, but then I can't mock the static methods I need.

I kind of blindly tried to use the runner PowerMockJUnit49RunnerDelegateImpl.class, but then I have the same problem mentioned above.

Another solution, which works, is to hack the ClassUtils class, which I basically did:

public class ClassUtils {

    private final static Logger log = LoggerFactory.getLogger(ClassUtils.class);

    private static ClassUtils INSTANCE; // added

    protected ClassUtils() { // added
        ClassUtils.INSTANCE = this;
    }

    @SuppressWarnings("unchecked")
    public static <T> Class<T> getClass(String className) {
        try {
            // return (Class<T>) Class.forName(className); // removed
            return (Class<T>) Class.forName(className, true, INSTANCE.getClassLoader()); // added
        } catch (Exception e) {
            throw new PippoRuntimeException("Failed to get class '{}'", className);
        }
    }

    protected ClassLoader getClassLoader() { // added - It will be mocked in the test
        return this.getClass().getClassLoader();
    }

   // ...
}

Within the ClassUtilsTest class I create a mock of ClassUtils, something like this:

public static class ClassUtilsMock extends ClassUtils {
    private URL jarUrl;
    ClassUtilsMock(URL jarUrl) {
        super();
        this.jarUrl = jarUrl;
    }
    @Override
    protected ClassLoader getClassLoader() {
        return new URLClassLoader(new URL[] { jarUrl });
    }
}

And in the test methods I refer to ClassUtilsMock.

What do you think? Any comments?

ps: I haven't committed yet.

[mhagnumdw]

Ohh, I had forgotten about this:

https://github.com/powermock/powermock/wiki/FAQ

I get a ClassCastException from DocumentBuilderFactory, SaxParserFactory or other XML related classes
The reason is that the XML framework tries to instantiate classes using reflection and does this from the thread context classloader (PowerMock's classloader) but then tries to assign the created object to a field not loaded by the same classloader. When this happens you need to make use of the @PowerMockIgnore annotation to tell PowerMock to defer the loading of a certain package to the system classloader. What you need to ignore is case specific but usually it's the XML framework or some packages that interact with it. E.g. @PowerMockIgnore({"org.xml.", "javax.xml."}). Another option would be to try to bootstrap using our Java Agent.

I spent a lot of time on this!

Adding the annotation below to the test class the problem went away:

@PowerMockIgnore({
    "com.sun.tools.*",
    "javax.tools.*",
})

[mhagnumdw]

Hi @decebals !

All changes are done. Please let me know if you want me to make any changes.

[decebals]

You committed a new .derived file. What is the purpose of this file? I feel that is a misake.

[decebals]

The code looks clean. I think that you can merge this request (always with squash :)). I don't know about file .derived.

[mhagnumdw]

You committed a new .derived file. What is the purpose of this file? I feel that is a misake.

Hi!

Folders/files that are auto-generated can be marked in Eclipse as derived. This causes them to be disregarded by Eclipse, not appearing in the search etc.

This file is from this plugin: https://nodj.github.io/AutoDeriv/

I can remove it without any problems. What do you say?

[decebals]

I can remove it without any problems. What do you say?

Yes. And I think that is a good idea to add a new entry in .gitignore to prevent its addition in the future.