[rfc] [icedtea-web] get rid of repated sout/serr in reproducers testcases

[Danesh Dadachanji]

On 25/05/12 08:55 AM, Jiri Vanek wrote:
> Hi!
>
> I would like to get rid of repeating std out/err reprint in reproducers by moving this into ServerAccess class. This patch is adding
> simple loging interface for stdout/err and (mainly) is hiding reprinting (logging since now:-) of processes out/err and "connectng: "
> message into executeProcess.
>
> My goal in longer term is to have html file (generated from logged xml file probably) which I will be able to link (anchor inlcuded)
> from current test results html files.And so I will be able to quickly find whats wrong. Now it is becoming little bit painfull. >(Both
> stdou/err of processes and searching in looong output file)
>
> Saad, Danesh - you have number of reproducers in development. Please count with this change.
>
>

Thanks for letting us know. I have some comments below. Could you also add a ChangeLog entry?


> diff -r 2c84a314c411 tests/jnlp_tests/simple/deadlocktest/testcases/DeadLockTestTest.java
> --- a/tests/jnlp_tests/simple/deadlocktest/testcases/DeadLockTestTest.java	Thu May 24 15:37:30 2012 -0400
> +++ b/tests/jnlp_tests/simple/deadlocktest/testcases/DeadLockTestTest.java	Fri May 25 14:36:05 2012 +0200

> @@ -168,12 +156,15 @@
>               if (old.contains(string)) {
>                   continue;
>               }
> -            System.out.println("Killing " + string);
> -            ServerAccess.ProcessResult pr = ServerAccess.executeProcess(Arrays.asList(new String[]{"kill", "-9", string}));
> +           ServerAccess.logO("Killing " + string);

Add a space for indenting please! (Surprised I caught this even, guess it just happened to stand :/  )

[snip]

> @@ -181,23 +172,26 @@
>       private static List<String>  countJavaInstances() throws Exception {
>           ensureLinux();
>           List<String>  result = new ArrayList<String>();
> -        ServerAccess.ProcessResult pr = ServerAccess.executeProcess(Arrays.asList(new String[]{"ps", "-eo", "pid,ppid,stat,fname"}));
> -        Matcher m = Pattern.compile("\\s*\\d+\\s+\\d+ .+ java\\s*").matcher(pr.stdout);
> -        //System.out.println(pr.stdout);
> -        //System.err.println(pr.stderr);
> +        ServerAccess.PROCES_LOG = false;
> +        try {
> +            ServerAccess.ProcessResult pr = ServerAccess.executeProcess(Arrays.asList(new String[]{"ps", "-eo", "pid,ppid,stat,fname"}));
> +       Matcher m = Pattern.compile("\\s*\\d+\\s+\\d+ .+ java\\s*").matcher(pr.stdout);
>           int i = 0;
>           while (m.find()) {
>               i++;
>               String ss = m.group();
> -            //System.out.println(i+": "+ss);
> +            //ServerAccess.logO(i+": "+ss);
>               result.add(ss.trim().split("\\s+")[0]);
>           }
> +         } finally {
> +            ServerAccess.PROCES_LOG = true;
> +        }

Indentation of this chunk looks a bit off, could you double check it?

[snip]

> diff -r 2c84a314c411 tests/netx/jnlp_testsengine/net/sourceforge/jnlp/ServerAccess.java
> --- a/tests/netx/jnlp_testsengine/net/sourceforge/jnlp/ServerAccess.java	Thu May 24 15:37:30 2012 -0400
> +++ b/tests/netx/jnlp_testsengine/net/sourceforge/jnlp/ServerAccess.java	Fri May 25 14:36:05 2012 +0200
> @@ -116,6 +116,10 @@
>        * all terminated processes are stored here. As wee need to 'wait' to termination to be finished.
>        */
>       private static Set<Thread>  terminated = new HashSet<Thread>();
> +    /**
> +     * this flag is indicating whether ooutput of executeProcess should be logged. By default true.
> +     */

s/ooutput/output/
I think also for style, we use single lined comments for class vars - /** <comment-on-one-line */

> +    public static boolean PROCES_LOG=true;
>

Could you refactor this so that it's s/PROCES/PROCESS/

Also, there are lots of missing spaces in this patch, e.g. around the '=' above. Could you please space correctly, thanks!

>       /**
>        * main method of this class prints out random free port
> @@ -720,9 +724,72 @@
>           return executeProcess(args, dir, null, null);
>       }
>
> -     public static ProcessResult executeProcess(final List<String>  args,File dir,ContentReaderListener stdoutl,ContentReaderListener stderrl) throws Exception {
>
> -        ThreadedProcess t = new ThreadedProcess(args,dir);
> +    private static String createConnectionMessage(ThreadedProcess t) {
> +        return "Connecting " + t.getCommandLine();
> +    }
> +
> +    public static void logE(String s) {
> +        log(s, false, true);
> +    }
> +
> +    public static void logO(String s) {
> +        log(s, true, false);
> +    }
> +

Could you name these logStdErr and logStdOut instead? Either that or add javadoc please. Seeing these outside of this class may throw 
some newcomers off.

> +    private static void log(String message, boolean out, boolean err) {
> +        String idded;
> +        StackTraceElement ste = getTestMethod();
> +        String fullId=ste.getClassName()+"."+ste.getMethodName();
> +        if (message.contains("\n")) {
> +            idded = fullId + ": \n" + message+"\n"+fullId+" ---";
> +        } else {
> +            idded = fullId + ": " + message;
> +
> +        }
> +        if (out) {
> +            System.out.println(idded);
> +        }
> +        if (err) {
> +            System.err.println(idded);
> +        }
> +    }
> +

Again, could you name the bools printToOut and printToErr?

Thanks for doing this!

Cheers,
Danesh