Solr
  1. Solr
  2. SOLR-2367

DataImportHandler unit tests are very noisy

    Details

      Description

      Running DataImportHandler unit tests emits a lot of console noise, mainly stacktraces because dataimport.properties can't be written. This makes it hard to scan the output for useful information.

      I'm attaching a patch to get rid of most of the noise by creating the conf directory before test runs so that the properties file write doesn't fail.

      1. SOLR-2367.patch
        3 kB
        Gunnlaugur Thor Briem
      2. SOLR-2367.patch
        2 kB
        Robert Muir
      3. SOLR-2367-extend-SolrException.patch
        2 kB
        Gunnlaugur Thor Briem
      4. SOLR-2367-log-exceptions-through-SolrException.patch
        6 kB
        Gunnlaugur Thor Briem

        Activity

        Hide
        Gunnlaugur Thor Briem added a comment - - edited

        Patch to address this issue. Creates conf directories under work directory before test runs, and suppresses a warning.

        The console noise that remains is some XML parsing failure, which may or may not be meaningful (I don't know) — at least now it is visible.

        This patch is against branch_3x as of just now.

        Show
        Gunnlaugur Thor Briem added a comment - - edited Patch to address this issue. Creates conf directories under work directory before test runs, and suppresses a warning. The console noise that remains is some XML parsing failure, which may or may not be meaningful (I don't know) — at least now it is visible. This patch is against branch_3x as of just now.
        Hide
        Robert Muir added a comment -

        Thanks for the patch. I modified it, to just specify the absolute path to these directories.

        this way we don't have to make any useless directories underneath the CWD.

        Separately, as far as the exceptions, this is in the test TestErrorHandling, its 'expected exceptions'. I tried to modify this test to use the 'expected exception' logic in SolrTestCaseJ4, etc, but I could not make it work.

        I think this is because DIH throws DataImportHandlerExceptions (extends RuntimeException) instead of ones that extend SolrException?

        Show
        Robert Muir added a comment - Thanks for the patch. I modified it, to just specify the absolute path to these directories. this way we don't have to make any useless directories underneath the CWD. Separately, as far as the exceptions, this is in the test TestErrorHandling, its 'expected exceptions'. I tried to modify this test to use the 'expected exception' logic in SolrTestCaseJ4, etc, but I could not make it work. I think this is because DIH throws DataImportHandlerExceptions (extends RuntimeException) instead of ones that extend SolrException?
        Hide
        Gunnlaugur Thor Briem added a comment -

        Oh, right, much neater.

        Show
        Gunnlaugur Thor Briem added a comment - Oh, right, much neater.
        Hide
        Gunnlaugur Thor Briem added a comment -

        If it helps, here's a patch that makes DataImportHandlerException extend SolrException (and deprecates a constructor that seems not to be used anywhere). All tests pass, but beyond that this has not been tried out at runtime (and maybe the change isn't even appropriate?) ... does this make the exception silencing work?

        Show
        Gunnlaugur Thor Briem added a comment - If it helps, here's a patch that makes DataImportHandlerException extend SolrException (and deprecates a constructor that seems not to be used anywhere). All tests pass, but beyond that this has not been tried out at runtime (and maybe the change isn't even appropriate?) ... does this make the exception silencing work?
        Hide
        Robert Muir added a comment -

        Thanks for the followup patch, I will try and see if i can use the exception ignores mechanism now with it... maybe this time it will work.

        Show
        Robert Muir added a comment - Thanks for the followup patch, I will try and see if i can use the exception ignores mechanism now with it... maybe this time it will work.
        Hide
        Robert Muir added a comment -

        I tried to use your patch and silence the tests in various ways... I was unsuccessful.

        Its a mystery really to me (because I don't understand the code that well) that all
        these exceptions are being thrown and nothing is failing... so I'm not sure how to silence them.

        Lets commit the first patch and fix 80% of the problem... maybe we can figure out the other exceptions in the future. I'll keep the issue open.

        Show
        Robert Muir added a comment - I tried to use your patch and silence the tests in various ways... I was unsuccessful. Its a mystery really to me (because I don't understand the code that well) that all these exceptions are being thrown and nothing is failing... so I'm not sure how to silence them. Lets commit the first patch and fix 80% of the problem... maybe we can figure out the other exceptions in the future. I'll keep the issue open.
        Hide
        Gunnlaugur Thor Briem added a comment -

        Here goes the remaining 20% — I'm attaching SOLR-2367-log-exceptions-through-SolrException.patch which makes DataImportHandler log exceptions through SolrException.log() instead of directly into the logger. This way the exception-ignoring mechanism gets a say in matters. Test output is nice and clean now. I addressed only those logger calls that were emitting exceptions in unit test runs.

        Note: this does not require DataImportHandlerException to extend SolrException, so the earlier SOLR-2367-extend-SolrException.patch is not needed. (Might still be worthwhile, I don't know — but not needed for this fix).

        Show
        Gunnlaugur Thor Briem added a comment - Here goes the remaining 20% — I'm attaching SOLR-2367 -log-exceptions-through-SolrException.patch which makes DataImportHandler log exceptions through SolrException.log() instead of directly into the logger. This way the exception-ignoring mechanism gets a say in matters. Test output is nice and clean now. I addressed only those logger calls that were emitting exceptions in unit test runs. Note: this does not require DataImportHandlerException to extend SolrException , so the earlier SOLR-2367 -extend-SolrException.patch is not needed. (Might still be worthwhile, I don't know — but not needed for this fix).
        Hide
        Robert Muir added a comment -

        Thanks Gunnlaugur for getting to the bottom of this!

        Show
        Robert Muir added a comment - Thanks Gunnlaugur for getting to the bottom of this!
        Hide
        Grant Ingersoll added a comment -

        Bulk close for 3.1.0 release

        Show
        Grant Ingersoll added a comment - Bulk close for 3.1.0 release

          People

          • Assignee:
            Robert Muir
            Reporter:
            Gunnlaugur Thor Briem
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - 5m
              5m
              Remaining:
              Remaining Estimate - 5m
              5m
              Logged:
              Time Spent - Not Specified
              Not Specified

                Development