[fyi][icedtea-web]Refactoring of reproducers as agreed in April
Omair Majid
omajid at redhat.com
Tue Jun 26 18:27:27 PDT 2012
On 06/22/2012 07:16 AM, Jiri Vanek wrote:
> 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.
> 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.
> Best regards
> J.
>
>
> 2012-06-21 Jiri Vanek <jvanek at redhat.com>
>
> Makefile adapted to recent (three changesets) refactoring
> * Makefile.am: (JNLP_TESTS_ENGINE_SRCDIR) now points correctly to
> test-extensions. (JNLP_TESTS_ENGINE_TESTS_SRCDIR) new variable for
> test-extensions-tests. (JNLP_TESTS_SRCDIR) now points to reproducers.
> (JNLP_TESTS_ENGINE_TESTS_DIR) new variable for built
> JNLP_TESTS_ENGINE_TESTS_SRCDIR
> (netx-dist-tests-tests-source-files.txt) new target for list of
> content of JNLP_TESTS_ENGINE_TESTS_SRCDIR.
> (stamps/netx-dist-tests-tests-compile.stamp) new target for compiling
> netx-dist-tests-tests-source-files.txt
> (netx-dist-tests-source-files.tx) now depends on
> stamps/netx-dist-tests-tests-compile.stamp
> ($(REPRODUCERS_CLASS_NAMES)) target is now working in
> JNLP_TESTS_ENGINE_TESTS_DIR
> instead of JNLP_TESTS_ENGINE_DIR
> (stamps/run-netx-dist-tests.stamp): added
> JNLP_TESTS_ENGINE_TESTS_DIR to classpath
> (stamps/run-unit-test-code-coverage.stamp),
> (stamps/run-reproducers-test-code-coverage.stamp)
> added JNLP_TESTS_ENGINE_TESTS_DIR to classpath and
> JNLP_TESTS_ENGINE_TESTS_SRCDIR to sources path
>
> 2012-06-21 Jiri Vanek <jvanek at redhat.com>
>
> All tests from test-extensions extracted to test-extensions-tests.
> All inner classes in test-extensions extracted as outer classes
> * tests/test-extensions/net/sourceforge/jnlp/ResourcesTest.java:
> moved to test-extensions-tests
> * tests/test-extensions-tests/net/sourceforge/jnlp/ResourcesTest.java:
> new file, copied from test-extensions
> *
> tests/test-extensions-tests/net/sourceforge/jnlp/ServerAccessTest.java:
> all tests from original ServerAccess.java
> * tests/test-extensions/net/sourceforge/jnlp/ContentReader.java:
> * tests/test-extensions/net/sourceforge/jnlp/LogItem.java:
> * tests/test-extensions/net/sourceforge/jnlp/LoggingBottleneck.java:
> * tests/test-extensions/net/sourceforge/jnlp/ProcessAssasin.java:
> * tests/test-extensions/net/sourceforge/jnlp/ProcessResult.java:
> * tests/test-extensions/net/sourceforge/jnlp/ServerLauncher.java:
> * tests/test-extensions/net/sourceforge/jnlp/TestsLogs.java:
> * tests/test-extensions/net/sourceforge/jnlp/ThreadedProcess.java:
> * tests/test-extensions/net/sourceforge/jnlp/TinyHttpdImpl.java:
> new files, extracted classes from ServerAccess
> * tests/test-extensions/net/sourceforge/jnlp/ServerAccess.java:
> extracted tests and inner classes
>
>
> 2012-06-21 Jiri Vanek <jvanek at redhat.com>
>
> hg move tests/netx/jnlp_testsengine/ tests/test-extensions/
>
> 2012-06-21 Jiri Vanek <jvanek at redhat.com>
>
> hg move tests/jnlp_tests/ tests/netx/reproducers
>
> jnlp_tests2reproducers
>
>
> # HG changeset patch
> # User Jiri Vanek <jvanek at redhat.com>
> # Date 1340358597 -7200
> # Node ID 8f67ddbf36f63b1f8bd35094e084aaea2be42576
> # Parent 4a89a9c1a66276e0ad42153279332573b948327b
> Moved all reproducers from tests/jnlp_tests/ to tests/netx/reproducers/
>
> diff -r 4a89a9c1a662 -r 8f67ddbf36f6 tests/jnlp_tests/README
> --- a/tests/jnlp_tests/README Tue Jun 19 10:26:44 2012 -0400
> +++ /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.
> 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"?>
> +<jnlp spec="1.0"
> + codebase="./"
> + href="ReadEnvironment.jnlp">
> + <information>
> + <title>ReadEnvironment using System.getenv()</title>
> + <vendor>IcedTea</vendor>
> + </information>
> + <resources>
> + <jar href="ReadEnvironment.jar" main="true" />
> + </resources>
> + <application-desc main-class="ReadEnvironment"/>
> +</jnlp>
> diff -r 4a89a9c1a662 -r 8f67ddbf36f6 tests/netx/reproducers/simple/ReadProperties/resources/ReadProperties1.jnlp
> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> +++ b/tests/netx/reproducers/simple/ReadProperties/resources/ReadProperties1.jnlp Fri Jun 22 11:49:57 2012 +0200
> @@ -0,0 +1,17 @@
+3
> +<?xml version="1.0" encoding="utf-8"?>
> +<jnlp spec="1.0"
> + codebase="./"
> + href="ReadProperties1.jnlp">
> + <information>
> + <title>read properties using System.getenv()</title>
> + <vendor>IcedTea</vendor>
> + </information>
> + <resources>
> + <jar href="ReadProperties.jar" main="true"/>
> + </resources>
> + <application-desc main-class="ReadProperties">
> + <argument>user.name</argument>
> + </application-desc>
> +</jnlp>
> diff -r 4a89a9c1a662 -r 8f67ddbf36f6 tests/netx/reproducers/simple/ReadProperties/resources/ReadProperties2.jnlp
> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> +++ b/tests/netx/reproducers/simple/ReadProperties/resources/ReadProperties2.jnlp Fri Jun 22 11:49:57 2012 +0200
> @@ -0,0 +1,15 @@
+4
> +<?xml version="1.0" encoding="utf-8"?>
> +<jnlp spec="1.0"
> + codebase="./"
> + href="ReadProperties2.jnlp">
> + <information>
> + <title>read properties using System.getenv()</title>
> + <vendor>IcedTea</vendor>
> + </information>
> + <resources>
> + <jar href="ReadProperties.jar" main="true"/>
> + </resources>
> + <application-desc main-class="ReadProperties">
> + <argument>user.home</argument>
> + </application-desc>
> +</jnlp>
> diff -r 4a89a9c1a662 -r 8f67ddbf36f6 tests/netx/reproducers/simple/RedirectStreams/resources/RedirectStreams.jnlp
> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> +++ b/tests/netx/reproducers/simple/RedirectStreams/resources/RedirectStreams.jnlp Fri Jun 22 11:49:57 2012 +0200
> @@ -0,0 +1,13 @@
+5
> +<?xml version="1.0" encoding="utf-8"?>
> +<jnlp spec="1.0"
> + codebase="./"
> + href="RedirectStreams.jnlp">
> + <information>
> + <title>redirect stdin/stdout streams</title>
> + <vendor>IcedTea</vendor>
> + </information>
> + <resources>
> + <jar href="RedirectStreams.jar" main="true"/>
> + </resources>
> + <application-desc main-class="RedirectStreams"/>
> +</jnlp>
> diff -r 4a89a9c1a662 -r 8f67ddbf36f6 tests/netx/reproducers/simple/ReplaceSecurityManager/resources/ReplaceSecurityManager.jnlp
> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> +++ b/tests/netx/reproducers/simple/ReplaceSecurityManager/resources/ReplaceSecurityManager.jnlp Fri Jun 22 11:49:57 2012 +0200
> @@ -0,0 +1,13 @@
+6
> +<?xml version="1.0" encoding="utf-8"?>
> +<jnlp spec="1.0"
> + codebase="./"
> + href="ReplaceSecurityManager.jnlp">
> + <information>
> + <title>Test replacing security manager</title>
> + <vendor>IcedTea</vendor>
> + </information>
> + <resources>
> + <jar href="ReplaceSecurityManager.jar" main="true"/>
> + </resources>
> + <application-desc main-class="ReplaceSecurityManager"/>
> +</jnlp>
> diff -r 4a89a9c1a662 -r 8f67ddbf36f6 tests/netx/reproducers/simple/SetContextClassLoader/resources/SetContextClassLoader.jnlp
> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> +++ b/tests/netx/reproducers/simple/SetContextClassLoader/resources/SetContextClassLoader.jnlp Fri Jun 22 11:49:57 2012 +0200
> @@ -0,0 +1,13 @@
+7
> +<?xml version="1.0" encoding="utf-8"?>
> +<jnlp spec="1.0"
> + codebase="./"
> + href="SetContextClassLoader.jnlp">
> + <information>
> + <title>set context classloader</title>
> + <vendor>IcedTea</vendor>
> + </information>
> + <resources>
> + <jar href="SetContextClassLoader.jar" main="true" download="eager"/>
> + </resources>
> + <application-desc main-class="SetContextClassLoader"/>
> +</jnlp>
>
> jnlp_testsengine2test-extensions
>
>
> # 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.
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.
> 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.
> 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/
> + * 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.
> 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.
> 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
Also, this class makes me think it would be hard to represent sets of
browsers, like, say, "firefox and chromium". Perhaps this should be
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.
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.
> +/**
> + *
> + * @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.
> 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.
> + 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/
> + * 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?
> +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?
> 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).
I am not going to read the rest of this class.
> + * 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?
> +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.
Cheers,
Omair
More information about the distro-pkg-dev
mailing list