[rf][icedtea-web] fix xdgTests again

Wed Apr 22 14:49:12 UTC 2015

Ok.

Thanx to jie, I foud one more error. I mixed up usage of default/set paths.
At least I made myself clear, that only *default* patsh are worthy of copying.

See:

+            //note - all here use default path. Any call to getFullPAth will invoke cration of 
config singleton
+            // but: we DO copy only defaults. There is no need to copy nondefaults!
+            // nondefault will be used tahnx to config singleton read from copied deployment.properties

Hopefuly final patch atached.

J.
On 04/22/2015 03:49 PM, Jiri Vanek wrote:
> 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_3.patch
Type: text/x-patch
Size: 42180 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20150422/fd71b39d/fixXDGtests-v3_3-0001.patch>