fix tests

fix tests

[JohnL, 2016-08-30 22:46:36]

"Fix tests" is not a very helpful commit message. Maybe "Ensure tests threw the
expected exception."

John L

https://codereview.appspot.com/306370043/diff/1/test/com/google/enterprise/ad...
File test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java
(right):

https://codereview.appspot.com/306370043/diff/1/test/com/google/enterprise/ad...
test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java:123:
boolean catched = false;
s/catched/caughtException/ or something, but the traditional idiom is to call
fail("Should have thrown an exception") just after the statement that should
have thrown an exception.

[imysak, 2016-08-30 23:07:04]

On 2016/08/30 22:46:36, JohnL wrote:
> "Fix tests" is not a very helpful commit message. Maybe "Ensure tests threw
the
> expected exception."
> 
> John L
> 
>
https://codereview.appspot.com/306370043/diff/1/test/com/google/enterprise/ad...
> File test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java
> (right):
> 
>
https://codereview.appspot.com/306370043/diff/1/test/com/google/enterprise/ad...
> test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java:123:
> boolean catched = false;
> s/catched/caughtException/ or something, but the traditional idiom is to call
> fail("Should have thrown an exception") just after the statement that should
> have thrown an exception.

Hi, thanks.
I agree with commit message, my bad.

I use this "catched"-flag construction such as in this test we need to catch
Exception together with verification error message.

If test method throw Exception then we can't check anything after this method
without catching exception manually(int try-catch block) to be able check
anything.
Possible, there are could be better solutions but I didn't found any.

[JohnL, 2016-09-26 22:24:11]

On 2016/08/30 23:07:04, imysak wrote:
> On 2016/08/30 22:46:36, JohnL wrote:
> > "Fix tests" is not a very helpful commit message. Maybe "Ensure tests threw
> the
> > expected exception."
> > 
> > John L
> > 
> >
>
https://codereview.appspot.com/306370043/diff/1/test/com/google/enterprise/ad...
> > File test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java
> > (right):
> > 
> >
>
https://codereview.appspot.com/306370043/diff/1/test/com/google/enterprise/ad...
> > test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java:123:
> > boolean catched = false;
> > s/catched/caughtException/ or something, but the traditional idiom is to
call
> > fail("Should have thrown an exception") just after the statement that should
> > have thrown an exception.
> 
> Hi, thanks.
> I agree with commit message, my bad.
> 
> I use this "catched"-flag construction such as in this test we need to catch
> Exception together with verification error message.
> 
> If test method throw Exception then we can't check anything after this method
> without catching exception manually(int try-catch block) to be able check
> anything.
> Possible, there are could be better solutions but I didn't found any.

Is anything happening with this CL?

I don't understand what you mean by "we can't check anything after this method",
because there isn't anything in the test after throwing (and catching) the
exception. Here's the idiom I was describing:

    try {
      adaptor.init(TestHelper.createConfigAdaptorContext(config));
      fail("Expected an InvalidConfigurationException");
    } catch (InvalidConfigurationException ice) {
      assertTrue(ice.getMessage().contains(
          "requires docId.isUrl to be \"true\""));
    }

John L

[imysak, 2016-10-26 00:35:28]

