[rfc][icedtea-web][policyeditor] CustomPolicyEditor method extractions and tests added

[Andrew Azores]

On 06/06/2014 03:11 PM, Andrew Azores wrote:
> On 06/06/2014 02:51 PM, Jie Kang wrote:
>>
>> ----- Original Message -----
>>> On 06/06/2014 11:22 AM, Lukasz Dracz wrote:
>>>> ----- Original Message -----
>>>>> From: "Andrew Azores" <aazores at redhat.com>
>>>>> To: "IcedTea" <distro-pkg-dev at openjdk.java.net>
>>>>> Sent: Friday, June 6, 2014 10:49:38 AM
>>>>> Subject: Re: [rfc][icedtea-web][policyeditor] CustomPolicyEditor 
>>>>> method
>>>>>     extractions and tests added
>>>>>
>>>>> On 06/06/2014 10:47 AM, Andrew Azores wrote:
>>>>>> Hi,
>>>>>>
>>>>>> In the same vein as the previous patch to PolicyEditor which 
>>>>>> extracted
>>>>>> some methods to allow unit testing, this attached patch does the 
>>>>>> same
>>>>>> for CustomPolicyViewer. Additionally, CustomPolicyViewer no longer
>>>>>> holds a reference of its own to the PolicyFileModel, instead using
>>>>>> appropriate methods exposed through its parent PolicyEditor.
>>>>>> CustomPolicyViewer now also uses CustomPolicy.fromString() rather 
>>>>>> than
>>>>>> its own "parsing" solution, and also no longer allows duplicate
>>>>>> permissions to be added to the user interface (which was a bug since
>>>>>> the actual collection modelling this is a Set anyway, so the 
>>>>>> JList and
>>>>>> Set could become out of sync).
>>>>>>
>> Hello,
>>
>> Currently testAddCustomPermissionDuplicates calls 
>> testAddCustomPermission in order to add a base permission. Though it 
>> is convenient I don't think this is good practice. Everything else 
>> looks fine.
>>
>>
>>>>>> Thanks,
>>>>>>
>>>>> Sorry, in the subject line that should be CustomPolicy*Viewer*, not
>>>>> CustomPolicyEditor.
>>>>>
>>>>> Thanks,
>>>>>
>>>>> -- 
>>>>> Andrew A
>
> Yea, I wasn't too happy with it either. The adding of a permission on 
> its own is not enough to extract into a method, it needs to be done 
> along with the asserts to be anything useful. I don't want to have the 
> asserts in a non- at Test method really, so what's left is either calling 
> a test from within other tests, or just having duplicate code. The 
> attached patch lets the duplicate code be rather than calling another 
> @Test method from within tests.
>
> Thanks,
>

Ping.

Thanks,

-- 
Andrew A