[EMAIL PROTECTED] wrote:
"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"...
I think that's always an issue, finding the best method for the job. I
don't think putting them in a single class is the best solution. JUnit
doesn't do that, and neither does the JDK. There are a couple of ways to
find methods:
1) Look at the javadoc, Andrew recently made the testing javadoc be
built nightly:
http://db.apache.org/derby/javadoc/testing/
Focus on the org.apache.derbyTesting.junit package
2) Ask a question on derby-dev, allows other to help out and see the
solution. I highly recommend this approach.
3) Look at other tests, if a test seems to have its own method that
you think should be a common utility, make it a common method, and/or
ask derby-dev why the test has its own version.
And of course, once you have found the method if you can think of any
improvement in javadoc, wiki pages etc. that would help the next person
find the method, then make the improvement.
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...
So improve the utility methods. The standard JUnit Assert methods have
versions that take no message and those that take a message, so there's
no difference here apart from no-one has scratched the itch to add a
version with a passed in message. No-one is saying the current Derby
junit setup is perfect, it's a work in progress, as people find needs
they implement utility methods.
One comment on the loop indices in assert messages. This style of code
can be expensive at runtime:
for (int i = 0; i < count; i++) {
assertEquals("Testing at iteration " + i, f(i), g(i));
}
The issue is the message will be created 'count' times even though it is
never used. This takes up cpu time and garbage collection time.
In the Derby sanity code this style was a problem, causing extended test
running time, so the encouraged practice (for Derby sanity checks) is
if (SanityManager.ASSERT)
{
if (X != Y)
SanityManager.THROWASSERT("Value should be " + X + " is " + Y);
}
rather than
if (SanityManager.ASSERT)
{
SanityManager.ASSERT(X == Y, "Value should be " + X + " is " + Y);
}
I'm not sure what should be the best practice for JUnit tests should be,
but based upon past experience when I see a loop with a complex
iteration specific assert message I worry about execution time.
In some cases the message can be fixed, e.g. this coding style should
not be encouraged:
assertEquals("Expected " + X + " to be the equal to " + Y, X, Y);
This is because the JUnit mechanism will print out the value of X and Y,
so no need to repeat it in a generated message.
Dan.
