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