search

dileepajayakody / semanticvectors

Automatically exported from code.google.com/p/semanticvectors
Other
1 stars 0 forks source link

Integration tests hang rather than exiting when something breaks. #50

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
What steps will reproduce the problem?
1. Comment back in the dormant code in testBuildAndSearchBinaryPermutationIndex.
2. Run "ant run-integration-tests".

What is the expected output? What do you see instead?

You should see a lot of in-process logging and eventually see the number of 
passing and failing tests at the end. Instead the end is never reached.

Original issue reported on code.google.com by widd...@google.com on 28 Aug 2011 at 1:41

GoogleCodeExporter commented 9 years ago 
Was this fixed in R603?

Original comment by ThomasCR...@gmail.com on 11 Sep 2011 at 8:05

GoogleCodeExporter commented 9 years ago 
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

GoogleCodeExporter commented 9 years ago 
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

GoogleCodeExporter commented 9 years ago 
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

GoogleCodeExporter commented 9 years ago 
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

GoogleCodeExporter commented 9 years ago 
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

GoogleCodeExporter commented 9 years ago 
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

GoogleCodeExporter commented 9 years ago 
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

GoogleCodeExporter commented 9 years ago 
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

GoogleCodeExporter commented 9 years ago

Original comment by widd...@google.com on 14 Nov 2011 at 2:51