[rf][icedtea-web] fix xdgTests again
Jie Kang
jkang at redhat.com
Wed Apr 22 15:59:28 UTC 2015
----- 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?
+ 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
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.
+ //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.
+ 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
or something like that?
+ private static void itwDoesComplainAboutOldConfig(ProcessResult pr) {
How about
verifyLegacyCacheConfigMoved
+ /**
+ * 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.
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.
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.
> >
> >>
> >>
> >
>
>
--
Jie Kang
OpenJDK Team - Software Engineering Intern
More information about the distro-pkg-dev
mailing list