[rf][icedtea-web] fix xdgTests again

Wed Apr 22 13:49:20 UTC 2015

On 04/21/2015 11:21 PM, Jie Kang wrote:
>
>
> ----- Original Message -----
>> Hi!
>>
>> So the xdgtest startet to faila again.
>>
>> Mainly because of change in ignorance of attributes from true/false to enum.
>> To this the change in XDGspecificationTests is related (rest is small java7
>> liek refactoring).
>>
>> However,it appeared that this is not enough, that setupable
>> InfrastrucureDescriptors did it small
>> evil hit there.
>> They caused, that DeploymentConfiguration singleton was created suddnely in
>> the middle of
>> files/directories move. That means, that it was created *before*
>> deployment.properties were moved,
>> co it actually caused lost of this data. The fix was obvious, move moving of
>> deployment.properties
>> to the begginig. So it moves, and then singleton can be created any time...
>> later.
>>
>> Thats  v1.patch.
>>
>> Then I realized, that what if legacy deployment.properties contains some
>> custom values, they are
>> actually lost anyway. So I started to write v2.patch monstrosity
>> It is based on (good idea) to load DeploymentConfiguration legacy  deployment
>> configuration as first
>> step, and then use that getFullPath(differentConfig) mechanism. All went fine
>> and worked, except
>> fact, that the custom files actually dont need to be handled like that!
>> Because once they are custom-set  they do not need to be migrated.
>> DeploymentConfiguration (loaded
>> from copied deployment.configuration) will find them correctly.
>>
>> So I reverted most the changes in  DeploymentConfiguration, but kept few
>> nioce refactorings which
>> were result of v2. So v3 is v1 + few refactorngs which I conisdered as ok.
>>
>> So v3 is the one which I wont to push.
>>
>>
>> But.... During this email, I started to feel like v2 approach really cold be
>> the best one. But
>> considering that no one should be running on itw1.4 now, I would like to
>> touch this code as few as
>> possible. Everybody should be already on XDG and I relly do not won to spam
>> users by the warning
>> that both legacy and new config exists together.
>>
>>
>> Thanx for bringing me out of this maze!
>
> Hello Jiri,
>
>
> Thanks for the work here!
>
> When comparing v2 and v3, I prefer v3 since it's less invasive. However v2 was easier to understand for me. For you, what are the benefits for v2 over v3?

Issue with v2 is that it is probably nonsense and is bringing in complete useless and confusing 
logic. (but I posted it because of fear that it actually may be right way (today after sleeping, I 
think it really is useless)

The main isssue is, that config singleton must be initialised *after* the deployment.properties is 
moved from legacy to new destination.

If it is loaded before, then the deployment.properties don't exists in time of reading or is somehow 
invalid.

So my v2 workaround was to create false config from legacy deployment.properties and drop it 
afterwards. But if deployment.properties is moved before regular config creation then everything 
should be much better then  with false loaded config.

Is this description of moveLegacyToCurrnt valid? :
move all defautl files to new default locations.
If they are setUpByUser to different  location in deployment.properties, then dont touch them, 
because config is created after move of deployment properties so it contains all necessary paths.

If this description is valid, and this method do this, then there is no more to do. Or is this 
description invalid?

>
>
> Below is patch review nits for v3:
>
> +    /** source of true location */
>
> Can this comment be expanded? I can't tell what you mean by 'true' location.

"Source of always right and only path to file (even if underlying path changes)"

>
> +    private final InfrastructureFileDescriptor userPropertiesFileDescriptor;

ok.

>
> I think the name could be changed to not be confused with PropertiesFile objects.
>
> s/userPropertiesFileDescriptor/userDeploymentFileDescriptor ?

ok!

>
> +     public DeploymentConfiguration(InfrastructureFileDescriptor usedConfigFile) {
>
> Here as well, the argument name 'usedConfigFile' is a bit weird. s/usedConfigFile/configFile ?\

ok:)

>
> +            //move this first, the cration of config singleton may happen anytime...(but hsould not in this block)
>
> s/cration/creation
> s/hsould/should

sure!
>
> One question I have is; for the creation of config singleton, can't we force it to happen when we want by making a deliberate call beforehand? That way it won't be 'anytime' but, before this block is executed. Or did you want the config singleton to be created afterwards? Can you explain this part a little more for me? Sorry;;

Sure :)

it should be answered in first paragraph

>
>
> diff -r dd4f7ccae35e tests/reproducers/simple/simpletest1/testcases/XDGspecificationTests.java
> --- a/tests/reproducers/simple/simpletest1/testcases/XDGspecificationTests.java	Mon Apr 20 14:13:08 2015 -0400
> +++ b/tests/reproducers/simple/simpletest1/testcases/XDGspecificationTests.java	Tue Apr 21 17:21:58 2015 +0200
> @@ -90,7 +90,7 @@
>
> For this set of tests, when I run them they fail because Permissions attribute isn't set, which launches a dialog causing applet not to launch. Shouldn't there be a before/after class to set the 'deployment.manifest.attributes.check' attribute to 'NONE' using the new DeploymentPropertiesModifier class?

Ok thats bad[1]. I run them with various startup configurations, and they always run. I also made 
various modifications and then undo it all. Always worked.
You may see that the tests are faking environemnt on start of each test.  It is not in 
before/afterclass only because each test needs it a bti differently (legacy/no-legacy with/without 
xdg....)

[1] if it fails for you, it means that my fix do not work (but it works for me!). Can you try with 
the correct default settings? (or bettter with proper beforeclass/afterclass in new patch.

Also, I wonted to add tests to ensure that security files are correctly handled and that once the 
moving is done, ITW do not try to move it again. But I found they already do so :)

So littl ebit messy patch (v3_2) atached .
J.

>
>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: fixXDGtests-v3_2.patch
Type: text/x-patch
Size: 41162 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20150422/b942bc0f/fixXDGtests-v3_2-0001.patch>