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

[Jiri Vanek]

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 ) and renamed to ServerLauncher
     Enchanted wrapping of executeProcess

On 06/29/2011 07:08 PM, Omair Majid wrote:
> 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 :/
Thanx for catch. Fixed
>
>> + 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.
done. thanx.
>
>> @@ -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()?
variable with uselessly "cached" valuee removed, String generating code moved to method as suggested. ok?

>
>> @@ -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 :)
Done. I have increased inner version flag by one due to change of "api".
>
>> @@ -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()?

I would like to keep this in code. It helps me to find correct process when debuging.
>
> Everything else looks fine.
>
> Cheers,
> Omair
Thank you for your time J.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: fixedMinorChanges.diff
Type: text/x-patch
Size: 12053 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20110711/a8bca78d/fixedMinorChanges.diff