ping: [FYI][icedtea-web] minor changes in reproducers engine

[Omair Majid]

On 06/24/2011 05:21 AM, Jiri Vanek wrote:
> changelog:
>
>      *tests/netx/jnlp_testsengine/net/sourceforge/jnlp/ServerAccess.java: String containing "localhost"  have been declared as final constant.
>      (SERVER_NAME) have been  moved instant Server instance so each
> server can have it name without affecting others
>      (getUrl()) added -  can return URL of server singleton.
> Implementation of this method is inside server, so each server can
> return its own useful URL.
>      (saveFile()) is now public.
>      Added identification for ThreadedProcess based on commandlineArgs
> and its run is now slowed by Thread.sleep
>      (ServerLuncher) inner class is now public (it was bug to not be as
> we have getIndependentInstance of it method )
>      Enchanted wrapping of executeProcess
>
>

Comments in-line below.

> +
> +    /**
> +     *
> +     * @return port on which is runing cached server. If non singleton instance is runnig, new is created.
> +     */

Javadoc says port, but the method is named getUrl :/

> +    public URL getUrl(String resource) throws MalformedURLException {
> +        if (server == null) {
> +            getInstance();
> +        }
> +        //if (!server.isRunning()) throw new RuntimeException("Server mysteriously died");
> +        return server.getUrl(resource);
> +
> +    }
> +    /**
> +     *
> +     * @return port on which is runing cached server. If non singleton instance is runnig, new is created.
> +     */
> +    public URL getUrl() throws MalformedURLException {
> +        if (server == null) {
> +            getInstance();
> +        }
> +        //if (!server.isRunning()) throw new RuntimeException("Server mysteriously died");
> +        return getUrl("");
> +
> +    }

You might be able to get away with just getUrl(""); getUrl(String) 
already does the checks.

> @@ -566,6 +591,7 @@
>           List<String>  args;
>           Integer exitCode;
>           Boolean running;
> +        String commandLine="unknown command";
>
>           public Boolean isRunning() {
>               return running;
> @@ -578,8 +604,21 @@
>           public ThreadedProcess(List<String>  args) {
>
>               this.args = args;
> +            if (args!=null&&  args.size()>0){
> +                commandLine="";
> +                for (Iterator<String>  it = args.iterator(); it.hasNext();) {
> +                    String string = it.next();
> +                    commandLine=commandLine+" "+string;
> +
> +                }
> +            }
>           }
>
> +        public String getCommandLine() {
> +            return commandLine;
> +        }
> +
> +
>           public Process getP() {
>               return p;
>           }

commandLine has a large overlap with args. It is also not used - the 
runtime.exec() call still uses args. Wouldn't it make more sense to 
create the commandLine string in getCommandLine()?

> @@ -609,12 +648,27 @@
>        * to allow terminations and stuff arround.
>        */
>
> -    static class ServerLuncher implements Runnable {
> +    public static class ServerLuncher implements Runnable {
>

If you are fixing this, you might want to rename it to ServerL*a*uncher 
- unless it actually eats servers for lunch :)

> @@ -778,23 +842,26 @@
>                       //System.out.println(time - startTime);
>                       //System.out.println((time - startTime)>  timeout);
>                       if ((time - startTime)>  timeout) {
> -                        System.err.println("Timeouted " + p.toString() + " .. killing");
> +                        System.err.println("Timeouted " + p.toString() +" "+ p.getP().toString()+" .. killing "+p.getCommandLine()+": ");
>                           System.err.flush();
>                           wasTerminated = true;
>                           p.interrupt();
>                           while (!terminated.contains(p)) {
>                               Thread.sleep(100);
>                           }
> -                        System.err.println("Timeouted " + p.toString() + " .. killed");
> +                        System.err.println("Timeouted " + p.toString() +" "+ p.getP().toString()+ " .. killed "+p.getCommandLine());
>                           System.err.flush();
>                           break;
>
>
>                       }
> +                    Thread.sleep(100);
>                   } catch (Exception ex) {
>                       ex.printStackTrace();
>                   }
>               }
> +            System.err.println("assasin for" + p.toString() +" "+ p.getP().toString()+" .. done "+p.getCommandLine()+"  termination "+wasTerminated);
> +            System.err.flush();
>           }
>       }
>

ThreadedProcess does not have a toString() method. Do you really need to 
see the object ids, or can you get rid of p.toString()?

Everything else looks fine.

Cheers,
Omair