Hi Tom,
Attached is the version with added missing 'throw <x>' statements.
This change required changing Throwable and Exception to RuntimeException
in EntityManagerJUnitTestSuite (there is no obvious reason to catch more
than that for EM api calls), and one typo where the outer exception was
thrown (it is a typo, as the outer exception is the expected one).
Please review. All e-p-t tests passed on Oracle with these changes.
thanks,
-marina
Tom Ware wrote:
> 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!
>>
>>
>>