Small refactoring in searching for attributes

[Jiri Vanek]

On 10/26/2013 03:53 PM, Omair Majid wrote:
...
>>
>> ...
>>
>>
>> Are a bit unrelated, but worthy.
>
> Yup. This is actually very nice to have! I think all the fields in JARDesc
> are now final. Can you mark the class (maybe in the javadoc) as
> immutable/thread-safe ?

Thanx, mentioned and pushed as separate chngeset.
>
>> I wonted to write setter for "main" field, and realised how wrong
>> this class is. So I did not made the api dirty (used refelction in dummyJnlp) and adde those finals.
>
> I have mixed feelings about this. If you need to peek at a field to find
> out that this class is working correctly, maybe that fields needs to be
> public (or accessible through a getter/setter)?

Nope. But I fixed it. Now the "main" is set in constructor so it is as it should be. JArdesc 
immutability is correct, and field is set during construction. Ideal. This is best on review 
process. One is forced to read patch at least one more times :)

>
> The way I see it, the unit tests are verifying the API/functionality. If
> something is not visible externally (at all), does it really have any
> purpose? Does it need to be tested?
>
>> Ok for head (both patch and fix?)
>
> The patch was okay. It's just that I generally put fix and test in one
> commit (because creating a good test often exposes deficiencies in the
> fix).

immutable jarDesc pushed to 1.4 as well.
>
>> Also both, or one, or none :) to 1.4 ?
>
> There are no functional changes, right? Sounds okay to me.

correct.
>
>> +++ b/tests/netx/unit/net/sourceforge/jnlp/runtime/JNLPClassLoaderTest.java	Mon Oct 21 15:25:09 2013 +0200
>
>> +    public void getCustomAtributes() throws Exception {
>> +        File tempDirectory = FileTestUtils.createTempDirectory();
>> +        File jarLocation = new File(tempDirectory, "testX.jar");
>
> Can you decouple the unit test from the file system? The more this unit
> test does, the less useful it becomes as a 'unit' test (since the 'unit'
> now becomes 3 large classes and the file system).

Sorry, not in time to investiage this. So no right now ;(
>
>> +        /* Test with attributes in manifest */ {
>
> When you feel tempted to write comments like this, it's an indication
> that it should be a separate unit test :)
>
>> +      @Test
>> +    public void checkOrderWhenReadingAttributes() throws Exception {
>> +        File tempDirectory = FileTestUtils.createTempDirectory();
>> +        File jarLocation5 = new File(tempDirectory, "test5.jar");
>> +
>> +        /* Test with main-class in manifest */ {
>> +            Manifest manifest1 = new Manifest();
>> +            manifest1.getMainAttributes().put(Attributes.Name.MAIN_CLASS, "DummyClass1"); //two times, but one in main jar, see jnlpFile.setMainJar(3);
>
> Something is wrong in the indentation here. Please following the coding
> style: http://icedtea.classpath.org/wiki/IcedTea-Web#Code_style

restyled

>
>> +            Manifest manifest2 = new Manifest();
>> +            manifest2.getMainAttributes().put(Attributes.Name.IMPLEMENTATION_VENDOR, "rh1"); //two times, both in not main jar, see jnlpFile.setMainJar(3);
>> +
>> +            Manifest manifest3 = new Manifest();
>> +            manifest3.getMainAttributes().put(Attributes.Name.IMPLEMENTATION_TITLE, "it"); //jsut once in not main jar, see jnlpFile.setMainJar(3);
>
> What's setMainJar(3) ?

There is no such method :P (in new version :)

>
>> +            assertNoFileLeak(new Runnable() {
>
> Not leaking files is good, but it's better to not use files in unit
> tests.
>
>> +++ b/tests/test-extensions/net/sourceforge/jnlp/mock/DummyJNLPFileWithJar.java	Mon Oct 21 15:25:09 2013 +0200
>
>> +    public void setMainJar(int i) throws Exception {
>> +        Field field = JARDesc.class.getDeclaredField("main");
>> +        Field modifiersField = Field.class.getDeclaredField("modifiers");
>> +        modifiersField.setAccessible(true);
>> +        modifiersField.setInt(field, field.getModifiers() & ~Modifier.FINAL);
>> +        modifiersField.setInt(field, field.getModifiers() & ~Modifier.PRIVATE);
>> +        modifiersField.setInt(field, Modifier.PUBLIC);
>> +        field.set(jarDescs[i], true);
>
> Uhm. Please consider a different implementation.
Yah, really nasty from me. Fixed (set in constructor)



Thnx for eyabl. Should e much better now!

J.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: testForSearchForAttribute2.patch
Type: text/x-patch
Size: 10894 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20131030/b8680e00/testForSearchForAttribute2.patch