[rfc][icedtea-web] add DeploymentPropertiesModifierTests
Jie Kang
jkang at redhat.com
Thu Apr 9 21:00:22 UTC 2015
----- Original Message -----
> Hello,
>
> I have attached an updated patch.
>
> ----- Original Message -----
> > From: "Jie Kang" <jkang at redhat.com>
> > To: "Lukasz Dracz" <ldracz at redhat.com>
> > Cc: "IcedTea Distro List" <distro-pkg-dev at openjdk.java.net>
> > Sent: Wednesday, April 8, 2015 5:17:25 PM
> > Subject: Re: [rfc][icedtea-web] add DeploymentPropertiesModifierTests
> >
> >
> >
> > ----- Original Message -----
> > > Hello,
> > >
> > > This patch adds four tests for DeploymentPropertiesModifierTests.
> >
> > Hello,
> >
> > Nice work!
> >
> > Nits below:
> >
> >
> > Could you also rename the class DeploymentPropetiesModifier to
> > DeploymentPropertiesModifier in this patch?
>
> Yes
>
> > Can you use a temporary file for the tests instead of the user's Deployment
> > Configuration file? This way you can pre-write data to the file, possibly
> > simplify the tests and not mess around with user's file if the class
> > doesn't
> > restore properly.
>
> Yes the temp file is a good idea, thanks for the suggestion ! The only
> problem I had
> was I needed to make InfrastructureFileDescriptor's constructor protected to
> be able to make
> a dummy InfrastructureFileDescriptor which I believe this change or a similar
> change is/will be
> coming in one of Jiri's PathsAndFiles patchs ( I believe its the 4th one ).
> So when those
> patches get pushed this dummy InfrastructureFileDescriptor will need to be
> updated.
Okay.
>
> >
> > + for (String line : properties.split("\n")) {
> > + if (line.contains(DeploymentConfiguration.KEY_SECURITY_LEVEL))
> > {
> > + assertEquals(setProperty, line);
> > + }
> > + }
> >
> > For something like this, a shorter way of writing this would be something
> > along the lines of:
> > assertTrue(properties.contains("\n" + var + "\n");
> >
> >
> > + if (savedLine == null) {
> > + assertFalse(foundLine);
> > + } else {
> > + assertTrue(foundLine);
> > + }
> >
> > Rather than checking the single line, it'd be faster/easier to just check
> > the
> > whole file's contents to match for proper restoration. Or, if you use
> > temporary configuration file setup before the tests, you wouldn't need the
> > null check stuff as well. In general, I feel like there is some
> > over-complication in testSetAndRestoreProperties.
> >
> > I think there could be a few more tests like:
> >
> > testSetProperties :: only sets
> > testRestoreProperties :: only restores and rather than using DPM, manually
> > clobbering the deployment configuration file with stuff :: this test should
> > not need to rely on DPM's set capability working to run
>
> I have split the testSetAndRestoreProperties into two separate tests and
> updated the other tests to use a temp file.
> The only problem with RestoreProperties test is that it requires
> SetProperties, there is no way around that it throws
> an IllegalStateException if setProperties is not performed before
> RestoreProperties. I also added a test to make sure
> this exception is thrown.
Cool. Thanks for this!
The patch looks fine to me.
Regards,
>
>
> > Everything else look fine to me.
>
> Thank you,
> Lukasz Dracz
>
> > Regards,
> >
> > >
> > > Thank you,
> > > Lukasz Dracz
> >
> > --
> >
> > Jie Kang
> >
> > OpenJDK Team - Software Engineering Intern
> >
>
--
Jie Kang
OpenJDK Team - Software Engineering Intern
More information about the distro-pkg-dev
mailing list