Solr
  1. Solr
  2. SOLR-3344

POM dependencies not all there yet for solr-test-framework

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 4.0-ALPHA
    • Fix Version/s: 4.0-ALPHA
    • Component/s: Build
    • Labels:
      None

      Description

      The pom for solr-test-framework does not mention jetty, so a test case that doesn't get jetty into it's classpath otherwise fails with:

      java.lang.NoClassDefFoundError: org/eclipse/jetty/server/SessionIdManager
      
      1. 0001-Add-jetty-deps.patch
        1 kB
        Benson Margulies
      2. SOLR-3344.patch
        3 kB
        Steve Rowe
      3. SOLR-3344-runtime-scope.patch
        0.8 kB
        Steve Rowe

        Activity

        Hide
        Benson Margulies added a comment -

        Here's the fix.

        Show
        Benson Margulies added a comment - Here's the fix.
        Hide
        Steve Rowe added a comment -

        Benson, where are you seeing these failures? Why hasn't Jenkins unearthed them?

        Why should Solr test-framework declare these dependencies at all? That is, why can't modules with Jetty test dependencies simply declare the dependency there (with scope=test)? This is already the case for the following Solr modules: solr-core, clustering, and dataimporthandler.

        If it does turn out that Solr test-framework is the right place for these dependencies, it doesn't require Jetty to compile, so at a minimum, I would argue that the default compile scope you've used is inappropriate. Maybe scope=runtime?

        Show
        Steve Rowe added a comment - Benson, where are you seeing these failures? Why hasn't Jenkins unearthed them? Why should Solr test-framework declare these dependencies at all? That is, why can't modules with Jetty test dependencies simply declare the dependency there (with scope=test)? This is already the case for the following Solr modules: solr-core, clustering, and dataimporthandler. If it does turn out that Solr test-framework is the right place for these dependencies, it doesn't require Jetty to compile, so at a minimum, I would argue that the default compile scope you've used is inappropriate. Maybe scope=runtime?
        Hide
        Benson Margulies added a comment -

        Here's how this plays out.

        In my very own project, not in the lucene tree anywhere, I wrote a test class that uses the base class for distributed tests. So I put the test-framework in as a <scope>test</scope> dependency.

        It compiles fine, but gets NoClassDefFound for Jetty when I run it. Why? Well, jetty is listed as an optional dependency of solr-core. So it is not transitive.

        I think that it's reasonable for the test framework jar to list it as a required dependency, since tests in there depend on it. However, there is an alternative (other than nothing): list it again as an optional dependency in test-framework, just to give people a hint, and/or change the javadoc for the BaseDistributedTestCase to note this requirement.

        Show
        Benson Margulies added a comment - Here's how this plays out. In my very own project, not in the lucene tree anywhere, I wrote a test class that uses the base class for distributed tests. So I put the test-framework in as a <scope>test</scope> dependency. It compiles fine, but gets NoClassDefFound for Jetty when I run it. Why? Well, jetty is listed as an optional dependency of solr-core. So it is not transitive. I think that it's reasonable for the test framework jar to list it as a required dependency, since tests in there depend on it. However, there is an alternative (other than nothing): list it again as an optional dependency in test-framework, just to give people a hint, and/or change the javadoc for the BaseDistributedTestCase to note this requirement.
        Hide
        Steve Rowe added a comment -

        I think that it's reasonable for the test framework jar to list it as a required dependency, since tests in there depend on it. However, there is an alternative (other than nothing): list it again as an optional dependency in test-framework, just to give people a hint, and/or change the javadoc for the BaseDistributedTestCase to note this requirement.

        I assume you're referring to BaseDistributed*Search*TestCase, which depends on solr-core's oas.client.solrj.embedded.JettySolrRunner?

        I like the optional dependencies alternative better: add optional jetty dependencies to Solr test-framework, and change the javadoc for BaseDistributedSearchTestCase to note this requirement.

        Show
        Steve Rowe added a comment - I think that it's reasonable for the test framework jar to list it as a required dependency, since tests in there depend on it. However, there is an alternative (other than nothing): list it again as an optional dependency in test-framework, just to give people a hint, and/or change the javadoc for the BaseDistributedTestCase to note this requirement. I assume you're referring to BaseDistributed*Search*TestCase , which depends on solr-core's oas.client.solrj.embedded.JettySolrRunner ? I like the optional dependencies alternative better: add optional jetty dependencies to Solr test-framework, and change the javadoc for BaseDistributedSearchTestCase to note this requirement.
        Hide
        Steve Rowe added a comment -

        I like the optional dependencies alternative better: add optional jetty dependencies to Solr test-framework, and change the javadoc for BaseDistributedSearchTestCase to note this requirement.

        Thinking about this more, the rationale I would ordinarily use to justify making a dependency optional involves minimizing runtime dependencies. But the Solr test-framework is a test-only module, and requiring test dependencies that won't be used by all consumers should not cause any undue hardship.

        So I've changed my mind. I'll put up a superset of your patch, Benson, that removes the dataimporthandler and clustering POMs' jetty dependencies. I'll leave solr-core's optional dependency as-is.

        Show
        Steve Rowe added a comment - I like the optional dependencies alternative better: add optional jetty dependencies to Solr test-framework, and change the javadoc for BaseDistributedSearchTestCase to note this requirement. Thinking about this more, the rationale I would ordinarily use to justify making a dependency optional involves minimizing runtime dependencies. But the Solr test-framework is a test-only module, and requiring test dependencies that won't be used by all consumers should not cause any undue hardship. So I've changed my mind. I'll put up a superset of your patch, Benson, that removes the dataimporthandler and clustering POMs' jetty dependencies. I'll leave solr-core's optional dependency as-is.
        Hide
        Benson Margulies added a comment -

        Thank you very much.

        Show
        Benson Margulies added a comment - Thank you very much.
        Hide
        Steve Rowe added a comment -

        Patch, adding 3 required Jetty dependencies to the Solr test-framework POM template, and removing those dependencies from the dataimporthandler and clustering POM templates.

        Tests all pass for me locally.

        Committing shortly.

        Show
        Steve Rowe added a comment - Patch, adding 3 required Jetty dependencies to the Solr test-framework POM template, and removing those dependencies from the dataimporthandler and clustering POM templates. Tests all pass for me locally. Committing shortly.
        Hide
        Steve Rowe added a comment -

        Committed to trunk: r1311905.

        Show
        Steve Rowe added a comment - Committed to trunk: r1311905.
        Hide
        Steve Rowe added a comment -

        If it does turn out that Solr test-framework is the right place for these dependencies, it doesn't require Jetty to compile, so at a minimum, I would argue that the default compile scope you've used is inappropriate. Maybe scope=runtime?

        I forgot to incorporate this change - reopening to change Solr test-framework's Jetty dependencies' scope from compile to runtime.

        Show
        Steve Rowe added a comment - If it does turn out that Solr test-framework is the right place for these dependencies, it doesn't require Jetty to compile, so at a minimum, I would argue that the default compile scope you've used is inappropriate. Maybe scope=runtime? I forgot to incorporate this change - reopening to change Solr test-framework's Jetty dependencies' scope from compile to runtime.
        Hide
        Steve Rowe added a comment -

        Patch switching Solr test-framework's Jetty dependencies' scope from compile to runtime. All Solr tests pass. Committing shortly.

        Show
        Steve Rowe added a comment - Patch switching Solr test-framework's Jetty dependencies' scope from compile to runtime. All Solr tests pass. Committing shortly.
        Hide
        Steve Rowe added a comment -

        Committed compile->runtime scope changes in r1311957.

        Show
        Steve Rowe added a comment - Committed compile->runtime scope changes in r1311957.

          People

          • Assignee:
            Steve Rowe
            Reporter:
            Benson Margulies
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development