"Knut Anders Hatlen (JIRA)" <[EMAIL PROTECTED]> writes: > [ > https://issues.apache.org/jira/browse/DERBY-827?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12477237 > ] > > Knut Anders Hatlen commented on DERBY-827: > ------------------------------------------ > > Thanks for writing the test, Dyre. I think it is a valuable addition > to the test suite. I committed rsfromps.v1 with revision 513679. > > Some small comments: > > - assertRow() and assertResultSet() duplicate functionality that is > already implemented in JDBC.assertRowInResultSet() and > JDBC.assertFullResultSet(). Perhaps they can be reused?
A couple of things about this. When I started working on this test I started by copying lang.GroupByExpressionTest since that seemed like a pretty standard lang JUnit test. I observed that it had its own methods for verifying that result sets. I found that a bit odd, since that seemed something every test has to do, so I checked what was available in BaseJDBCTestCase since GroupByExpressionTest inherits from it. When I didn't find anything I decided to use those that I had copied. I ended up making quite a few modifications to them while writing the test because I needed additional functionality to debug the failures I saw during development. Now, I absolutely see the value in having such utilites in a location where it can be shared by all tests, but do we really need to spread these utilities to so many places? Makes it hard for someone who isn't a JUnit wizard when you can't find tests that actually follow "established best practices"... Well, I tried using the asserts from JDBC, but why don't they follow the standard JUnit assert pattern, (assertX(String message, T expected, T returned))? This isn't just a style issue, in this test I have relied heavily on passing context information, such as loop indices and prepared statement parameters, down to the actual assert method so that it gets displayed when something fails (also borrowed from GroupByExpressionTest). That info will just be lost if I switch to JDBC.assertFullResultSet... When I tried letting my assertResultSet call JDBC.assertFullResultSet() I got ArrayIndexOutOfBoundsExceptions all over the place since I use the assert method to test empty resultsets against zero-length Object arrays... -- dt
