Hi Marina,
I think your changes look good.
I am not clear about the last line of your email:
"so let me know, if you are ok to add 'em.close(); throw ex;' to all
missing cases."
Are you referring to the changes you have included or some other set
of changes?
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.
-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