[rfc][icedtea-web] add DeploymentPropertiesModifierTests

[Jie Kang]

----- 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