Skip to main content
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [List Home]
Re: [eclipselink-dev] rewriting updateall queries clean-up code in tests 
Hi Guys,

  A few comments inline:

Andrei Ilitchev wrote:

Hi Dies,

Two questions about the suggested patch.

1. Sequencing.
My understanding that Symfoware platform doesn't allow SEQUENCE to be used as a table name, but rather than changing all the tests that use AUTO and TABLE sequences (and that work on all platforms other than Symfoware) please consider re-defining default sequence table name by defining sequence generators with magic names: "SEQ_GEN" and "SEQ_GEN_TABLE". 

The sequence defined by SEQ_GEN will be used for AUTO;
the sequence defined in SEQ_GEN_TABLE will be used for table sequence in cases like: @GeneratedValue(strategy=GenerationType.TABLE, generator="CMP3_AAA_GENERATOR") 
when no CMP3_AAA_GENERATOR is defined.
In general, I agree with Andrei that we shouldn't stop testing table sequences with the name SEQUENCE for all platforms - it has been our default for a long time so most users will have it. The issue here will be making changes that fit into our test framework.
In talking with Andrei, I think a better solution may be to implement a customizer that loops through the sequences in a project and changes any table sequence with the name "SEQUENCE" to something else. That customizer could be enabled with a System property when running the tests on Symfoware. 



2. UpdateAll/DeleteAll
Please consider instead of removing all usages of updateAll/deleteAll queries in the tests clean up code adding a flag supportsModifyAllQueries (or smth like that) to the platform 
(or alternatively adding a persistence unit property?)
and keeping the original updateAll/deleteAll unless the platform doesn't support them
I am not a fan of adding a method to DatabasePlatform that will just be used for testing.
I can see the argument that we should not alter our testing for one platform, and that there may be some testing that occurs as a side-effect of these clean-up methods. When I, however, look at the code we are changing, nearly all of them exercise a DeleteAll query on a Basic field on Employee. In my opinion, we have exercised that particular query adequately in the DeleteAll-specific tests and the changes Dies has provided should be fine. There are a small number these changes I have questions about, but I will be able to provide more detail when I investigate integrating the final patch.
My vote is that we keep the majority of Dies' updates - but I am open to arguments that go the other way. 

<snip>

I also noticed such queries in some other tests and wasn't sure
whether/how to fix them. What do you think?

*
jpa/eclipselink.jpa.test/src/org/eclipse/persistence/testing/tests/jpa/advanced/ReportQueryAdvancedJUnitTest.java 

The tests here don't seem to use UpdateAll/DeleteAll queries but the
clear method does. It uses UnitOfWork instead of EntityManager. Where
can I find the UnitOfWork API equivalent to doing a find and removing
each object in the resultset?
I am a hesitant to change this test for all platforms, but you could add an isSymfoware() check and then do some alternate clean up. Which queries fail? (I assume just the ones that deal with Employee) 

Here is some pseudo code:

Iterator<Employee> emps = uow.readAllObjects(Employee.class).iterator()
while (emps.hasNext()){
  Employee emp = emps.next();
  emp.setManager(null);
  emp.setAddress(null);
  uow.deleteObject(emp);
};




*
jpa/eclipselink.jpa.test/src/org/eclipse/persistence/testing/tests/jpa/advanced/JoinedAttributeAdvancedJunitTest.java 

Same as above, UpdateAll/DeleteAll with UOW in clear method. Here I
think the following tests rely on UpdateAll/Delete queries so need to be
excluded for Symfoware, would you agree?

- testTwoUnrelatedResultWithOneToManyJoins
- testMultipleUnrelatedResultWithOneToManyJoins
- testTwoUnrelatedResultWithOneToOneJoins
- testTwoUnrelatedResultWithOneToOneJoinsWithExtraItem


The clear should be able to be fixed in much the same way as above.

I agree those test should be excluded for symfoware.



*
jpa/eclipselink.jpa.test/src/org/eclipse/persistence/testing/tests/jpa/fieldaccess/advanced/JoinedAttributeAdvancedJunitTest.java 

Same as above.

*
jpa/eclipselink.jpa.test/src/org/eclipse/persistence/testing/tests/jpa/jpql/JUnitJPQLComplexTestSuite.java 

complexConditionCaseInUpdateTest

This test fails because it uses a multi-table entity. If I would rewrite
it to use an entity that is mapped to a simple table, it might pass.
Should I try, or should I just skip these type of tests on Symfoware?
Let's just skip this kind of test for Symfoware. Altering the test will reduce our testing of multi-table. 


The patch also includes a number of other issues I addressed. Could you
review them?
When we have come up with a good solution for the issue with SEQUENCE, I'll go through the whole patch and either integrate it (with any small changes needed) or provide feedback about bigger changes. 

-Tom



Thanks,
Dies

_______________________________________________
eclipselink-dev mailing list
eclipselink-dev@xxxxxxxxxxx
https://dev.eclipse.org/mailman/listinfo/eclipselink-dev



_______________________________________________
eclipselink-dev mailing list
eclipselink-dev@xxxxxxxxxxx
https://dev.eclipse.org/mailman/listinfo/eclipselink-dev

Back to the top