[rfc][icedtea-web] additions to test-extensions needed for the new AWTFramework
Jana Fabrikova
jfabriko at redhat.com
Fri Dec 14 08:14:34 PST 2012
Thank you for your comments, I hope I have improved the code a little,
modified patch is attached to this mail,
Jana
On 12/13/2012 11:21 AM, Jiri Vanek wrote:
> On 12/13/2012 11:03 AM, Jana Fabrikova wrote:
>> 2012-12-13 Jana Fabrikova <jfabriko at redhat.com>
>>
>> * /tests/test-extensions/net/sourceforge/jnlp/ServerAccess.java:
>> Added several new versions of method executeBrowser with lists
>> of ContentReaderListeners as arguments.
>
> It is common to use ..ew versions of (executeBrowser) with lis...
> parentheses which measn method/target...
>>
>> * /tests/test-extensions/net/sourceforge/jnlp/ProcessWrapper.java:
>> Added new versions of the constructor and methods for adding
>> ContentReaderListeners using List<ContentReaderListener>
>> instead of one ContentReaderListener.
>> Added a new version of constructor with less arguments (used
>> in case when several arguments would be passed as null, thus
>> causing ambiguity).
>
> same
>>
>> *
>> /tests/reproducers/simple/SingeInstanceServiceTest/testcases/SingleInstanceTest.java:
>>
>> Modified the call of executeBrowser method with null arguments
>> into a call of new method without the null arguments
>> (getting rid of an ambiguous call).
>
> same
>
>>
>>
>> I would like to ask for review of the attached patch,
>> thank you,
>> Jana
>>
>> AWTFramework_preparations.patch
>>
>>
>> diff --git
>> a/tests/reproducers/simple/SingleInstanceServiceTest/testcases/SingleInstanceTest.java
>> b/tests/reproducers/simple/SingleInstanceServiceTest/testcases/SingleInstanceTest.java
>>
>> ---
>> a/tests/reproducers/simple/SingleInstanceServiceTest/testcases/SingleInstanceTest.java
>>
>> +++
>> b/tests/reproducers/simple/SingleInstanceServiceTest/testcases/SingleInstanceTest.java
>>
>> @@ -109,7 +109,7 @@
>> try {
>> boolean isJavawsTest = isJnlp(launchFile);
>> pr = isJavawsTest ?
>> server.executeJavawsHeadless(launchFile, null, null)
>> - : server.executeBrowser(launchFile, null, null);
>> + : server.executeBrowser(launchFile);
>> } catch (Exception ex) {
>> ServerAccess.logException(ex);
>> } finally {
>> diff --git
>> a/tests/test-extensions/net/sourceforge/jnlp/ProcessWrapper.java
>> b/tests/test-extensions/net/sourceforge/jnlp/ProcessWrapper.java
>> --- a/tests/test-extensions/net/sourceforge/jnlp/ProcessWrapper.java
>> +++ b/tests/test-extensions/net/sourceforge/jnlp/ProcessWrapper.java
>> @@ -64,6 +64,20 @@
>> public ProcessWrapper() {
>> }
>>
>> + public ProcessWrapper(String toBeExecuted, List<String>
>> otherargs, URL u){
>> + Assert.assertNotNull(u);
>> + Assert.assertNotNull(toBeExecuted);
>> + Assert.assertTrue(toBeExecuted.trim().length() > 1);
>> + if (otherargs == null) {
>> + otherargs = new ArrayList(1);
>> + }
>> + List<String> urledArgs = new ArrayList(otherargs);
>> + urledArgs.add(0, toBeExecuted);
>> + urledArgs.add(u.toString());
>> + this.args = urledArgs;
>> + this.vars=null;
>> + }
>> +
>
> This smell with doubled code. Can you extract all the duplicate parts?
>
>
>> public ProcessWrapper(String toBeExecuted, List<String>
>> otherargs, URL u, ContentReaderListener stdoutl, ContentReaderListener
>> stderrl, String[] vars) throws Exception {
>> Assert.assertNotNull(u);
>> Assert.assertNotNull(toBeExecuted);
>> @@ -81,6 +95,22 @@
>>
>> }
>>
>> + public ProcessWrapper(String toBeExecuted, List<String>
>> otherargs, URL u, List<ContentReaderListener> stdoutl,
>> List<ContentReaderListener> stderrl, String[] vars) throws Exception {
>> + Assert.assertNotNull(u);
>> + Assert.assertNotNull(toBeExecuted);
>> + Assert.assertTrue(toBeExecuted.trim().length() > 1);
>> + if (otherargs == null) {
>> + otherargs = new ArrayList(1);
>> + }
>> + List<String> urledArgs = new ArrayList(otherargs);
>> + urledArgs.add(0, toBeExecuted);
>> + urledArgs.add(u.toString());
>> + this.args = urledArgs;
>> + this.addStdOutListeners(stdoutl);
>> + this.addStdErrListeners(stderrl);
>> + this.vars=vars;
>> + }
>
> This smells with similar duplication.
>
>> +
>> ProcessWrapper(final List<String> args, File dir,
>> ContentReaderListener stdoutl, ContentReaderListener stderrl, String[]
>> vars) {
>> this.args = args;
>> this.dir = dir;
>> @@ -89,6 +119,14 @@
>> this.vars = vars;
>> }
>>
>> + public ProcessWrapper(final List<String> args, File dir,
>> List<ContentReaderListener> stdoutl, List<ContentReaderListener>
>> stderrl, String[] vars) {
>> + this.args = args;
>> + this.dir = dir;
>> + this.addStdOutListeners(stdoutl);
>> + this.addStdErrListeners(stderrl);
>> + this.vars = vars;
>> + }
>> +
>> public final void addStdOutListener(ContentReaderListener l) {
>> if (l == null) {
>> return;
>> @@ -105,6 +143,22 @@
>>
>> }
>>
>> + public final void addStdOutListeners(List<ContentReaderListener>
>> l) {
>> + if (l == null) {
>> + return;
>> + }
>> + stdoutl.addAll(l);
>> +
>> + }
>> +
>> + public final void addStdErrListeners(List<ContentReaderListener>
>> l) {
>> + if (l == null) {
>> + return;
>> + }
>> + stderrl.addAll(l);
>> +
>> + }
>> +
>> /**
>> * @return the args
>> */
>> diff --git
>> a/tests/test-extensions/net/sourceforge/jnlp/ServerAccess.java
>> b/tests/test-extensions/net/sourceforge/jnlp/ServerAccess.java
>> --- a/tests/test-extensions/net/sourceforge/jnlp/ServerAccess.java
>> +++ b/tests/test-extensions/net/sourceforge/jnlp/ServerAccess.java
>> @@ -573,6 +573,10 @@
>> return executeBrowser(getBrowserParams(), resource, stdoutl,
>> stderrl);
>> }
>>
>> + public ProcessResult executeBrowser(String resource,
>> List<ContentReaderListener> stdoutl, List<ContentReaderListener>
>> stderrl) throws Exception {
>> + return executeBrowser(getBrowserParams(), resource, stdoutl,
>> stderrl);
>> + }
>> +
>> /**
>> * wrapping method to executeProcess (eg: javaws arg arg
>> http://localhost:port/resource)
>> * will execute default javaws (@see JAVAWS_BUILD_BIN) upon
>> default url upon cached server (@see SERVER_NAME @see getPort(), @see
>> getInstance()))
>> @@ -592,7 +596,7 @@
>> }
>>
>> public ProcessResult executeBrowser(List<String> otherargs,
>> String resource) throws Exception {
>> - ProcessWrapper rpw = new ProcessWrapper(getBrowserLocation(),
>> otherargs, getUrlUponThisInstance(resource), null, null, null);
>> + ProcessWrapper rpw = new ProcessWrapper(getBrowserLocation(),
>> otherargs, getUrlUponThisInstance(resource));
>> rpw.setReactingProcess(getCurrentBrowser());//current
>> browser may be null, but it does not metter
>> return rpw.execute();
>> }
>> @@ -603,8 +607,16 @@
>> return rpw.execute();
>> }
>>
>> + public ProcessResult executeBrowser(List<String> otherargs,
>> String resource, List<ContentReaderListener> stdoutl,
>> List<ContentReaderListener> stderrl) throws Exception {
>> + ProcessWrapper rpw = new ProcessWrapper(getBrowserLocation(),
>> otherargs, getUrlUponThisInstance(resource), stdoutl, stderrl, null);
>> + rpw.setReactingProcess(getCurrentBrowser());// current
>> browser may be
>> + // null,
>> but it does not
>> + // matter
>> + return rpw.execute();
>
> What an nasty indentation. Perhaps you have ide configured to use tabs
> as tabs? (should be tabs as spaces, I think tab is 4sapces by default
> for ITW)
> But maybe just email transformation messed it up. Then please ignore
> this...
>> + }
>> +
>> public ProcessResult executeBrowser(Browser b, List<String>
>> otherargs, String resource) throws Exception {
>> - ProcessWrapper rpw = new ProcessWrapper(b.getBin(),
>> otherargs, getUrlUponThisInstance(resource), null, null, null);
>> + ProcessWrapper rpw = new ProcessWrapper(b.getBin(),
>> otherargs, getUrlUponThisInstance(resource));
>> rpw.setReactingProcess(b);
>> return rpw.execute();
>> }
>> @@ -615,6 +627,12 @@
>> return rpw.execute();
>> }
>>
>> + public ProcessResult executeBrowser(Browser b, List<String>
>> otherargs, String resource, List<ContentReaderListener> stdoutl,
>> List<ContentReaderListener> stderrl) throws Exception {
>> + ProcessWrapper rpw = new ProcessWrapper(b.getBin(),
>> otherargs, getUrlUponThisInstance(resource), stdoutl, stderrl, null);
>> + rpw.setReactingProcess(b);
>> + return rpw.execute();
>> + }
>> +
>> /**
>> * Create resource on http, on 'localhost' on port on which this
>> cached instance is running
>> * @param resource
>> @@ -661,7 +679,7 @@
>> * @throws Exception
>> */
>> public static ProcessResult executeProcessUponURL(String
>> toBeExecuted, List<String> otherargs, URL u) throws Exception {
>> - return new ProcessWrapper(toBeExecuted, otherargs, u, null,
>> null, null).execute();
>> + return new ProcessWrapper(toBeExecuted, otherargs, u).execute();
>> }
>>
>> public static ProcessResult executeProcessUponURL(String
>> toBeExecuted, List<String> otherargs, URL u, ContentReaderListener
>> stdoutl, ContentReaderListener stderrl) throws Exception {
>>
>
> Otherwise the changes have sense. Please double check that all the
> affected codes by small refactoring you included in this patch have no
> impact to compilation/runtime of reproducers!
>
>
> Thankx for dooing this
> J.
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: AWTFramework_preparations2.patch
Type: text/x-patch
Size: 7090 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20121214/3d05aa0c/AWTFramework_preparations2.patch
More information about the distro-pkg-dev
mailing list