[rfc][icedteaweb] Add comments about permission attributes not being checked in reproducers

[Jiri Vanek]

On 03/05/2015 10:52 PM, Lukasz Dracz wrote:
> Hello,
>
> ----- Original Message -----
>> From: "Jiri Vanek" <jvanek at redhat.com>
>> To: "Lukasz Dracz" <ldracz at redhat.com>, "IcedTea Distro List" <distro-pkg-dev at openjdk.java.net>
>> Sent: Thursday, March 5, 2015 2:48:25 AM
>> Subject: Re: [rfc][icedteaweb] Add comments about permission attributes not being checked in reproducers
>>
>> On 03/04/2015 10:00 PM, Lukasz Dracz wrote:
>>> Hello,
>>>
>>> I have been looking into why certain reproducers are failing. For the
>>> following tests
>>>
>>> testPartiallySignedAppletWithSandboxPermissionsInManifestLaunchWithSignedHTMLApp
>>> testPartiallySignedJNLPAppletWithSandboxPermissionsInManifestLaunchWithSignedApp
>>> testSignedAppletWithSandboxPermissionsInManifestHtml
>>> testSignedAppletWithSandboxPermissionsInManifestHtmlJnlpHref
>>> testSignedAppletWithSandboxPermissionsInManifestJnlpApplet
>>> testSignedAppletWithSandboxPermissionsInManifestJnlpApplication
>>>
>>> I found that changeset 1129:0284eb954ebc is reason.
>>>
>>> The reason why is that the permissions attributes check has been moved from
>>> always being checked into an if statement
>>> where they are only checked when deployment.manifest.attributes.check is
>>> set to true. I have tested and found the tests to pass when this
>>> deployment.manifest.attributes.check is set to true.
>>>
>>> This patch just adds comments to the tests for reference to help explain to
>>> anyone in the future looking at the tests. At the moment I don't have a
>>> solution, does anyone have an idea on how to properly fix this or handle
>>> this ?
>>> I had tried to set the property to true for the tests and revert it back
>>> after to no avail. I will try to think more on this problem.
>>>
>>
>> Hi!
>>
>> So yes - the testsuite is running with
>> deployment.manifest.attributes.check=false.
>>
>> IMHO best would be to tune it to run without any
>> deployment.manifest.attributes.check.
>>
>>
>> For your group of tests - I think the best way to go is :
>> @beforeclass ensure that no deployment.manifest.attributes.check is present
>> in
>> deploymnet.properties. If it is, remove it.
>> @afterclass return it back, if it was here.
>>
>>
>> What do you think?  Note - tehre is plenty util methods which loads file to
>> string or save string to
>> file. Just do not add more :)
>
> I have tested the @beforeclass @afterclass method of editing the deployment.properties file and well it works somewhat, some of the tests pass now
> but the other ones will prompt a pop-up and if you click to proceed it passes but obviously failing to click proceed it fails.
Maybe instead of beforeclass and after class use this setup only for specified tests?

>
> This defeats the purpose of the manifest attributes check being set to false.
> I can't seem to think of a solution I would like.
yes:(
> I suppose we could split the deployment.manifest.attributes.check into two properties, by adding a new one that decides whether to check the Permissions Attribute and the default deployment.manifest.attributes.check would check all the other attributes.
>
> For the test we would edit the manifest like you mentioned with @beforeclass to change the deployment.manifest.permissions.attributes.check to true and keep deployment.manifest.attributes.check false.
> What do you think about this solution ?
> Do you have any other ideas ?

The only other idea of mine is to make deployment.manifest.attributes.check  tri state (enum TRUE 
FALSE PERMISSIONS_ONLY) instead of boolean  an new attribute - true (all are tests) false (none is 
tested) all_but_permissions - then only permission s attribnute is check.

The deployment.manifest.attributes.check is itw Internal, so we can do moreover whatever we need 
with it.  And the code is documentation itself .

imho - the tristate is better to impl. maintain read. But instead of it, go with any option you wont 
to fix those tests.
>
> I suppose we could also try to make it possible to edit the property directly and having that change updated when done. I would need to look into how feasible this is.

Not sure If I understood:(
>
> Thoughts ?
>
> Thank you,
> Lukasz Dracz
>
>
>
>
>> Thank you!
>>
>> J.
>>
>>
>>
>>
>>
>>
>>