[rf][icedtea-web] fix xdgTests again
Jiri Vanek
jvanek at redhat.com
Wed Apr 22 16:08:41 UTC 2015
On 04/22/2015 05:59 PM, Jie Kang wrote:
>
>
> ----- Original Message -----
>> 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.
>
> Hello~
>
> Awesome!
>
>
> Nits below:
>
>
> +
> +
>
> Could you remove these extra lines?
Sure (Acutally already di, i noted them after emial)
>
> + public static final String TRANSFER_TITLE = "Legacy configuration and cache found. Those will be now transported to new locations";
> +
>
> + //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
>
> s/cration/creation
> s/nondefault/non-default
> s/tahnx/thanks
fixed.
>
>
> diff -r dd4f7ccae35e tests/reproducers/custom/PartiallySignedAppletManifestSpecifiesSandbox/testcases/PartiallySignedAppletManifestSpecifiesSandboxTests.java
> --- a/tests/reproducers/custom/PartiallySignedAppletManifestSpecifiesSandbox/testcases/PartiallySignedAppletManifestSpecifiesSandboxTests.java Mon Apr 20 14:13:08 2015 -0400
> +++ b/tests/reproducers/custom/PartiallySignedAppletManifestSpecifiesSandbox/testcases/PartiallySignedAppletManifestSpecifiesSandboxTests.java Wed Apr 22 16:46:37 2015 +0200
> @@ -38,6 +38,7 @@
>
> Is this part of the patch? I really think it should be a separate patch.
partially.
>
>
> + //this method is currenly unused, each test is setting up its ownn fakeing in legacy or normal config
> + private static DeploymentPropertiesModifier.MultipleDeploymentPropertiesModifier setupDeploymentProperties() throws IOException {
>
> Could you remove this method if it's unused? I don't really see a need to keep it here anymore.
Ok.I will rmeove it. It left bny accident (when I was playing with it)
>
> + private static void itwDoesNotComplainAgain(ProcessResult pr) {
>
> The method name doesn't explain very much. It looks to me like it makes sure that IT-W doesn't transport legacy cache/config. How about renaming it to:
>
> verifyLegacyCacheConfigNotMoved
sounds same to me.. Do you insists?
>
> or something like that?
>
> + private static void itwDoesComplainAboutOldConfig(ProcessResult pr) {
>
> How about
>
> verifyLegacyCacheConfigMoved
Again. Does sounds same ot me. Do you insists?
>
>
> + /**
> + * for advanced users, less verbose, less fool-proof multi-impl
> + */
> + public static class MultipleDeploymentPropertiesModifier {
>
> Since this is a test-extension class, I don't think the comment is needed.
>
> Also, I don't really agree with adding a new class to support modifying multiple deployment properties. To me it'd be much better to just modify the original DeploymentPropertiesModifier class to allow modifying [1, n] properties. Just adding a Map should do the trick.
Well yes. But any object should do the smallest possible work. So you can test individual objects.
not jsut one big object.
From design poin this is much better then adding map.
> As well, is this related to the xdgTests? Along with the changes to PartiallySignedAppletManifestSpecifiesSandboxTests.java I really think this should be it's own patch.
You tyrant :)
Ok. I will push it as two changesets. ok?
>
>
> Thanks for your work here!
>
>
> Regards,
>
>
>>
>> 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.
>>>
>>>>
>>>>
>>>
>>
>>
>
More information about the distro-pkg-dev
mailing list