Solr
  1. Solr
  2. SOLR-3430

DIH: More-realistic testing & subsequent bug fix

    Details

      Description

      A couple of people have mentioned on the users' list that 3.6 changes break CachedSqlEntityProcessor. Part of the problem is the current DIH tests are not realistic: they mostly test 1 class at a time and not the full end-to-end interactions between components.

      This is a test of (Cached) SqlEntityProcessor using an in-memory Derby database. This hopefully is a first step in creating a series of new, more-rigorous DIH tests, which is needed in order to make changes without worry.

      This also revealed a bug in SortedMapBackedCache, which is fixed also. I'm not sure this is the same bug causing problems for the 2 users but once committed I will post a reply on the users' list to see if they can try it.

      1. SOLR-3430.patch
        33 kB
        James Dyer
      2. SOLR-3430.patch
        34 kB
        James Dyer
      3. SOLR-3430.patch
        35 kB
        James Dyer
      4. SOLR-3430.patch
        36 kB
        James Dyer
      5. SOLR-3430_build.patch
        3 kB
        Robert Muir
      6. derby-NOTICE.txt
        7 kB
        James Dyer
      7. derby-LICENSE-ASL.txt
        11 kB
        James Dyer
      8. derby-10.8.2.2.jar.sha1
        0.0 kB
        James Dyer
      9. derby-10.8.2.2.jar
        2.55 MB
        James Dyer

        Activity

        Hide
        Uwe Schindler added a comment -

        Bulk close for 3.6.1

        Show
        Uwe Schindler added a comment - Bulk close for 3.6.1
        Hide
        James Dyer added a comment -

        Committed.

        Trunk: r1335140 & r1335196 (rmuir's build changes)
        Branch_36: r1335176 & r1335205 (rmuir's build changes)

        Robert, thanks for the help with ant.

        Show
        James Dyer added a comment - Committed. Trunk: r1335140 & r1335196 (rmuir's build changes) Branch_36: r1335176 & r1335205 (rmuir's build changes) Robert, thanks for the help with ant.
        Hide
        Robert Muir added a comment -

        James thanks for adding these tests!

        Attached (SOLR-3430_build.patch) is a tiny tweak to the build: currently DIH tests can fail if 'ant example' has not previously been run (because the hsqldb.jar might not be resolved).

        So this patch ensures the hsqldb.jar is always resolved.

        Show
        Robert Muir added a comment - James thanks for adding these tests! Attached ( SOLR-3430 _build.patch) is a tiny tweak to the build: currently DIH tests can fail if 'ant example' has not previously been run (because the hsqldb.jar might not be resolved). So this patch ensures the hsqldb.jar is always resolved.
        Hide
        James Dyer added a comment -

        I added a check to the # of queries being issued to the datasource and found an additional bug. This is likely the bug users are reporting on the mailing list.

        I will commit now to Trunk & 3.6.1 .

        Show
        James Dyer added a comment - I added a check to the # of queries being issued to the datasource and found an additional bug. This is likely the bug users are reporting on the mailing list. I will commit now to Trunk & 3.6.1 .
        Hide
        James Dyer added a comment -

        this update also contains a fix in "testJdbcDataSource", which was not cleaning up a registered datasource causing the new test to fail.

        Show
        James Dyer added a comment - this update also contains a fix in "testJdbcDataSource", which was not cleaning up a registered datasource causing the new test to fail.
        Hide
        James Dyer added a comment -

        Mikhail,

        So far this is a new test and I'm not removing any old tests (yet). Eventually I'd like to replace all of the (cached)SqlEntityProcessor tests with one like this. The point here is to make the tests more realistic, make the tests easier to read and understand and to eliminate the need to pollute the class api for the sake of the tests (just scan the code for comments like "this constructor is for tests only" and for null-checks that are only needed to make tests run, etc).

        I think the bug found in SOLR-3360 had to do with the multi-threaded feature that only exists in 3.x and was removed from Trunk. Whenever we get to a point where we can reintroduce multi-threading to DIH we'll need to be sure all the various scenarios get tested.

        In the end if our tests mimic closely what an actual user is doing, that is, run DIH against realistic datasources using actual DIH configurations, etc, I think we'll have more realistic testing and we can catch the real bugs that are actually affecting the usability of the product. You had a good point in the past about some unrealistic features in MockDataSource. Personally I never liked using that class to write tests because I found it cumbersome. I think its going to be more "enjoyable" to write tests using a real database.

        Show
        James Dyer added a comment - Mikhail, So far this is a new test and I'm not removing any old tests (yet). Eventually I'd like to replace all of the (cached)SqlEntityProcessor tests with one like this. The point here is to make the tests more realistic, make the tests easier to read and understand and to eliminate the need to pollute the class api for the sake of the tests (just scan the code for comments like "this constructor is for tests only" and for null-checks that are only needed to make tests run, etc). I think the bug found in SOLR-3360 had to do with the multi-threaded feature that only exists in 3.x and was removed from Trunk. Whenever we get to a point where we can reintroduce multi-threading to DIH we'll need to be sure all the various scenarios get tested. In the end if our tests mimic closely what an actual user is doing, that is, run DIH against realistic datasources using actual DIH configurations, etc, I think we'll have more realistic testing and we can catch the real bugs that are actually affecting the usability of the product. You had a good point in the past about some unrealistic features in MockDataSource. Personally I never liked using that class to write tests because I found it cumbersome. I think its going to be more "enjoyable" to write tests using a real database.
        Hide
        Mikhail Khludnev added a comment -

        James,

        I agree that DB is better than Mock. I'd like to confirm that this approach covers query repetitions issues too. In SOLR-3360 we had a bug when the same query runs many times. Blind automated tests with in-memory DB won't cover such bugs - it will just run same sql 10 times w/o any problems. Will we have a kind of utility wrapper prevents same query run again and again?

        Regards

        Show
        Mikhail Khludnev added a comment - James, I agree that DB is better than Mock. I'd like to confirm that this approach covers query repetitions issues too. In SOLR-3360 we had a bug when the same query runs many times. Blind automated tests with in-memory DB won't cover such bugs - it will just run same sql 10 times w/o any problems. Will we have a kind of utility wrapper prevents same query run again and again? Regards
        Hide
        James Dyer added a comment -

        Here's a version that uses hsqldb instead of derby and modify's "solr/common-build.xml" to include this in the testing classpath.

        I would like to commit this one in a few days, and backport to the 3.6.1 as it includes a bugfix for dih caching.

        Show
        James Dyer added a comment - Here's a version that uses hsqldb instead of derby and modify's "solr/common-build.xml" to include this in the testing classpath. I would like to commit this one in a few days, and backport to the 3.6.1 as it includes a bugfix for dih caching.

          People

          • Assignee:
            James Dyer
            Reporter:
            James Dyer
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development