Details

    • Type: Sub-task Sub-task
    • Status: Closed Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.2-SNAPSHOT
    • Fix Version/s: 1.3 rc1
    • Component/s: JDO tests
    • Labels:
      None
    • Number of attachments :
      9
    1. patch20080505-test30-03.txt
      70 kB
      Udai Gupta
    2. patch20080505-test30-test31.txt
      139 kB
      Udai Gupta
    3. patch20080506_test30_test31-01.diff
      72 kB
      Udai Gupta
    4. patch20080507-C2377-running.diff
      74 kB
      Udai Gupta
    5. patch-C2377-20080506-02.txt
      68 kB
      Ralf Joachim
    6. patch-C2377-20080512.txt
      73 kB
      Ralf Joachim
    7. patch-TC30-20080505.txt
      123 kB
      Udai Gupta
    8. patch-test30-20080505-01.txt
      72 kB
      Udai Gupta
    9. patch-test30-20080505-02.txt
      72 kB
      Udai Gupta

      Issue Links

        Activity

        Hide
        Udai Gupta added a comment -

        I have attached the patch for the Refactorization TC30 into test30. Here I have created a separate test method for each query.

        Show
        Udai Gupta added a comment - I have attached the patch for the Refactorization TC30 into test30. Here I have created a separate test method for each query.
        Hide
        Werner Guttmann added a comment -

        Udai, I don't think there's any value in overloading the suite() method in this test case. JUnit 3.x has a default convention where any method will be added to the suite if it has a prefix of 'test'. As we have followed that convention throughout, please drop this method.

        Show
        Werner Guttmann added a comment - Udai, I don't think there's any value in overloading the suite() method in this test case. JUnit 3.x has a default convention where any method will be added to the suite if it has a prefix of 'test'. As we have followed that convention throughout, please drop this method.
        Hide
        Werner Guttmann added a comment -

        A few more observations:

        a) Using code statements in setUp() like this

        LOG.info("Test30 Started");

        could be slightly mis-leading, as the methods setUp() and tearDown() will be executed before and after each test case (as opposed to once per test class).

        b) the populateDatabase*() methods should imho not be included in the test suite (if we decide to keep a suite method, which in my opinion is completely redundant).

        c) Shouldn't the populateDatabase*() methods be included in the setUp() method, so that each test has a clean database before it is being executed ?

        Show
        Werner Guttmann added a comment - A few more observations: a) Using code statements in setUp() like this LOG.info( "Test30 Started" ); could be slightly mis-leading, as the methods setUp() and tearDown() will be executed before and after each test case (as opposed to once per test class). b) the populateDatabase*() methods should imho not be included in the test suite (if we decide to keep a suite method, which in my opinion is completely redundant). c) Shouldn't the populateDatabase*() methods be included in the setUp() method, so that each test has a clean database before it is being executed ?
        Hide
        Ralf Joachim added a comment -

        I hope to be able to review your patch in detail taking Werner's comments into account later on today. ATM I only recognized that some changes are contained twice in the patch. In addition you seam to have also moved entities not used by TC30 but other TC3x tests.

        Show
        Ralf Joachim added a comment - I hope to be able to review your patch in detail taking Werner's comments into account later on today. ATM I only recognized that some changes are contained twice in the patch. In addition you seam to have also moved entities not used by TC30 but other TC3x tests.
        Hide
        Udai Gupta added a comment -

        Thanks for your comments ....I have modified accordingly ....I hope this time it will be fine.

        Show
        Udai Gupta added a comment - Thanks for your comments ....I have modified accordingly ....I hope this time it will be fine.
        Hide
        Werner Guttmann added a comment -

        Yes, much better. But there was no need to actually remove the populateDatabase*() methods and move the code into the setUp() method. Just leave those methods as is, and simply call them from the setUp() method.

        Show
        Werner Guttmann added a comment - Yes, much better. But there was no need to actually remove the populateDatabase*() methods and move the code into the setUp() method. Just leave those methods as is, and simply call them from the setUp() method.
        Hide
        Udai Gupta added a comment -

        done!!

        Show
        Udai Gupta added a comment - done!!
        Hide
        Werner Guttmann added a comment -

        Great. Please leave the call to

        _db = getJDOManager(DBNAME).getDatabase();
        

        in the setUp()method, though (as that's where it belongs to ...) .. .

        Show
        Werner Guttmann added a comment - Great. Please leave the call to _db = getJDOManager(DBNAME).getDatabase(); in the setUp()method, though (as that's where it belongs to ...) .. .
        Hide
        Ralf Joachim added a comment -

        Updated patch with a bunch of small improvements to test30. According to the tests i extracted 2 tests of test30 into a seperate test class but most was about code formating. Having said that I had not enough time to test them.

        Udai, could you please test my changes and review test31 yourself with respect to the changes I made to test30.

        Show
        Ralf Joachim added a comment - Updated patch with a bunch of small improvements to test30. According to the tests i extracted 2 tests of test30 into a seperate test class but most was about code formating. Having said that I had not enough time to test them. Udai, could you please test my changes and review test31 yourself with respect to the changes I made to test30.
        Hide
        Udai Gupta added a comment -

        Thanks Ralf , I will take care of all this next time when I will code.....There was some minor problems in the patch of tests you provided in context of running them....I fixed those ...now its perfect....I will modify the TC31 accordingly

        Show
        Udai Gupta added a comment - Thanks Ralf , I will take care of all this next time when I will code.....There was some minor problems in the patch of tests you provided in context of running them....I fixed those ...now its perfect....I will modify the TC31 accordingly
        Hide
        Ralf Joachim added a comment -

        Again removed test30_call table from ddl script and also had to do some code formatting at test31. Of what I recall there where class variables that where only needed by one method and did not conform to our naming conventions for class variables and a lot of lines with wrong indention. Also one of the table names did not start with test31. In addition some of the tables got not reset before test execution.

        I have fixed all this myself and added release notes so that the patch now is ready to commit.

        Having said that all tests already run without failures against mysql.

        Show
        Ralf Joachim added a comment - Again removed test30_call table from ddl script and also had to do some code formatting at test31. Of what I recall there where class variables that where only needed by one method and did not conform to our naming conventions for class variables and a lot of lines with wrong indention. Also one of the table names did not start with test31. In addition some of the tables got not reset before test execution. I have fixed all this myself and added release notes so that the patch now is ready to commit. Having said that all tests already run without failures against mysql.

          People

          • Assignee:
            Ralf Joachim
            Reporter:
            Udai Gupta
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: