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

[Jiri Vanek]

On 05/25/2012 09:08 PM, Danesh Dadachanji wrote:
> 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?
>

All issues should be fixed. Except renaming - i Renamed it little bit differently: logErrorReprint 
and logOutputReprint. Thats because I would like chars before auto-completion finish it for me as 
short as possible:)


Sorry  of changelog inside patch, but I accidentally make some work in same branch before sending to 
you.


>
>> 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 */

All docs in ServerAcces are multi-lined. I would like to keep them so.

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

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