Hi Marina,
The changes look good.
-Tom
Marina Vatkina wrote:
>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!
>>>
>>>
>>>
>>>
>>>