Derby
  1. Derby
  2. DERBY-5764

Make DatabaseMetaDataTest more robust wrt changes made by other tests

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 10.9.1.0
    • Fix Version/s: 10.10.1.1
    • Component/s: Test
    • Labels:
      None

      Description

      The subset of tests from DatabaseMetaDataTest being run as part of the upgrade tests is sensitive to changes in the database made by other tests. For instance, adding tables with foreign keys will make the test fail due to extra rows in system tables.

      Usually this could be solved by using a single-use db wrapper of some sort, but in the upgrade tests the database will be booted several times with different versions of Derby and the data needs to be preserved between some of these boots.

      1. derby-5764-1a-upgraderun_cleanup.diff
        2 kB
        Kristian Waagan
      2. derby-5764-2a-specify_schema.diff
        46 kB
        Kristian Waagan
      3. derby-5764-3a-add_test_case_schema_null.diff
        5 kB
        Kristian Waagan
      4. derby-5764-3b-add_test_case_schema_null.diff
        5 kB
        Kristian Waagan

        Activity

        Hide
        Kristian Waagan added a comment -

        Attached some trivial cleanups to UpgradeRun in patch 1a.
        Committed to trunk with revision 1339007.

        Show
        Kristian Waagan added a comment - Attached some trivial cleanups to UpgradeRun in patch 1a. Committed to trunk with revision 1339007.
        Hide
        Kristian Waagan added a comment -

        Attaching patch 2a, which causes the user name to be used as the schema name.

        Initially I had the schema name as a static variable, but that caused some extra complexity for no gain since it must be possible to use two different schema names during a run of suites.All (running the test individually, and as part of the upgrade tests). I found that the schema name only has to stay constant for the duration of a fixture.

        Running the full regression suite for good measure.
        Patch ready for review.

        As mentioned in the thread on derby-dev, the next step would be to add some extra tests to compensate for the coverage lost by always specifying a schema in the queries.

        Show
        Kristian Waagan added a comment - Attaching patch 2a, which causes the user name to be used as the schema name. Initially I had the schema name as a static variable, but that caused some extra complexity for no gain since it must be possible to use two different schema names during a run of suites.All (running the test individually, and as part of the upgrade tests). I found that the schema name only has to stay constant for the duration of a fixture. Running the full regression suite for good measure. Patch ready for review. As mentioned in the thread on derby-dev, the next step would be to add some extra tests to compensate for the coverage lost by always specifying a schema in the queries.
        Hide
        Kristian Waagan added a comment -

        suites.All passed.

        Show
        Kristian Waagan added a comment - suites.All passed.
        Hide
        Knut Anders Hatlen added a comment -

        The 2a patch looks good to me. +1

        Show
        Knut Anders Hatlen added a comment - The 2a patch looks good to me. +1
        Hide
        Kristian Waagan added a comment -

        Thanks, Knut Anders.

        Committed patch 2a to trunk with revision 1339240.
        I hope to post the remaining patch soon.

        Show
        Kristian Waagan added a comment - Thanks, Knut Anders. Committed patch 2a to trunk with revision 1339240. I hope to post the remaining patch soon.
        Hide
        Kristian Waagan added a comment -

        Patch 3a (attached) adds, or reverts, some test cases using null for the schema in the meta data calls.
        I factored out some code from JDBC.assertUnsortedResultSet into a helper method and added the new method assertResultSetContains. The latter method asserts that the expected rows are in the result set, but it doesn't care if there are more rows than expected.

        I ran the test individually and I also ran the upgrade suite without failures.

        Patch ready for review.

        Show
        Kristian Waagan added a comment - Patch 3a (attached) adds, or reverts, some test cases using null for the schema in the meta data calls. I factored out some code from JDBC.assertUnsortedResultSet into a helper method and added the new method assertResultSetContains. The latter method asserts that the expected rows are in the result set, but it doesn't care if there are more rows than expected. I ran the test individually and I also ran the upgrade suite without failures. Patch ready for review.
        Hide
        Dag H. Wanvik added a comment -

        Looks good to me. If the last Assert in assertRSContains necessary? ("Assert.assertTrue("Extra rows in ResultSet", actual.isEmpty())").
        You already tests same cardinality and "contains"..
        +1

        Show
        Dag H. Wanvik added a comment - Looks good to me. If the last Assert in assertRSContains necessary? ("Assert.assertTrue("Extra rows in ResultSet", actual.isEmpty())"). You already tests same cardinality and "contains".. +1
        Hide
        Kristian Waagan added a comment -

        No, one of the checks can go away.
        When modifying the code in for patch 3a I considered printing missing or extra rows, but ended up not doing it. I've removed the last check in patch 3b.

        Show
        Kristian Waagan added a comment - No, one of the checks can go away. When modifying the code in for patch 3a I considered printing missing or extra rows, but ended up not doing it. I've removed the last check in patch 3b.
        Hide
        Kristian Waagan added a comment -

        Committed patch 3b to trunk with revision 1351212.
        Resolving issue.

        Thanks, for the review, Dag.

        Show
        Kristian Waagan added a comment - Committed patch 3b to trunk with revision 1351212. Resolving issue. Thanks, for the review, Dag.
        Hide
        Knut Anders Hatlen added a comment -

        [bulk update: close all resolved issues that haven't had any activity the last year]

        Show
        Knut Anders Hatlen added a comment - [bulk update: close all resolved issues that haven't had any activity the last year]

          People

          • Assignee:
            Kristian Waagan
            Reporter:
            Kristian Waagan
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development