Small refactoring in searching for attributes

[Omair Majid]

Hi,

* Jiri Vanek <jvanek at redhat.com> [2013-10-25 06:11]:
> ping?

Sorry, fell off my radar. Comments below.

> -------- Original Message --------
> Subject: Re: Small refactoring in searching for attributes
> Date: Mon, 21 Oct 2013 15:29:01 +0200
> From: Jiri Vanek <jvanek at redhat.com>
> To: Omair Majid <omajid at redhat.com>
> CC: IcedTea Distro List <distro-pkg-dev at openjdk.java.net>
> 
> On 10/18/2013 05:00 PM, Omair Majid wrote:
> >* Jiri Vanek <jvanek at redhat.com> [2013-10-18 09:14]:
> >>Please note, this have to go also to 1.4, and that tests are already included n itw.
> >
> >Patch itself looks okay to me. You have introduced two new public
> >methods, though:
> 
> thanx, pushed.
> 
> >
> >>+    public String checkForAttributeInJars(List<JARDesc> jars, Attributes.Name name) {
> >
> >>+    public String getManifestAttribute(URL location, Attributes.Name  attribute) {
> >
> >Could you add unit tests for these new methods? Just like the previous
> >unit tests give us some guarantee that users of the old methods are
> >still covered, it would be nice to have some guarantee that users are
> >also covered if these new methods were refactored too.
> 
> Here is test. It growe a bit
>   - to test the order during search the dummy jnlp file had to be extended to work with set of jars
> 
> 
> changes ala
> 
> +++ b/netx/net/sourceforge/jnlp/JARDesc.java	Mon Oct 21 15:25:09 2013 +0200
> 
> -    private URL location;
> +    private final URL location;
> 
> ...
> 
> 
> 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 ?

> 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)?

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).

> Also both, or one, or none :) to 1.4 ?

There are no functional changes, right? Sounds okay to me.

> +++ 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).

> +        /* 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

> +            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) ?

> +            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.