[fyi][icedtea-web]Refactoring of reproducers as agreed in April
Omair Majid
omajid at redhat.com
Wed Jun 27 11:43:48 PDT 2012
On 06/27/2012 01:49 PM, Jiri Vanek wrote:
> On 06/27/2012 03:27 AM, Omair Majid wrote:
>> On 06/22/2012 07:16 AM, Jiri Vanek wrote:
>
> Wooooooou You have teaken really deeeep look into this. Even to veeerey
> unrelated and veeery long pushed (and maybe rotten )code.
Well, I commented on everything that caught my eye. You are right in
that this need not be addressed at once. I just wanted to note down the
issues so they are known and can be resolved.
> I will split this answer to this reply, to expallanation of bounch of
> things and to separate possible issues for future tasks.
>
>>> Hi!
>>>
>>> This is effort to make reproducers structure little bit more sense-full
>>> and to *get rid of all the initialization errors* in tests runs.
>>> The refactoring is done as we have agreed in "Re: [rfc] [icedtea-web]
>>> blacklist for reproducers" from 13.4.2012 11:54PM (cet?)
>>>
>>> so sumamry :)
>>> 1. hg move tests/jnlp_tests/ tests/netx/reproducers
>>
>> I like this organization. But that still leaves applet tests under
>> tests/netx/reproducers. Perhaps they should be moved to
>> tests/applet/reproducers ? If they are exercising the browser plugin
>> too, then tests/plugin/reproducers seems good to me as well.
>
>
> Well this was not agrred in april :)
> I'm against splliting jnl and applet tests. Really a lot of resources
> are shared - One jar is often loaded through jnlp and by html because
> issue is shared to. Also plugin stays on netx so both are netx
> reprodcures. And at the end, both browser and javaws is just launched
> processes:)
> Imho really no need tosplit from all points of view.
In that case, how about just
hg move tests/jnlp_tests/ tests/reproducers ?
>>> 2. hg move tests/netx/jnlp_testsengine/ tests/test-extensions/
>>> 3. extracted inner classes and tests from test-extensions
>>> 4. adapted makefile
>>>
>>> Those are four changesets. I have included them together as first two
>>> are little bit "nothing to do" and each separated have no sense.
>>>
>>> At the end I have discovered I have forgot to add header files to new
>>> (extracted inner classes) files. Please, if you can survive them, I will
>>> add them in next changeset.
>>
>> I guess by header you mean the licence header.
>
> I have added all headers to files I'm adding in this changests.
I dont see any patches in this email (not sure if that's intentional).
Btw, if you find yourself managing and holding on to a number of patches
until they are reviewed/pushed, mq is a fantastic tool. If you aren't
familiar with it, I would recommend trying it out. It allows you to
locally keep a series of patches and (once they are reviewed on the
mailing lists) to convert them into changesets and push them. The nice
thing about it is that it allows later patches that rely on files being
renamed to survive additional renaming.
>>> +++ /dev/null Thu Jan 01 00:00:00 1970 +0000
>>
>> You might want to try and use the git format for these diffs. It makes
>> file renames easier to review. See
>> http://mercurial.selenic.com/wiki/GitExtendedDiffFormat for information
>> on how to set it up.
>
> done - both rebukes (this and the second) accepted. And I hope fixed.
> Thanx for --gir switch :)
They weren't rebukes! More like gentle reminders :) I want just trying
to help myself down the road when I have to dig through mercurial
history to trace something.
As for the --git switch, it's so useful that I can't imagine how I lived
without it.
>>
>>> diff -r 4a89a9c1a662 -r 8f67ddbf36f6
>>> tests/netx/reproducers/simple/CreateClassLoader/resources/CreateClassLoader.jnlp
>>>
>>> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
>>> +++
>>> b/tests/netx/reproducers/simple/CreateClassLoader/resources/CreateClassLoader.jnlp
>>> Fri Jun 22 11:49:57 2012 +0200
>>> @@ -0,0 +1,13 @@
>>
>> Not related to your changes, but should we have a license header here?
>> Unless I am mistaken, they are present in other jnlp files but not here.
>> If you dont mind, I will post a patch for this separately. I am noting
>> it here so I can find it easily next time.
>>
>>> diff -r 4a89a9c1a662 -r 8f67ddbf36f6
>>> tests/netx/reproducers/simple/ReadEnvironment/resources/ReadEnvironment.jnlp
>>>
>>> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
>>> +++
>>> b/tests/netx/reproducers/simple/ReadEnvironment/resources/ReadEnvironment.jnlp
>>> Fri Jun 22 11:49:57 2012 +0200
>>> @@ -0,0 +1,13 @@
>>
>> +2
>>
>>> +<?xml version="1.0" encoding="utf-8"?>
>
> all missing headers added as fifth change set. Thanx for enumerating
Thanks for doing this! I was going to do it myself, but now I guess I
wont have to :)
>> Also, something that just occurred to me. Since these extensions are not
>> part of netx per se, perhaps we should put them in a separate package.
>> The browsertesting subpackage is a good start, IMHO.
>
> Hmm.. This will lead to another refactoring in all reproducers.. And so
> it is something I would like to lay in my head for a while and so
> postpone this change.
Right. I didn't mean to imply lets fix all this now. More as a "this
would be nice to have".
> Currently I'm against it a t all. I was thinking about
> net.sourceforge.jnlp as to namespace when creating this package. And I'm
> still thinking arround this in same manner - and because those test
> extensions were made for testing of net.sourceforge.. I would like to
> keep the "namespace" same.
Sorry, but I dont follow. The tests extensions are not tied to
net.sourceforge.jnlp* packages, are they? A package org.foo.bar would be
able to make use of them too, no? I would like to see this under
org.classpath.icedteaweb.test or something, TBH.
>>
>>> diff -r 8f67ddbf36f6 -r 93962b0ba8fb
>>> tests/test-extensions/net/sourceforge/jnlp/ContentReaderListener.java
>>> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
>>> +++
>>> b/tests/test-extensions/net/sourceforge/jnlp/ContentReaderListener.java
>>> Fri Jun 22 11:52:13 2012 +0200
>>> @@ -0,0 +1,8 @@
>>> +package net.sourceforge.jnlp;
>>> +
>>> +public interface ContentReaderListener {
>>> +
>>> + public void charReaded(char ch);
>>> + public void lineReaded(String s);
>>
>> The names of the methods are not very clear. I don't know what they are
>> meant for, but 'readChar' and 'readLine' sound more correct. If you
>> could you explain the purpose of these methods, we may be able to come
>> up with better names.
>
> It is listener, not "command". It is hit when char is read. for
> "readSomething" I will imaginne order to read something, not reaction to
> reading. However. it is very unrelated and if you can survive it I will
> be very happy. If you really want to change this name before it is more
> wide spread (now just two or three reproducers are using it) lets make
> your command - but please as another changeset.
Well, if you really want, charRead/lineRead should be okay for now. And
yes, separate changeset is fine.
>>
>>> + @Test
>>> + public void testsProcessResultFiltering() throws Exception {
>>
>> Having test methods in the same file/class as the actual code? I am not
>> very happy with this. It's okay to leave it as it is for now, but we
>> should move it out.
>>
>
> nn:) You have overlooked yourself:) To get tests out of implementation
> was one of the main reasons of this refactoring. So after applicattion
> of extractedTestsAndInnerClasses, all tests are seaprated as you wish in
> this request.
>
Ah, cool!
>>> diff -r 8f67ddbf36f6 -r 93962b0ba8fb
>>> tests/test-extensions/net/sourceforge/jnlp/browsertesting/Browsers.java
>>> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
>>> +++
>>> b/tests/test-extensions/net/sourceforge/jnlp/browsertesting/Browsers.java
>>> Fri Jun 22 11:52:13 2012 +0200
>>> @@ -0,0 +1,46 @@
>>> +package net.sourceforge.jnlp.browsertesting;
>>> +
>>> +/**
>>> + * When all represent all configured browser, one represens one random
>>> + * (the first found) configured browser. Each other represents
>>> inidivdual browsers
>>> + *
>>> + */
>>> +public enum Browsers {
>>> +
>>> + none, all, one, opera, googleChrome, chromiumBrowser, firefox,
>>> midori,epiphany;
>>
>> I believe the convention is to have enums in UPPER_CASE:
>> http://docs.oracle.com/javase/tutorial/java/javaOO/enum.html
>
> ARG. Big mistake and reviwer's overlook :(( Can we left it for (more
> far) future?
>
>>
>> Also, this class makes me think it would be hard to represent sets of
>> browsers, like, say, "firefox and chromium". Perhaps this should be
>
> No. It is not problem. Annotation (which is the best place to use this
> enum) is accepting array
> so @testInBrowser(testIn={Browsers.firefox,Browsers.chromium})
> Will do exactly what you want to do and was one of the reasons of
> creating this annotation.
How is testIn={Browsers.firefox,Browsers.all} handled? Or is it handled
at all?
>> split into two: a Browser class should just list browsers and a
>> BrowserSet class would provide a set of browsers (include all, or none).
>>
>>> +
>>> + public String toExec() {
>>> + switch (this) {
>>> + case opera:
>>> + return "opera";
>>> + case googleChrome:
>>> + return "google-chrome";
>>> + case chromiumBrowser:
>>> + return "chromium-browser";
>>> + case firefox:
>>> + return "firefox";
>>> + case midori:
>>> + return "midori";
>>> + case epiphany:
>>> + return "epiphany";
>>> + default:
>>> + return null;
>>
>> I would suggest to make this part of the enum itself and rely on object
>> orientation.
>
> Can you be little bit more specific?
The enum that I elaborated below is what I meant. Each instance
(FIREFOX, CHROMIUM) would know about the executable itself, so you wont
have a nasty switch statement (or multiple switch statements) to handle
all instances.
Anyway - although looks like
> nice-to-have fix, and I would like to follow your advice here, please
> let it for another changeset (in closer feature!).
Of course!
>> public enum Browser {
>>
>> FIREFOX("firefox"),
>> CHROMIUM("chromium-browser"),
>> :
>> OPERA("opera");
>>
>> private String exeutable;
>>
>> public Browser(String executable) {
>> this.executable = executable;
>> }
>>
>> public String getExecutableName() {
>> return executable;
>> }
>> }
^^^ My "elaboration" :)
>>> + if (stdout == null) {
>>> + this.stdout = null;
>>> + } else {
>>> + this.stdout = stdout.replaceAll("EMMA:.*\n?", "");
>>> + }
>>
>> I would recommend leaving this out. Let's create a subclass that does
>> this operation so we are not tying this class to emma at all.
>
> I'm for this refactoring in closer feature too. Anyway, I can not let
> EMMA hint inside. It makes all tests depending on "std should contains
> .." to fail :(
I meant we should have a sublcass that wraps this class and does the
removal. The test can use that class instead of this.
>>> + * to allow terminations and stuff around.
>>> + */
>>> +public class ServerLauncher implements Runnable {
>>
>>> diff -r 93962b0ba8fb -r 6f0fbc88e4de
>>> tests/test-extensions/net/sourceforge/jnlp/ThreadedProcess.java
>>> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
>>> +++
>>> b/tests/test-extensions/net/sourceforge/jnlp/ThreadedProcess.java
>>> Fri Jun 22 12:01:12 2012 +0200
>>> @@ -0,0 +1,103 @@
>>> +package net.sourceforge.jnlp;
>>> +
>>> +import java.io.File;
>>> +import java.util.List;
>>> +
>>> +/**
>>> + *
>>> + * wrapper around Runtime.getRuntime().exec(...) which ensures that
>>> process is run inside its own, by us controlled, thread.
>>> + * Process builder caused some unexpected and weird behavior :/
>>> + */
>>
>> Hm... this is really strange. Each process runs in it's own process
>> space. The thread here is for monitoring/wait()ing for it, I suppose.
>> What problems were you having with ProcessBuilder?
>
> This was dsicussed with you when we were creating the initial concept of
> reproducers:) There were many reasons to use sync. System.exec instead
> of Processbuilder. Some of them were caused by my wrong implementation
> and maybe current implementation is causing some issues you are refering
> lower.
>
> I would definitely investigate this (together witl lower hints), but
> please as differerent changeset.
>>
>>> +class ThreadedProcess extends Thread {
>>> +
>>> + Process p = null;
>>> + List<String> args;
>>> + Integer exitCode;
>>> + Boolean running;
>>> + File dir;
>>> + Throwable deadlyException = null;
>>> + /*
>>> + * before removing this "useless" variable
>>> + * check DeadLockTestTest.testDeadLockTestTerminated2
>>> + */
>>
>> :)
>>
>>> + private boolean destoyed = false;
>>> +
>>
>>> + @Override
>>> + public void run() {
>>> + try {
>>> + running = true;
>>> + Runtime r = Runtime.getRuntime();
>>> + if (dir == null) {
>>> + p = r.exec(args.toArray(new String[0]));
>>> + } else {
>>> + p = r.exec(args.toArray(new String[0]), new
>>> String[0], dir);
>>> + }
>>> + try {
>>> + exitCode = p.waitFor();
>>> + Thread.sleep(500); //this is giving to fast done
>>> proecesses's e/o readers time to read all. I would like to know
>>> better solution :-/
>>
>> More surprising. Do you have a test case or something that shows this
>> bug?
>
> yah I have :)
>
> I was using executeProcess("ps -A") in deadlock xfork reproducer.
> Without this sleep, tehere was always some random amount of lines missing.
>
> If you will find the pitfal, I will be very glad. But IMHO it is (with
> above and lower) subject of longer (and unrelated O:)) investigations.
> But thanx for diging it out!
/me notes this down for digging into later
>>> + * When resource starts with XslowX prefix, then resouce (without
>>> XslowX)
>>> + * is returned, but its delivery is delayed
>>> + */
>>
>> I thought this XslowX hack was removed? Is this comment outdated?
>
> It was not removed, because we have not found better solution yet :(
My first thought is "annotation".
>>> +class TinyHttpdImpl extends Thread {
>>
>>> # HG changeset patch
>>> # User Jiri Vanek<jvanek at redhat.com>
>>> # Date 1340359905 -7200
>>> # Node ID 0d1566c6d7457ab0ec9eb4cdacd245c82931c56a
>>> # Parent 6f0fbc88e4def1214d88b521c2fa3b3091328232
>>> Adapted Makefile.am
>>
>> Nothing here looks wrong at first glance, but I will go through it in
>> detail once all the other details are ironed out.
>
>
> Wooou. it was really THE review. Thanx for it. I hope you will survive
> my "unrelated" talks - sorry for them. But those issues you have pointed
> out ( I will summarize them later) really need deeper investrigations
> then "refactoring"
>
> Best regards, and thanx for your time
Glad I could help! Looking forward to the rest of the patches.
Cheers,
Omair
More information about the distro-pkg-dev
mailing list