[rfc][icedtea-web] add DeploymentPropertiesModifierTests

[Lukasz Dracz]

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.

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


> Everything else look fine to me.

Thank you,
Lukasz Dracz
 
> Regards,
> 
> > 
> > Thank you,
> > Lukasz Dracz
> 
> --
> 
> Jie Kang
> 
> OpenJDK Team - Software Engineering Intern
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: deploymentPropertiesModifierTests-3.patch
Type: text/x-patch
Size: 16612 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20150409/a29a62e5/deploymentPropertiesModifierTests-3-0001.patch>