The parameter value for a JdbcInterceptor config is parsed incorrectly if there is any white-space following the closing parentheses (it includes the closing paren in the parameter value). I will be attaching two items momentarily: * a patch to resolve the issue * a unit test that fails in TRUNK but passes post-patch. The parser can behave unpredictably in several other boundary cases (extra white-space, spurious semicolons, empty parm lists, etc.) so I have attempted to document the current behavior in the unit test for clarity. It may be worth considering a more uniform response to incorrect config(s), but I skipped that here to keep it simple. I may submit an enhancement request later (with some suggestions as a patch).
Created attachment 29833 [details] Patch for PoolProperties.java
Created attachment 29835 [details] New unit test for parser
FWIW, test cases using the following idiom: caught = null; try { props.getJdbcInterceptorsAsArray(); } catch (Exception e) { caught = e; } assertNull(caught); can be written somewhat more simply like this: try { props.getJdbcInterceptorsAsArray(); } catch (Exception e) { assertFail("Didn't expect exception"); } Likewise: caught = null; try { props.getJdbcInterceptorsAsArray(); } catch (Exception e) { caught = e; } assertNotNull(caught); can be written like this: try { props.getJdbcInterceptorsAsArray(); assertFail("Expected Exception"); } catch (Exception e) { // Expected }
Created attachment 29836 [details] Updated unit test per comments Well, the JUnit 3.8.1 pulled in by Maven doesn't have an "assertFail(String)" method, but I think the "fail(String)" alternative is there in spirit. Point(s) taken, unit test attachment updated. I had been doing more with the Exception in case 1, and didn't refactor far enough when that changed, and case 2 was copy/paste laziness (blah,blah). Thanks for the good suggestions.
(In reply to comment #3) > FWIW, test cases using the following idiom: > (..) > can be written somewhat more simply like this: > > try { > props.getJdbcInterceptorsAsArray(); > } catch (Exception e) { > assertFail("Didn't expect exception"); > } 1. Just add "throws Exception" to the test method. JUnit notices the thrown exception and the test does not pass. 2. It is the first time I am seeing "assertFail". A typo? The method that I know about is called fail(message) (Assert.fail(..)).
Sorry, yes, I meant "fail()". As for "throws Exception" doesn't that change a failure into an "error" when JUnit runs?
Love nothing more than receiving tests The assertions are backwards (parameters swapped), other than that looks great. org.junit.ComparisonFailure: Expected :value2 ) Actual :value2 Thanks for the report Fixed in r1616599