Hi Craig,
Thanks for the suggestion.
I have entered a RFE for this.
https://glassfish.dev.java.net/issues/show_bug.cgi?id=1420
-Tom
Craig L Russell wrote:
>Hi,
>
>It might be worthwhile to look at adding some behavior to the toplink
>base class JUnitTestCase to allow a subclass to acquire an
>EntityManager that will be cleaned up by the tearDown method in the
>base class.
>
>Maybe add a protected field EntityManager em to the base class.
>Subclass tests could either clean up their own EntityManager using a
>try finally pattern or if they assigned the base class em field could
>simply throw an exception and exit. At the end of the test, if the em
>field is not null, the base class tearDown would clean it up.
>
>This pattern provides for cleaning up the EntityManager without
>duplicating or adding code to each test case.
>
> From my experience, it's difficult to look at a test case that does
>its own cleanup, because each test writer's ideas of what needs to be
>cleaned up tends to be different. Some tests use a try catch where
>the catch cleans up, some don't clean up at all, some use try finally.
>
>It's more efficient to have common code that can evolve as our
>understanding of how to clean up evolves.
>
>Craig
>
>On Nov 2, 2006, at 6:09 AM, Tom Ware wrote:
>
>
>
>>Hi Marina,
>>
>> Comment inline:
>><snip>
>>
>>
>>
>>>My proposal is to add
>>> em.close();
>>> throw e; (or ex or whatever is the variable name)
>>>where it is missing.
>>>
>>>
>>>
>>>
>>>> If you are talking about additional changes, in general, when we
>>>>get an exception, it is ok to close the em and throw the
>>>>exception. We just have to ensure there is not any later code in
>>>>the test that will make that an issue.
>>>>
>>>>
>>>>
>>>But if we throw an exception, the rest of the test won't be executed
>>>(vs. now when it proceeds as the exception is ignored).
>>>
>>>thanks,
>>>-marina
>>>
>>>
>>>
>>I am generally ok, with making that kind of change. Obviously, you
>>will have to decide on a case by case basis whether it makes sense
>>to make the change for individual tests. As an example (one you
>>have dealt with well in the code you have already sent), there is
>>some code that cleans up after tests are complete that
>>intentionally ignores exceptions. That code should remain as it
>>is. For each case, you should ask yourself if the test can
>>continue to do what it is intended to do if the exception is
>>caught, and if not, make the change.
>>
>>-Tom
>>
>>
>>
>>>>-Tom
>>>>
>>>>
>>>>Marina Vatkina wrote:
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>>Hi Tom,
>>>>>
>>>>>Here are the test suits with tests (too many to list explicitly)
>>>>>that (A) do not close em, and/or (B) do not rethrow the exception
>>>>>in the 'catch (...Exception ...)' block:
>>>>>
>>>>>EntityManagerJUnitTestSuite - most tests have (A) and some (B)
>>>>>ExpressionJUnitTestSuite - (B)
>>>>>EntityMappingsInheritedJUnitTestCase - some (A), some (B)
>>>>>EntityMappingsMergeInheritedJUnitTestCase - some (A), some (B)
>>>>>JUnitEJBQLValidationTestSuite - this one closes em only if
>>>>>rolled back (?)
>>>>>
>>>>>There are of course others that do not close em at the end of
>>>>>the test.
>>>>>
>>>>>Attached are the changes for the issue itself. All but the last
>>>>>file
>>>>>listed above are changed for the issue anyway, so let me know,
>>>>>if you
>>>>>are ok to add 'em.close(); throw ex;' to all missing cases.
>>>>>
>>>>>thanks,
>>>>>-marina
>>>>>
>>>>>Tom Ware wrote On 11/01/06 10:42,:
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>>Hi Marina,
>>>>>>
>>>>>>Answers inline:
>>>>>>
>>>>>>Marina Vatkina wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>>Tom, Team,
>>>>>>>
>>>>>>>I'm in the process of fixing this issue (https://
>>>>>>>glassfish.dev.java.net/issues/show_bug.cgi?id=1226),
>>>>>>>and have several related questions:
>>>>>>>
>>>>>>>1. In some tests the 'catch (...Exception e)' results in
>>>>>>>em.close() calls, while in other it doesn't. Should I add
>>>>>>>em.close() in at least those cases that I change? Or should I
>>>>>>>try to fix all of them?
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>In general, I think it is ok to add em.close() in most cases we
>>>>>>catch an exception. I, however, think you should do decide on
>>>>>>a case by case basis whether it is appropriate to the test you
>>>>>>are modifying.
>>>>>>It is probably a good idea to eventually ensure all
>>>>>>EntityManager's close when we are done using them in the
>>>>>>testing, but that may be a bigger task then just fixing them
>>>>>>where exceptions are thrown. Unless you are seeing some errors
>>>>>>as a result of these EMs not closing, I suggest simply filing
>>>>>>an Enhancement.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>>2. There are cases, when the exception is completely ignored
>>>>>>>(i.e. no 'fail' or
>>>>>>>anything to that extent being done inside the catch block). Is
>>>>>>>it OK to ignore
>>>>>>>those exceptions?
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>I think this is another issue that you need to decide on a case
>>>>>>by case basis. I'd be happy to comment on any specific
>>>>>>examples you have questions about.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>>3. This is a more generic question: let's come with the rules
>>>>>>>that we can
>>>>>>>document in the readme file, to make sure we don't need to do
>>>>>>>this exercise
>>>>>>>again :(.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>I think a published set of rules for the tests is a great idea
>>>>>>- although maybe not necessarily simple to put together in a
>>>>>>way that keeps everyone in the community happy due to coding
>>>>>>styles and such. In general, however, I think this is one of
>>>>>>several pieces of collateral that we need to provide to ease
>>>>>>the process of contributing to the project. Do you know of any
>>>>>>examples of such standards currently available to the glassfish
>>>>>>community. Ideally, we could use an already published one as a
>>>>>>basis for the one we eventually settle on.
>>>>>>
>>>>>>-Tom
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>>thanks,
>>>>>>>-marina
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>--
>>Tom Ware
>>Principal Software Engineer
>>Oracle Canada Inc.
>>
>>Direct: (613) 783-4598
>>Email: tom.ware_at_oracle.com
>>
>>
>
>Craig Russell
>Architect, Sun Java Enterprise System http://java.sun.com/products/jdo
>408 276-5638 mailto:Craig.Russell_at_sun.com
>P.S. A good JDO? O, Gasp!
>
>
>