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!
- application/pkcs7-signature attachment: smime.p7s