Re: RFR [6943190] TEST_BUG: java/lang/Runtime/exec/ExecWithInput.java hardcodes path to cat
Thank you Alan! This looks okay although I would prefer for a number of these tests to fail if the command is not found (otherwise it is will just hide issues). Now I've got two suggestions that somehow contradict each other. Martin suggested to check for particular OS only as a last resort, but without knowing that we run under Unix, we cannot treat a command absence as an error. I think we can assume that none of the tests which need UnixCommands are for Windows, so I added explicit checking for that. Having made sure the OS is a Unix (i.e. not Windows), the absence of a required command now causes an exception to be thrown. Would you please take a look at the updated webrev? http://cr.openjdk.java.net/~igerasim/6943190/6/webrev/ Sincerely yours, Ivan
Re: RFR [6943190] TEST_BUG: java/lang/Runtime/exec/ExecWithInput.java hardcodes path to cat
On 20/03/2014 07:25, Ivan Gerasimov wrote: Now I've got two suggestions that somehow contradict each other. Martin suggested to check for particular OS only as a last resort, but without knowing that we run under Unix, we cannot treat a command absence as an error. It's always hard to get complete agreement on things like this but for tests that are *nix specific then I would say that it's better to check it at the start and have it just pass on Windows without needlessly checking for programs that don't exist or can't be used by the test. I think we can assume that none of the tests which need UnixCommands are for Windows, so I added explicit checking for that. Having made sure the OS is a Unix (i.e. not Windows), the absence of a required command now causes an exception to be thrown. Would you please take a look at the updated webrev? http://cr.openjdk.java.net/~igerasim/6943190/6/webrev/ This looks okay to me. An alternative would be for findCommand to throw an exception when the command is not found and that would allow most of the ensureCommandAvailables(...) usages to be removed and saves searching for them twice. -Alan
Re: RFR [6943190] TEST_BUG: java/lang/Runtime/exec/ExecWithInput.java hardcodes path to cat
Thank you Alan! I think we can assume that none of the tests which need UnixCommands are for Windows, so I added explicit checking for that. Having made sure the OS is a Unix (i.e. not Windows), the absence of a required command now causes an exception to be thrown. Would you please take a look at the updated webrev? http://cr.openjdk.java.net/~igerasim/6943190/6/webrev/ This looks okay to me. An alternative would be for findCommand to throw an exception when the command is not found and that would allow most of the ensureCommandAvailables(...) usages to be removed and saves searching for them twice. I think it is informative to have the prerequisites checking at the beginning of the test. And the results of the search are cached in UnixCommands.nameToCommand map, so they're searched for only once. Sincerely yours, Ivan
Re: RFR [6943190] TEST_BUG: java/lang/Runtime/exec/ExecWithInput.java hardcodes path to cat
Thanks Alan! These two tests used the commands run from non very common location (/usr/bin/ instead of /bin/), so I suspect they have been rarely run. As it follows from the summaries, one of them ensures the VM doesn't crash; the other checks, whether the input/output streams are left open. Both tests in the case of a failure could interfere with other tests unless they run in the othervm mode. That's why I thought it's better to add the flag. If a test is badly behaved and is leaving streams open then I would agree with othervm. However these are old issues (right?) and should not be happening now. Also if a test tickles a bug that causes the VM to crash then jtreg will spin up a new VM for the next test. So if it's possible to avoid othervm then we should (and from what I can tell then these tests have been well behaved when running without othervm before). Yes, got it. I will remove othervm mode. Once I was suggested to add the othervm mode to the test which was to detect a memory leak (in the case of a poor behavior). So I was under wrong impression that we need to use this mode even when normal run of a test doesn't influence any other. Omitting othervm for the tests that do not interfere with others during normal runs does make sense, and I will follow this rule from now on. IMO ideally, there should be a configurable part of the harness, where all the shell commands are set up. So that they could be accessed by both Java and shell-based regtests. test/lib/testlibrary is the place for test-suite wide infrastructure. I don't know if there are tests beyond the Process area that needs to do the same kind of thing. I meant something that can be configured by a user of the harness. It's beyond the scope of this bug, of course. Sincerely yours, Ivan
Re: RFR [6943190] TEST_BUG: java/lang/Runtime/exec/ExecWithInput.java hardcodes path to cat
Here's the (hopefully final) polished webrev: removed 'othervm' and replaced stderr with stdout in reporting of the wrong OS. Would you please take a look? http://cr.openjdk.java.net/~igerasim/6943190/4/webrev/ Sincerely yours, Ivan
Re: RFR [6943190] TEST_BUG: java/lang/Runtime/exec/ExecWithInput.java hardcodes path to cat
Thanks Martin!
On 19.03.2014 20:23, Martin Buchholz wrote:
I expected to see System.out used instead (not that it matters much).
I would write:
System.out.println('true' command not found; skipping test);
Two cases are mixed here:
- normal run (OS does not have this command),
- erroneous run (OS isn't configured correctly, so the command couldn't
be find).
I agree it's not matters much, so no problem to replace the output as
you suggest.
Here's the (final?) webrev:
http://cr.openjdk.java.net/~igerasim/6943190/5/webrev/
Sincerely yours,
Ivan
Re: RFR [6943190] TEST_BUG: java/lang/Runtime/exec/ExecWithInput.java hardcodes path to cat
On 19/03/2014 17:34, Ivan Gerasimov wrote: Here's the (final?) webrev: http://cr.openjdk.java.net/~igerasim/6943190/5/webrev/ This looks okay although I would prefer for a number of these tests to fail if the command is not found (otherwise it is will just hide issues). We can create another bug for this if you'd prefer. -Alan.
Re: RFR [6943190] TEST_BUG: java/lang/Runtime/exec/ExecWithInput.java hardcodes path to cat
On 16/03/2014 16:52, Ivan Gerasimov wrote: Would you please take a look at the updated webrev with your suggestions incorporated: http://cr.openjdk.java.net/~igerasim/6943190/3/webrev/ Ivan - I see that a number of the tests have been changed to /othervm and I'm wondering what the reason for this is. Clearly tests for Process will be launching new VMs but it would be great if we could avoid having to launch a VM for the test main too. Another general comment is that if a command is not found the tests will now pass and the only way to know that anything has gone wrong is to look in the .jtr file. I would personally prefer the tests to fail so that any issues in the environment or special-casing for new ports can be found quickly. For the Linux or Solaris-only tests then the tests now output an information message via System.err, I assume this should be System.out as it's not really an error or usage message. On the places to search then I'm wondering about /system/bin. Is that needed? I assume that if there is a port to an operating system that uses this location then it will be added as part of the port. -Alan.
Re: RFR [6943190] TEST_BUG: java/lang/Runtime/exec/ExecWithInput.java hardcodes path to cat
Thank you Alan! On 17.03.2014 11:50, Alan Bateman wrote: On 16/03/2014 16:52, Ivan Gerasimov wrote: Would you please take a look at the updated webrev with your suggestions incorporated: http://cr.openjdk.java.net/~igerasim/6943190/3/webrev/ Ivan - I see that a number of the tests have been changed to /othervm and I'm wondering what the reason for this is. Clearly tests for Process will be launching new VMs but it would be great if we could avoid having to launch a VM for the test main too. These two tests used the commands run from non very common location (/usr/bin/ instead of /bin/), so I suspect they have been rarely run. As it follows from the summaries, one of them ensures the VM doesn't crash; the other checks, whether the input/output streams are left open. Both tests in the case of a failure could interfere with other tests unless they run in the othervm mode. That's why I thought it's better to add the flag. Another general comment is that if a command is not found the tests will now pass and the only way to know that anything has gone wrong is to look in the .jtr file. I would personally prefer the tests to fail so that any issues in the environment or special-casing for new ports can be found quickly. I've tried not to change the tests' logic too much. For example, in test/java/lang/Runtime/exec/ConcurrentRead.java, if 'tee' hadn't been found, the test just silently exited. And I only added some debug output about that fact. Otherwise, if we treat the 'tee' absence as an error, we may start seeing failures in different environments, which would not be related to ConcurrentRead.java itself. IMO ideally, there should be a configurable part of the harness, where all the shell commands are set up. So that they could be accessed by both Java and shell-based regtests. For the Linux or Solaris-only tests then the tests now output an information message via System.err, I assume this should be System.out as it's not really an error or usage message. Fixed it. On the places to search then I'm wondering about /system/bin. Is that needed? I assume that if there is a port to an operating system that uses this location then it will be added as part of the port. -Alan. Sincerely yours, Ivan
Re: RFR [6943190] TEST_BUG: java/lang/Runtime/exec/ExecWithInput.java hardcodes path to cat
On 17/03/2014 09:06, Ivan Gerasimov wrote: Thank you Alan! These two tests used the commands run from non very common location (/usr/bin/ instead of /bin/), so I suspect they have been rarely run. As it follows from the summaries, one of them ensures the VM doesn't crash; the other checks, whether the input/output streams are left open. Both tests in the case of a failure could interfere with other tests unless they run in the othervm mode. That's why I thought it's better to add the flag. If a test is badly behaved and is leaving streams open then I would agree with othervm. However these are old issues (right?) and should not be happening now. Also if a test tickles a bug that causes the VM to crash then jtreg will spin up a new VM for the next test. So if it's possible to avoid othervm then we should (and from what I can tell then these tests have been well behaved when running without othervm before). : For example, in test/java/lang/Runtime/exec/ConcurrentRead.java, if 'tee' hadn't been found, the test just silently exited. Okay, it's not a big deal as it shouldn't happen but still worth considering as it would be a lot better (in my view) to not hide an issue that prevents the test from running. : IMO ideally, there should be a configurable part of the harness, where all the shell commands are set up. So that they could be accessed by both Java and shell-based regtests. test/lib/testlibrary is the place for test-suite wide infrastructure. I don't know if there are tests beyond the Process area that needs to do the same kind of thing. For the Linux or Solaris-only tests then the tests now output an information message via System.err, I assume this should be System.out as it's not really an error or usage message. Fixed it. Thanks. -Alan.
Re: RFR [6943190] TEST_BUG: java/lang/Runtime/exec/ExecWithInput.java hardcodes path to cat
Thank you Martin, for your comments!
On 16.03.2014 4:41, Martin Buchholz wrote:
Thanks for trying to make these tests more robust.
33 private static final String[] paths = {/bin, /usr/bin,
34 /usr/local/bin, /system/bin, /sbin, /usr/bin/sbin,
35 /usr/local/sbin, /system/sbin};
I think in practice { /bin, /usr/bin } would work very well. I
would definitely leave out /usr/local, since that is a location for
non-standard program variants to be installed. I don't know anything
about /system - I would leave it out. Pedants would suggest getting
the system default PATH using getconf PATH, and that might get you
/usr/xpg4/bin, but probably overkill here.
I agree, and I'll make this search list shorter.
I wouldn't fall back to looking on PATH. There should be as few
dependencies on the user's environment as possible.
I imagine the perfect world with this information provided to the test
by the harness tool.
These commands are executed from shell scripts too, and I've seen the
same problem with hard-coded path.
Take https://bugs.openjdk.java.net/browse/JDK-6543375, for example.
The fix there was to remove the absolute path and rely on the PATH variable.
I would leave out the crufty windows-checking code from each test, and
instead write test code like:
If (UnixCommands.tee() == null || UnixCommands.cat() == null)
return;
Done. I kept the check for specific OS, where it is required by the test
itself.
Would you please take a look at the updated webrev with your suggestions
incorporated:
http://cr.openjdk.java.net/~igerasim/6943190/3/webrev/
Sincerely yours,
Ivan
Imagine these tests will be run on some other strange OS someday.
On Fri, Mar 14, 2014 at 9:58 AM, Ivan Gerasimov
ivan.gerasi...@oracle.com mailto:ivan.gerasi...@oracle.com wrote:
Hello!
This is a friendly reminder.
Can someone with the Reviewer status please help review this?
In short: we have a few java tests that run shell commands.
In some tests the path to the command is omitted, in other tests
an absolute path is given.
In the third group of tests, the *other* absolute path to the same
commands is used.
As Martin suggested, I added searching for the command at certain
fixed directories before using it.
The final webrev can be found here:
http://cr.openjdk.java.net/~igerasim/6943190/2/webrev/
http://cr.openjdk.java.net/%7Eigerasim/6943190/2/webrev/
Thanks in advance,
Ivan
Re: RFR [6943190] TEST_BUG: java/lang/Runtime/exec/ExecWithInput.java hardcodes path to cat
Hello! This is a friendly reminder. Can someone with the Reviewer status please help review this? In short: we have a few java tests that run shell commands. In some tests the path to the command is omitted, in other tests an absolute path is given. In the third group of tests, the *other* absolute path to the same commands is used. As Martin suggested, I added searching for the command at certain fixed directories before using it. The final webrev can be found here: http://cr.openjdk.java.net/~igerasim/6943190/2/webrev/ Thanks in advance, Ivan
Re: RFR [6943190] TEST_BUG: java/lang/Runtime/exec/ExecWithInput.java hardcodes path to cat
Thank you Roger for looking at this. On 27.02.2014 2:19, roger riggs wrote: Hi Ivan, A few comments on UnixCommands: - The trailing _ on the method names looks very odd, they could all use some suffix Pgm X, etc. I replaced true_() and false_() with explicit calls to findCommand(). - I'm not sure the specific command methods do you any good (over a method with a string argument). since they immediately revert to the string command name. They are shorter :-) - The final 0 in findCommand0 is using the convention for a native method but it is not native. Yes. But I think the convention is more general -- the f0() is usually a function of a lower level than f(). Grepping shows some examples: ./jdk/share/classes/java/security/KeyStore.java: run0() is called from run() ./jdk/share/classes/java/util/regex/Pattern.java: match0() is called from match() and In the tests, main0() sometimes is called from main(), test0() from test(), etc. No problem to rename it, but I don't think it should confuse too much. - Use the File methods for concatenation rather than string concatenation. i.e. new File (parent, child). Good point, thanks! Please find the updated webrev here: http://cr.openjdk.java.net/~igerasim/6943190/2/webrev/ Sincerely yours, Ivan Roger On 2/26/2014 4:30 PM, Ivan Gerasimov wrote: On 24.02.2014 23:38, Martin Buchholz wrote: Thanks for working on these ancient Process tests. I would prefer to see them using feature tests. You wanna do cat /dev/zero? Then look for cat in /bin and /usr/bin and check for /dev/zero as well, e.g. using File.isExecutable OK. Here's another iteration. I introduced a utility class UnixCommands, which encapsulates searching for the command under several fixed directories. I didn't touch /dev/null though. I think we can assume it exists in every Unix system. Many other tests depend on it anyways. Would you please have a chance to review the updated wevrev? http://cr.openjdk.java.net/~igerasim/6943190/1/webrev/ Sincerely yours, Ivan Doing OS-specific things like: +static final boolean isLinux = +System.getProperty(os.name http://os.name, unknown).startsWith(Linux); should be a last resort. On Mon, Feb 24, 2014 at 9:26 AM, Ivan Gerasimov ivan.gerasi...@oracle.com mailto:ivan.gerasi...@oracle.com wrote: Hello! We've got one quite old report about been unable to run the test under Android. https://bugs.openjdk.java.net/browse/JDK-6943190 That was due to hard-coded path to the cat utility as /bin/cat. When investigating, I found two other tests under the same directory that use /usr/bin/cat and /usr/bin/echo. These two test seem to (almost) never run because of the unusual path. Here's the first version of the webrev, with the fix to only three tests mentioned above: http://cr.openjdk.java.net/~igerasim/6943190/0/webrev/ http://cr.openjdk.java.net/%7Eigerasim/6943190/0/webrev/ java/lang/Runtime/exec/ directory has also got several other tests, which run shell commands. Some of them have absolute path and some of them don't. So I have a general question: Why would we ever need the absolute path for the commands? Why wouldn't we rely on the PATH env variable? Sincerely yours, Ivan
Re: RFR [6943190] TEST_BUG: java/lang/Runtime/exec/ExecWithInput.java hardcodes path to cat
Hi Ivan, On 2/27/2014 4:34 AM, Ivan Gerasimov wrote: - I'm not sure the specific command methods do you any good (over a method with a string argument). since they immediately revert to the string command name. They are shorter :-) yes, but it creates a level of indirection that someone reading the code has to chase through. To me it would create less code and make for easier reading to call findCommand in all cases. $.02, Roger ... Please find the updated webrev here: http://cr.openjdk.java.net/~igerasim/6943190/2/webrev/ Sincerely yours, Ivan
Re: RFR [6943190] TEST_BUG: java/lang/Runtime/exec/ExecWithInput.java hardcodes path to cat
On 24.02.2014 23:38, Martin Buchholz wrote: Thanks for working on these ancient Process tests. I would prefer to see them using feature tests. You wanna do cat /dev/zero? Then look for cat in /bin and /usr/bin and check for /dev/zero as well, e.g. using File.isExecutable OK. Here's another iteration. I introduced a utility class UnixCommands, which encapsulates searching for the command under several fixed directories. I didn't touch /dev/null though. I think we can assume it exists in every Unix system. Many other tests depend on it anyways. Would you please have a chance to review the updated wevrev? http://cr.openjdk.java.net/~igerasim/6943190/1/webrev/ Sincerely yours, Ivan Doing OS-specific things like: +static final boolean isLinux = +System.getProperty(os.name http://os.name, unknown).startsWith(Linux); should be a last resort. On Mon, Feb 24, 2014 at 9:26 AM, Ivan Gerasimov ivan.gerasi...@oracle.com mailto:ivan.gerasi...@oracle.com wrote: Hello! We've got one quite old report about been unable to run the test under Android. https://bugs.openjdk.java.net/browse/JDK-6943190 That was due to hard-coded path to the cat utility as /bin/cat. When investigating, I found two other tests under the same directory that use /usr/bin/cat and /usr/bin/echo. These two test seem to (almost) never run because of the unusual path. Here's the first version of the webrev, with the fix to only three tests mentioned above: http://cr.openjdk.java.net/~igerasim/6943190/0/webrev/ http://cr.openjdk.java.net/%7Eigerasim/6943190/0/webrev/ java/lang/Runtime/exec/ directory has also got several other tests, which run shell commands. Some of them have absolute path and some of them don't. So I have a general question: Why would we ever need the absolute path for the commands? Why wouldn't we rely on the PATH env variable? Sincerely yours, Ivan
Re: RFR [6943190] TEST_BUG: java/lang/Runtime/exec/ExecWithInput.java hardcodes path to cat
Hi Ivan, A few comments on UnixCommands: - The trailing _ on the method names looks very odd, they could all use some suffix Pgm X, etc. - I'm not sure the specific command methods do you any good (over a method with a string argument). since they immediately revert to the string command name. - The final 0 in findCommand0 is using the convention for a native method but it is not native. - Use the File methods for concatenation rather than string concatenation. i.e. new File (parent, child). Roger On 2/26/2014 4:30 PM, Ivan Gerasimov wrote: On 24.02.2014 23:38, Martin Buchholz wrote: Thanks for working on these ancient Process tests. I would prefer to see them using feature tests. You wanna do cat /dev/zero? Then look for cat in /bin and /usr/bin and check for /dev/zero as well, e.g. using File.isExecutable OK. Here's another iteration. I introduced a utility class UnixCommands, which encapsulates searching for the command under several fixed directories. I didn't touch /dev/null though. I think we can assume it exists in every Unix system. Many other tests depend on it anyways. Would you please have a chance to review the updated wevrev? http://cr.openjdk.java.net/~igerasim/6943190/1/webrev/ Sincerely yours, Ivan Doing OS-specific things like: +static final boolean isLinux = +System.getProperty(os.name http://os.name, unknown).startsWith(Linux); should be a last resort. On Mon, Feb 24, 2014 at 9:26 AM, Ivan Gerasimov ivan.gerasi...@oracle.com mailto:ivan.gerasi...@oracle.com wrote: Hello! We've got one quite old report about been unable to run the test under Android. https://bugs.openjdk.java.net/browse/JDK-6943190 That was due to hard-coded path to the cat utility as /bin/cat. When investigating, I found two other tests under the same directory that use /usr/bin/cat and /usr/bin/echo. These two test seem to (almost) never run because of the unusual path. Here's the first version of the webrev, with the fix to only three tests mentioned above: http://cr.openjdk.java.net/~igerasim/6943190/0/webrev/ http://cr.openjdk.java.net/%7Eigerasim/6943190/0/webrev/ java/lang/Runtime/exec/ directory has also got several other tests, which run shell commands. Some of them have absolute path and some of them don't. So I have a general question: Why would we ever need the absolute path for the commands? Why wouldn't we rely on the PATH env variable? Sincerely yours, Ivan
RFR [6943190] TEST_BUG: java/lang/Runtime/exec/ExecWithInput.java hardcodes path to cat
Hello! We've got one quite old report about been unable to run the test under Android. https://bugs.openjdk.java.net/browse/JDK-6943190 That was due to hard-coded path to the cat utility as /bin/cat. When investigating, I found two other tests under the same directory that use /usr/bin/cat and /usr/bin/echo. These two test seem to (almost) never run because of the unusual path. Here's the first version of the webrev, with the fix to only three tests mentioned above: http://cr.openjdk.java.net/~igerasim/6943190/0/webrev/ java/lang/Runtime/exec/ directory has also got several other tests, which run shell commands. Some of them have absolute path and some of them don't. So I have a general question: Why would we ever need the absolute path for the commands? Why wouldn't we rely on the PATH env variable? Sincerely yours, Ivan
