Hi Tom,
Tom Ware wrote On 11/01/06 13:33,:
> 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?
No, current changes are only replacing
em.getTransaction().rollback();
with
if (em.getTransaction().isActive()){
em.getTransaction().rollback();
}
inside 'catch (...Exception...)' block (where it's of cause needed).
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
>
> -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
>>>>
>>>>
>>>>
>>>>
>
>