On 2016/09/26 22:24:11, JohnL wrote:
> On 2016/08/30 23:07:04, imysak wrote:
> > On 2016/08/30 22:46:36, JohnL wrote:
> > > "Fix tests" is not a very helpful commit message. Maybe "Ensure tests
threw
> > the
> > > expected exception."
> > > 
> > > John L
> > > 
> > >
> >
>
https://codereview.appspot.com/306370043/diff/1/test/com/google/enterprise/ad...
> > > File test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java
> > > (right):
> > > 
> > >
> >
>
https://codereview.appspot.com/306370043/diff/1/test/com/google/enterprise/ad...
> > > test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java:123:
> > > boolean catched = false;
> > > s/catched/caughtException/ or something, but the traditional idiom is to
> call
> > > fail("Should have thrown an exception") just after the statement that
should
> > > have thrown an exception.
> > 
> > Hi, thanks.
> > I agree with commit message, my bad.
> > 
> > I use this "catched"-flag construction such as in this test we need to catch
> > Exception together with verification error message.
> > 
> > If test method throw Exception then we can't check anything after this
method
> > without catching exception manually(int try-catch block) to be able check
> > anything.
> > Possible, there are could be better solutions but I didn't found any.
> 
> Is anything happening with this CL?
> 
> I don't understand what you mean by "we can't check anything after this
method",
> because there isn't anything in the test after throwing (and catching) the
> exception. Here's the idiom I was describing:
> 
>     try {
>       adaptor.init(TestHelper.createConfigAdaptorContext(config));
>       fail("Expected an InvalidConfigurationException");
>     } catch (InvalidConfigurationException ice) {
>       assertTrue(ice.getMessage().contains(
>           "requires docId.isUrl to be \"true\""));
>     }
> 
> John L

hi John, I guess I just confused this CL with another one.

I agree that your idiom is much better. Thanks.

[JohnL, 2016-10-27 06:26:01]

LGTM, though I was just a lurker here.

PJ may have preferences about my suggestion.

John L

https://codereview.appspot.com/306370043/diff/40001/test/com/google/enterpris...
File test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java
(right):

