Regression in itw from Tue Mar 26

[Adam Domurad]

On 04/25/2013 04:18 AM, Jiri Vanek wrote:
> On 04/24/2013 06:41 PM, Adam Domurad wrote:
>> On 04/24/2013 12:37 PM, Jiri Vanek wrote:
>>> On 04/24/2013 06:34 PM, Adam Domurad wrote:
>>>>> +    @Test
>>>>> +    public void testNotNullJnlpFile(){
>>>>> +        SecurityDesc securityDesc = new SecurityDesc(new 
>>>>> DummyJNLPFile(),
>>>>> SecurityDesc.SANDBOX_PERMISSIONS, "hey!");
>>>>> +        Assert.assertNotNull("securityDesc should not be 
>>>>> ",securityDesc);
>>>>> +
>>>>> +    }
>>>>
>>>> Huh. Why is this still in ? Sorry, I can't support superstitious 
>>>> coding.
>>>>
>>>> At least drop the assert if you want a constructor sanity check.
>>>
>>> No. I would like to keep it inside even with  assert.
>>> The code inside the constructor can change later. Eg some dummy jnl 
>>> file will be tried to be
>>> created and used into if argument is null.
>>>
>>> I would like to have it recorded.
>>
>> Sorry, but this assert is really a no-op. How could the result of 
>> 'new' ever be null ?
>
> No ists not "no-op"

Yes it is.

>
> It is testing that securityDesc was filled. Whta is somthing what will 
> not happened when exception is thrown from constructor.

It is testing nothing because if an exception happened it would not 
occur by definition.

> @Test
>     public void testNotNullJnlpFile(){
> Exception ex = null;
> try{
>         SecurityDesc securityDesc = new SecurityDesc(new 
> DummyJNLPFile(), SecurityDesc.SANDBOX_PERMISSIONS, "hey!");
> }catch(Exception exx){
> ex=exx;
> }
>         Assert.assertNull("no exception expectd ",ex);
>
>     }

This is good.

>
> It is more verbose the same as what I'm testing.

But more explicit. What you wrote is equivalent to this:

@Test
public void testNotNullJnlpFile(){
      Assert.assertNotNull( new SecurityDesc(new DummyJNLPFile(), 
SecurityDesc.SANDBOX_PERMISSIONS, "hey!") );
}

And it is *completely* equivalent to this:

@Test
public void testNotNullJnlpFile(){
      new SecurityDesc(new DummyJNLPFile(), 
SecurityDesc.SANDBOX_PERMISSIONS, "hey!");
}

The new operator will never return null.
This is not more verbose. Add a comment and you have your 'exception 
check', because of the nature of JUnit.

>
> What will be in "t"  and what in "tt" atetr main execution?
> And .. If there "will be something"" will it be "working"?

The fatal error in this class is leaking an incomplete 'this'. Even if 
this code could do this, I don't see the relevance.

>
> As solution you can see the result  or run.. But solution is probably 
> obvious for you (no ironic, you know already java better then me).

I know the dark parts well 8-)

Push without this please, or the version that explicitly catches the 
exception. Otherwise take it up on IRC :-)

Happy hacking,
-Adam