Closed GoogleCodeExporter closed 9 years ago
Was this fixed in R603?
Original comment by ThomasCR...@gmail.com
on 11 Sep 2011 at 8:05
Not yet.
What is fixed is that none of the tests actually hang. But if you change the
command line flags in the search calls to pass in an unrecognized flag, the
tests hand instead of exiting. Basically any call to TestUtils.getCommandOutput
is suspect. We could get rid of this call and make a programmatic call within
the same JVM, that might be better.
Original comment by widd...@google.com
on 11 Sep 2011 at 2:59
This is because TestUtils.getCommandOutput(Class, String[]) wasn't built to
expect stderr. I'm not convinced that is a good assumption, given how easy it
is to make a typo and cause the process to hang. An easy fix is to take the
following line from TestUtils:
Process p = spawnChildProcess(childMain, args, null, outputStream, null);
and change it into:
Process p = spawnChildProcess(childMain, args, null, outputStream,
outputStream);
The above change will send stderr to the same output stream, which provides
feedback to the test developer and causes stderr to drain, preventing a hang.
I'm going to consider committing this modification for at least a day. I'm not
sure why stderr wasn't sent to the outputStream from the beginning, and if
there was a reason that I'd like to account for it. For example, another
approach would be to have a separate stream for stderr, changing the output
format to better identify that the source was an error.
I will be back within one or more days to make this change after I've had some
time to let it sink in.
Original comment by ThomasCR...@gmail.com
on 12 Sep 2011 at 12:51
A few more thoughts:
• Writing both STDERR and STDOUT to the same stream can interleave both
outputs if both are writing "at the same time". This is a small consideration,
developers will figure it out.
• I'm cautious about making programatic calls to main() functions. Bugs that
arise from calling main() are often platform dependent and can require runtime
debugging to find. For example, Issue 30 only affected Windows JVMs.
• RegressionTests.java has at least one call to a main() function that I'd
also like to remove using the scanner construct provided in TestUtils.
• On the other hand, developers prefer to just call the main function. It
looks easy and works 95% of the time. Nobody wants more complexity.
Original comment by ThomasCR...@gmail.com
on 12 Sep 2011 at 1:10
Thanks for the proposals.
The current spawnChildProcess call was written by me without much particular
knowledge of what Java developers expect when talking to different filesytems.
Please go ahead and make improvements. Programmatic calls to main() should go
if there are more reliable ways to test the same computation and side effects.
The current integration tests call main() programmatically for index building,
and TestUtils.getCommandOutput() for searching - I'm not sure if there's any
good reason why I did them differently, so any more principled changes you put
forward will probably be good ones.
Original comment by widd...@google.com
on 12 Sep 2011 at 1:42
The fix proposed in comment 3 above shouldn't be checked in yet because it
causes the other regression tests to fail. In particular,
RegressionTests.java:78 occasionally has a nextTerm that is null, so the
nextTerm.equals(String) test fails.
At the moment I don't have time to find the solution, but I'll get back to it
soon. Meanwhile, I'll add a few related notes.
• if STDERR should be entirely ignored, I can make the right changes. This
would make the tests run as always, but any errors might get lost in the
shuffle.
• if the tests should be reworked to account for output on STDERR, then I can
do that too.
• I've looked at TestUtils some more and some concerns of using a single
output scanner for STDOUT and STDERR are already addressed. The utility already
detects a single stream and uses the Process redirection facilities to do
things right per VM (supposedly). What I'm saying here is that the fix proposed
in comment 3, above, is capable of working.
Original comment by ThomasCR...@gmail.com
on 12 Sep 2011 at 2:17
I have a little more to add now:
• getCommandOutput is working correctly
• STDERR is already accounted-for and drained properly, modeling what was
done before which is to say STDERR is ignored
• when the search command is passed invalid input it generates no output on
STDOUT, but it does generate output on STDERR
• if you follow the fix outlined in post Commend 3, above, you will generate
output on STDOUT because you've essentially piped STDERR to STDOUT too
• Scanner.hasNext() is a blocking call
• when the Scanner's OutputStream is empty, Scanner.hasNext() will block and
wait for data
• if you have no data, the scanner will block forever, which is what appears
to be occurring here
Ideally, I could ask the Scanner if it has data without blocking, and if no
data ever existed then move on to the next thing instead of blocking. But I
don't think Scanner allows this directly, so I need to consider another
approach.
Original comment by ThomasCR...@gmail.com
on 13 Sep 2011 at 3:02
Perhaps calling Search.RunSearch would be a better option and get rid of the
Scanner over output altogether?
The command line flags are parsed within RunSearch, so its behavior should be
more or less identical to the call to main().
Original comment by widd...@google.com
on 13 Sep 2011 at 3:51
Much of this was fixed in revision 631
(http://code.google.com/p/semanticvectors/source/detail?r=631).
Apologies for not communicating this here sooner.
Now instead of spawning a new process and getting a Scanner, tests call
Search.RunSearch.
This has solved the problem of hanging or otherwise hiding bad results in
tests. It has introduced the problem that there is a lot of persistent state
between tests kept in the Flags configuration. This really does show how
problematic the current Flags implementation is, it needs to be replaced
(probably) with a global one-time config.
Original comment by widd...@google.com
on 14 Nov 2011 at 2:51
Original comment by widd...@google.com
on 14 Nov 2011 at 2:51
Original issue reported on code.google.com by
widd...@google.com
on 28 Aug 2011 at 1:41