[fyi][icedtea-web]Refactoring of reproducers as agreed in April
Jiri Vanek
jvanek at redhat.com
Wed Jun 27 11:44:46 PDT 2012
Forgot attachements :( Sorry .
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.
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.
>> 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.
>
>> Best regards
>> J.
>> +++ /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 :)
>
>> 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
>>
>> # HG changeset patch
>> # User Jiri Vanek<jvanek at redhat.com>
>> # Date 1340358733 -7200
>> # Node ID 93962b0ba8fb54a5aa5216c926284a37363c6843
>> # Parent 8f67ddbf36f63b1f8bd35094e084aaea2be42576
>> Server access, surrounding logic and annotations moved from tests/netx/jnlp_testsengine/ to tests/test-extensions/
>
> Please try to keep the first line of changeset to about 50 characters.
> You can have multiple paragraphs here if you like, instead. See
> http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html for
> some ideas on good style (and reasons). Nice messages make it a lot
> easier to review changeset history.
The Second rebuke accepted too as already written.
> .
> 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.
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.
>
>> 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.
>
>> diff -r 8f67ddbf36f6 -r 93962b0ba8fb tests/test-extensions/net/sourceforge/jnlp/ServerAccess.java
>> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
>> +++ b/tests/test-extensions/net/sourceforge/jnlp/ServerAccess.java Fri Jun 22 11:52:13 2012 +0200
>> @@ -0,0 +1,1761 @@
>
>> +
>> +/**
>> + *
>> + * This class provides access to virtual server and stuff around.
>> + * It can find unoccupied port, start server, provides its singleton instantiation, lunch parallel instantiations,
>
> s/lunch/launch/
fixed O:(
>
>> + * read location of installed (tested javaws) see javaws.build.bin java property,
>> + * location of server www root on file system (see test.server.dir java property),
>> + * stubs for lunching javaws and for locating resources and read resources.
>> + *
>> + * It can also execute processes with timeout (@see PROCESS_TIMEOUT) (used during lunching javaws)
>> + * Some protected apis are exported because public classes in this package are put to be tested by makefile.
>> + *
>> + * There are included test cases which show some basic usages.
>> + *
>> + *
>> + */
>> +public class ServerAccess {
>
>
>> + @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.
>> diff -r 8f67ddbf36f6 -r 93962b0ba8fb tests/test-extensions/net/sourceforge/jnlp/annotations/TestInBrowsers.java
>> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
>> +++ b/tests/test-extensions/net/sourceforge/jnlp/annotations/TestInBrowsers.java Fri Jun 22 11:52:13 2012 +0200
>> @@ -0,0 +1,18 @@
>> +/*
>> + * To change this template, choose Tools | Templates
>> + * and open the template in the editor.
>> + */
>> +
>
> Please remove this comment.
Argh All those removed. I was doing this on my home machine where this tempalte is still enabled.
Sorry:(
>
>
>> 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.
> 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? 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!).
>
> 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;
> }
> }
>
>> extractedTestsAndInnerClasses
>>
>>
>> # HG changeset patch
>> # User Jiri Vanek<jvanek at redhat.com>
>> # Date 1340359272 -7200
>> # Node ID 6f0fbc88e4def1214d88b521c2fa3b3091328232
>> # Parent 93962b0ba8fb54a5aa5216c926284a37363c6843
>> All tests from test-extensions exctracted to test-extensions-tests. All inner classes in test-extensions extracted as outer classes.
>
>
>> diff -r 93962b0ba8fb -r 6f0fbc88e4de tests/test-extensions/net/sourceforge/jnlp/LoggingBottleneck.java
>> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
>> +++ b/tests/test-extensions/net/sourceforge/jnlp/LoggingBottleneck.java Fri Jun 22 12:01:12 2012 +0200
>> @@ -0,0 +1,186 @@
>> +/*
>> + * To change this template, choose Tools | Templates
>> + * and open the template in the editor.
>> + */
>
> Please replace this comment with the license header.
Argh All those removed. I was doing this on my home machine where this tempalte is still enabled.
Sorry:(
>
>> +/**
>> + *
>> + * @author jvanek
>> + */
>
> I am not sure what the policy with regards to having author tags in
> icedtea-web is, but I would recommend you note down your entire email
> address instead of the username if you insist on having an author tag.
I really do not want. Removed. Reason same as for "templates used" above. Sorry again for vasting
your space.
>
>
>> diff -r 93962b0ba8fb -r 6f0fbc88e4de tests/test-extensions/net/sourceforge/jnlp/ProcessAssasin.java
>> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
>> +++ b/tests/test-extensions/net/sourceforge/jnlp/ProcessAssasin.java Fri Jun 22 12:01:12 2012 +0200
>> @@ -0,0 +1,122 @@
>> +package net.sourceforge.jnlp;
>> +
>> +/**
>> + * class which timeout any ThreadedProcess. This killing of 'thread with process' replaced not working process.destroy().
>> + */
>> +class ProcessAssasin extends Thread {
>
>> + @Override
>> + public void run() {
>> + long startTime = System.nanoTime() / ServerAccess.NANO_TIME_DELIMITER;
>> + while (canRun) {
>> + try {
>> + long time = System.nanoTime() / ServerAccess.NANO_TIME_DELIMITER;
>> + //ServerAccess.logOutputReprint(time - startTime);
>> + //ServerAccess.logOutputReprint((time - startTime)> timeout);
>> + if ((time - startTime)> timeout) {
>
> Just a note that the nano time subtraction deals fairly well with
> overflow, but that may break down if we are dividing the values we are
> using in the computation with a factor before computing the difference.
>
>> diff -r 93962b0ba8fb -r 6f0fbc88e4de tests/test-extensions/net/sourceforge/jnlp/ProcessResult.java
>> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
>> +++ b/tests/test-extensions/net/sourceforge/jnlp/ProcessResult.java Fri Jun 22 12:01:12 2012 +0200
>> @@ -0,0 +1,32 @@
>> +package net.sourceforge.jnlp;
>> +
>> +/**
>> + * artefacts what are left by finished process
>> + */
>> +public class ProcessResult {
>> +
>> + public final String stdout;
>> + public final String notFilteredStdout;
>> + public final String stderr;
>> + public final Process process;
>> + public final Integer returnValue;
>> + public final boolean wasTerminated;
>> + /*
>> + * possible exception which caused Process not to be launched
>> + */
>> + public final Throwable deadlyException;
>> +
>> + public ProcessResult(String stdout, String stderr, Process process, boolean wasTerminated, Integer r, Throwable deadlyException) {
>> + this.notFilteredStdout = stdout;
>> + 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 :(
>
>> + this.stderr = stderr;
>> + this.process = process;
>> + this.wasTerminated = wasTerminated;
>> + this.returnValue = r;
>> + this.deadlyException = deadlyException;
>> + }
>> +}
>
>
>> diff -r 93962b0ba8fb -r 6f0fbc88e4de tests/test-extensions/net/sourceforge/jnlp/ServerLauncher.java
>> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
>> +++ b/tests/test-extensions/net/sourceforge/jnlp/ServerLauncher.java Fri Jun 22 12:01:12 2012 +0200
>
>> +/**
>> + * wrapper around tiny http server to separate lunch configurations and servers.
>
> s/lunch/launch/
fixed
>
>> + * 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!
>
>> diff -r 93962b0ba8fb -r 6f0fbc88e4de tests/test-extensions/net/sourceforge/jnlp/TinyHttpdImpl.java
>> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
>> +++ b/tests/test-extensions/net/sourceforge/jnlp/TinyHttpdImpl.java Fri Jun 22 12:01:12 2012 +0200
>> @@ -0,0 +1,136 @@
>
>> +/**
>> + * based on http://www.mcwalter.org/technology/java/httpd/tiny/index.html
>> + * Very small implementation of http return headers for our served resources
>
> If the code in this file is based on the implementation there, then we
> will have to adjust the license appropriately (and perhaps even
> reconsider using that code, depending on the license).
>
It is gpl2. I have added it it javadoc of this class. I thing it means no issues here.
> I am not going to read the rest of this class.
Good idea O:)
>
>> + * 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 :(
>
>> +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
J.
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: jnlp_testsengine2test-extensions2.giff
Url: http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20120627/18bff8a4/jnlp_testsengine2test-extensions2.giff
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: jnlp_tests2reproducers2.giff
Url: http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20120627/18bff8a4/jnlp_tests2reproducers2.giff
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: extractedTestsAndInnerClasses2.giff
Url: http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20120627/18bff8a4/extractedTestsAndInnerClasses2.giff
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: addedMissingHeaders.giff
Url: http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20120627/18bff8a4/addedMissingHeaders.giff
-------------- next part --------------
A non-text attachment was scrubbed...
Name: customMakefiles2.diff
Type: text/x-patch
Size: 11431 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20120627/18bff8a4/customMakefiles2.diff
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: Changelogs
Url: http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20120627/18bff8a4/Changelogs.ksh
More information about the distro-pkg-dev
mailing list