SHRINKWRAP-515 Fix name and JDK5 compliance of AddPackage*Test #109
Conversation
famod
commented
Jul 5, 2017
famod
commented
- suffix must be TestCase to be picked up by surefire
- removed JDK7 stuff
- consolidated by introducing test base class
- worked around file leak (?) on windows (just warn for now)
|
Thanks for your contribution!
Can you be more specific on what has been changed? Is it |
|
|
||
| protected Domain domain; | ||
|
|
||
| private File tempFile; |
There was a problem hiding this comment.
That's a very nice simplification, thanks for cleaning it up! I was wondering if we couldn't simply use TemporaryFolder rule from JUnit to have this auto-cleanup done on our behalf?
There was a problem hiding this comment.
Well, basically we could. But with the current file leak on windows (which prevents deletion anyway) there would not be a warning like with my current approach.
There was a problem hiding this comment.
Can you elaborate under which circumstances the leak occurs?
There was a problem hiding this comment.
No special circumstances really. I traced it back to the .addPackage(...) call in the test method. Once I remove this call (which renders the test pretty useless, though) the .tearDown() method does delete the file just fine.
In the beginning I thought that the URLClassLoader needs to be closed explicitly but firstly this is a JDK7+ feature and secondly this does not change anything (tried it briefly with JDK5_HOME pointing to JDK8).
Actually I was going to file another bug ticket anyway. WDYT?
There was a problem hiding this comment.
Please do so, but this makes me wonder what locks this file though. Do you have Linux to check if we are facing the same issue there?
There was a problem hiding this comment.
I just tested this on CentOS7 (via VirtualBox) and the problem does not occur there!
It is interesting to see that the problem does occur on Linux when the project resides on a virtualbox shared folder (which is a Windows folder "physically") but there is no problem when the project resides entirely within the Linux VM.
| public abstract class AddPackageTestBase { | ||
|
|
||
| /** | ||
| * Logger |
There was a problem hiding this comment.
This is just using 3 lines for not that much value :) Let's kick it out :)
There was a problem hiding this comment.
The Logger JavaDoc? Ok, will do. I just copied it from another class.
There was a problem hiding this comment.
I know, but we can boy scout here - it's just a noise :)
Yes, and I had to replace |
- suffix must be TestCase to be picked up by surefire - removed JDK7 stuff - consolidated by introducing test base class - worked around file leak (?) on windows (just warn for now)
|
@bartoszmajsak I removed the Logger JavaDoc. I suggest we take another stab at |
That makes perfect sense. Thanks for your contribution. |