https://codereview.appspot.com/306370043/diff/40001/test/com/google/enterpris...
test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java:127:
assertTrue(ice.getMessage().contains(
Since we're improving these tests, adding a message to assertTrue would help a
lot when diagnosing test failures:

  assertTrue(ice.toString(),
    ice.getMessage().contains("requires docId.isUrl to be \"true\""));

Some people don't like tests to fail by throwing a non-AssertionError, but I
actually prefer it, and would generally write this line by rethrowing:

  if (!ice.getMessage().contains("requires docId.isUrl to be \"true\"")) {
    throw e;
  }

This way you get the full stack trace (with causes, if any) if it fails.

[pjo, 2016-10-30 02:29:55]

Happy with John's suggestions. Thank you

[imysak, 2016-11-01 16:59:44]

Thanks John, I got your point, I updated code follow your suggestions.

[JohnL, 2016-11-01 23:24:52]

Trivial comments.

John L

https://codereview.appspot.com/306370043/diff/80001/test/com/google/enterpris...
File test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java
(right):

https://codereview.appspot.com/306370043/diff/80001/test/com/google/enterpris...
test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java:127:
if(!ice.getMessage().contains("requires docId.isUrl to be \"true\"")) {
Both here and below: s/if/if /

https://codereview.appspot.com/306370043/diff/80001/test/com/google/enterpris...
test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java:128: throw
new Error("Error message doesn't match expected", ice);
Both here and below: s/new Error/new AssertionError/ for consistency with fail
and the assert methods.

[imysak, 2016-11-02 01:01:48]

The is one issue here.
Firstly I tried to throw AssertionError, but the problem is that locally(with
ant) it print not full stacktrace:

    [junit] Testcase: testInitOfUrlMetadataListerNoDocIdIsUrl took 0.002 sec
    [junit] 	FAILED
    [junit] Error message doesn't match expected
    [junit] junit.framework.AssertionFailedError: Error message doesn't match
expected
    [junit] 	at
com.google.enterprise.adaptor.database.DatabaseAdaptorTest.testInitOfUrlMetadataListerNoDocIdIsUrl(DatabaseAdaptorTest.java:130)
    [junit] 


The good thing is with AssertionError the test is marked as FAILED, // but it
suppress initial exception stacktrace.

With throwing Error I got better stacktrace: 
    [junit] Testcase: testInitOfUrlMetadataListerNoDocIdIsUrl took 0.005 sec
    [junit] 	Caused an ERROR
    [junit] Error message doesn't match expected
    [junit] java.lang.Error: Error message doesn't match expected
    [junit] 	at
com.google.enterprise.adaptor.database.DatabaseAdaptorTest.testInitOfUrlMetadataListerNoDocIdIsUrl(DatabaseAdaptorTest.java:130)
    [junit] Caused by:
com.google.enterprise.adaptor.InvalidConfigurationException: db.modeOfOperation
of "urlAndMetadataLister" requires docId.isUrl to be "true"
    [junit] 	at
com.google.enterprise.adaptor.database.DatabaseAdaptor.init(DatabaseAdaptor.java:200)
    [junit] 	at
com.google.enterprise.adaptor.database.DatabaseAdaptorTest.testInitOfUrlMetadataListerNoDocIdIsUrl(DatabaseAdaptorTest.java:126)

However the test is marked as ERROR, what is not correct actually.

P.S. I just found that Eclipse provide you full stacktrace in case of using
AssertionError.

I can change Error on AssertionError for consistency if it is better. Please
advice.

[JohnL, 2016-11-02 01:11:13]

On 2016/11/02 01:01:48, imysak wrote:
> The is one issue here.
> Firstly I tried to throw AssertionError, but the problem is that locally(with
> ant) it print not full stacktrace:
> 
>     [junit] Testcase: testInitOfUrlMetadataListerNoDocIdIsUrl took 0.002 sec
>     [junit] 	FAILED
>     [junit] Error message doesn't match expected
>     [junit] junit.framework.AssertionFailedError: Error message doesn't match
> expected
>     [junit] 	at
>
com.google.enterprise.adaptor.database.DatabaseAdaptorTest.testInitOfUrlMetadataListerNoDocIdIsUrl(DatabaseAdaptorTest.java:130)
>     [junit] 
> 
> 
> The good thing is with AssertionError the test is marked as FAILED, // but it
> suppress initial exception stacktrace.
> 
> With throwing Error I got better stacktrace: 
>     [junit] Testcase: testInitOfUrlMetadataListerNoDocIdIsUrl took 0.005 sec
>     [junit] 	Caused an ERROR
>     [junit] Error message doesn't match expected
>     [junit] java.lang.Error: Error message doesn't match expected
>     [junit] 	at
>
com.google.enterprise.adaptor.database.DatabaseAdaptorTest.testInitOfUrlMetadataListerNoDocIdIsUrl(DatabaseAdaptorTest.java:130)
>     [junit] Caused by:
> com.google.enterprise.adaptor.InvalidConfigurationException:
db.modeOfOperation
> of "urlAndMetadataLister" requires docId.isUrl to be "true"
>     [junit] 	at
>
com.google.enterprise.adaptor.database.DatabaseAdaptor.init(DatabaseAdaptor.java:200)
>     [junit] 	at
>
com.google.enterprise.adaptor.database.DatabaseAdaptorTest.testInitOfUrlMetadataListerNoDocIdIsUrl(DatabaseAdaptorTest.java:126)
> 
> However the test is marked as ERROR, what is not correct actually.
> 
> P.S. I just found that Eclipse provide you full stacktrace in case of using
> AssertionError.
> 
> I can change Error on AssertionError for consistency if it is better. Please
> advice.

Hmm, so JUnit or Ant has some special treatment we don't want. OK. Well,
personally, I wouldn't include the wrapper at all, I don't see the added value
beyond "throw ice;", but if you want it, I would wrap ice in a RuntimeException
then, rather than an Error (throwing an Error other than AssertionError just
seems wrong). If you prefer Error I'm OK with that, I don't think it's confusing
or a source of potential problems.

The distinction between failures and errors seems like a very silly one to me,
but I know other people think tests should never throw errors. That's what I was
pointing out in my earlier comment. I want my hands on the stack trace and don't
really care which column the test failure is counted in.

John L

[imysak, 2016-11-02 17:59:47]

Replacing Error with RuntimeException

[JohnL, 2016-11-02 22:41:45]

Still need to fix the lint errors and add spaces after the "if" in both places.

John L

[imysak, 2016-11-03 00:30:24]

Thanks John, fixed

[JohnL, 2016-11-04 01:22:21]

LGTM

John L

[JohnL, 2016-11-11 23:02:45]

Committed:
https://github.com/googlegsa/database/commit/755cabb68b601d49023c507097a8da4e...

Please close this review.

John L