Lucene - Core
  1. Lucene - Core
  2. LUCENE-486

Core Test should not have dependencies on the Demo code

    Details

    • Type: Test Test
    • Status: Closed
    • Priority: Trivial Trivial
    • Resolution: Fixed
    • Affects Version/s: 1.4
    • Fix Version/s: 3.0
    • Component/s: general/build
    • Labels:
      None

      Description

      The TestDoc.java Test file has a dependency on the Demo FileDocument code. Some of us don't keep the Demo code around after downloading, so this breaks the build.

      Patch will be along shortly

      1. FileDocument.java
        2 kB
        Grant Ingersoll
      2. testdoc.patch
        2 kB
        Grant Ingersoll
      3. lucene-486.patch
        4 kB
        Michael Busch

        Activity

        Transition Time In Source Status Execution Times Last Executer Last Execution Date
        Open Open Closed Closed
        649d 14h 1 Grant Ingersoll 22/Oct/07 21:15
        Closed Closed Reopened Reopened
        701d 16h 41m 1 Michael Busch 23/Sep/09 13:57
        Reopened Reopened Resolved Resolved
        12d 17h 9m 1 Michael Busch 06/Oct/09 07:07
        Resolved Resolved Closed Closed
        50d 10h 40m 1 Uwe Schindler 25/Nov/09 16:47
        Mark Thomas made changes -
        Workflow Default workflow, editable Closed status [ 12563359 ] jira [ 12584505 ]
        Mark Thomas made changes -
        Workflow jira [ 12345570 ] Default workflow, editable Closed status [ 12563359 ]
        Uwe Schindler made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Michael Busch made changes -
        Status Reopened [ 4 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Hide
        Michael Busch added a comment -

        Committed revision 822139.

        Show
        Michael Busch added a comment - Committed revision 822139.
        Michael Busch made changes -
        Attachment lucene-486.patch [ 12421356 ]
        Hide
        Michael Busch added a comment -

        Will have to commit the change in TestDoc also to the compatibility tests branch.

        Show
        Michael Busch added a comment - Will have to commit the change in TestDoc also to the compatibility tests branch.
        Michael Busch made changes -
        Fix Version/s 3.0 [ 12312889 ]
        Priority Minor [ 4 ] Trivial [ 5 ]
        Component/s Build [ 12311546 ]
        Michael Busch made changes -
        Resolution Won't Fix [ 2 ]
        Status Closed [ 6 ] Reopened [ 4 ]
        Assignee Michael Busch [ michaelbusch ]
        Hide
        Grant Ingersoll added a comment -

        +1

        Show
        Grant Ingersoll added a comment - +1
        Hide
        Michael Busch added a comment -

        I still think we should remove the dependency that test has on demo. Looking into the code you can replace the call of FileDocument in TestDoc with just 2 lines:

        Document doc = new Document();
        doc.add(new Field("contents", new FileReader(file)));
        

        With this change alone we could remove the dependency. I think we should do that because it makes the build cleaner and I don't think we'll pay less attention to the demo code in the future just because we remove this classpath dependency. Any objections? Otherwise I'll reopen this issue and attach a patch.

        Show
        Michael Busch added a comment - I still think we should remove the dependency that test has on demo. Looking into the code you can replace the call of FileDocument in TestDoc with just 2 lines: Document doc = new Document(); doc.add( new Field( "contents" , new FileReader(file))); With this change alone we could remove the dependency. I think we should do that because it makes the build cleaner and I don't think we'll pay less attention to the demo code in the future just because we remove this classpath dependency. Any objections? Otherwise I'll reopen this issue and attach a patch.
        Grant Ingersoll made changes -
        Status Open [ 1 ] Closed [ 6 ]
        Resolution Won't Fix [ 2 ]
        Hide
        Grant Ingersoll added a comment -

        >The more stuff that's regularly compiled & tested, the better.

        Couldn't agree more.

        >We don't want the demo or contrib code to become neglected. Removing dependencies from the
        >core is important. But I don't see that as a priority for test code.

        Hence, me giving this bug a priority of Minor

        Show
        Grant Ingersoll added a comment - >The more stuff that's regularly compiled & tested, the better. Couldn't agree more. >We don't want the demo or contrib code to become neglected. Removing dependencies from the >core is important. But I don't see that as a priority for test code. Hence, me giving this bug a priority of Minor
        Hide
        Doug Cutting added a comment -

        The more stuff that's regularly compiled & tested, the better. We don't want the demo or contrib code to become neglected. Removing dependencies from the core is important. But I don't see that as a priority for test code.

        My primary objection to making tests dependent on contrib would be that it might substantially slow the compile & test loop, to the degree that folks would run tests less frequently. The demo is small enough that I don't think this is a concern.

        Show
        Doug Cutting added a comment - The more stuff that's regularly compiled & tested, the better. We don't want the demo or contrib code to become neglected. Removing dependencies from the core is important. But I don't see that as a priority for test code. My primary objection to making tests dependent on contrib would be that it might substantially slow the compile & test loop, to the degree that folks would run tests less frequently. The demo is small enough that I don't think this is a concern.
        Hide
        Grant Ingersoll added a comment -

        Doug,

        The intellij thing is just a symptom. I think the bigger issues are the consequences of the demo being changed and that it just doesn't feel right.

        In my mind, the Test code is for testing the core, not the demo. The demo, in my mind is a sample application using the core and has nothing to do with testing the core. To me the demo feels more akin to the contrib area then the core area and I think you would agree that the core tests should not be depend on anything in contrib, right?

        At any rate, I am not trying to create an aesthetics war here, so at this point I will leave it to the committers to decide.

        Show
        Grant Ingersoll added a comment - Doug, The intellij thing is just a symptom. I think the bigger issues are the consequences of the demo being changed and that it just doesn't feel right. In my mind, the Test code is for testing the core, not the demo. The demo, in my mind is a sample application using the core and has nothing to do with testing the core. To me the demo feels more akin to the contrib area then the core area and I think you would agree that the core tests should not be depend on anything in contrib, right? At any rate, I am not trying to create an aesthetics war here, so at this point I will leave it to the committers to decide.
        Hide
        Doug Cutting added a comment -

        Grant, the test code is not in the "main" source tree, it's in the "test" source tree, which depends on "core" and "demo". I still don't see what harm that dependency causes, aesthetically or otherwise. Perhaps such dependencies are awkward in intellij? Is that the issue?

        Show
        Doug Cutting added a comment - Grant, the test code is not in the "main" source tree, it's in the "test" source tree, which depends on "core" and "demo". I still don't see what harm that dependency causes, aesthetically or otherwise. Perhaps such dependencies are awkward in intellij? Is that the issue?
        Hide
        Erik Hatcher added a comment -

        I concur with Grant on this - the dependency from test to demo has caused me annoyance as well. I'm in favor of a fix to it, but haven't looked at Grant's solution yet.

        Show
        Erik Hatcher added a comment - I concur with Grant on this - the dependency from test to demo has caused me annoyance as well. I'm in favor of a fix to it, but haven't looked at Grant's solution yet.
        Hide
        Grant Ingersoll added a comment -

        I am not happy with the replication thing, but if you can't build something in the main source tree without having the "demo" code then that just doesn't feel right. What happens when someone changes the demo? Now we have to keep this demo code exactly the same lest we also want to update the dependencies in the main source as well. I suppose it isn't a big deal since it is only one file, but it just seems like an unnecessary dependency. Ideally, the FileDocument would live in some common place, but I am not sure where that would be.

        I don't include the demo source under my source tree in intellij, so it always complains at me (I hate those red squiggly marks), but I suppose I can get over it for this one file.

        Show
        Grant Ingersoll added a comment - I am not happy with the replication thing, but if you can't build something in the main source tree without having the "demo" code then that just doesn't feel right. What happens when someone changes the demo? Now we have to keep this demo code exactly the same lest we also want to update the dependencies in the main source as well. I suppose it isn't a big deal since it is only one file, but it just seems like an unnecessary dependency. Ideally, the FileDocument would live in some common place, but I am not sure where that would be. I don't include the demo source under my source tree in intellij, so it always complains at me (I hate those red squiggly marks), but I suppose I can get over it for this one file.
        Hide
        Doug Cutting added a comment -

        Why should we replicate this file in the t est tree? I don't see the need to make unit tests independent of the demo code. Why do you remove the demo code?

        Show
        Doug Cutting added a comment - Why should we replicate this file in the t est tree? I don't see the need to make unit tests independent of the demo code. Why do you remove the demo code?
        Grant Ingersoll made changes -
        Field Original Value New Value
        Attachment testdoc.patch [ 12321852 ]
        Attachment FileDocument.java [ 12321853 ]
        Hide
        Grant Ingersoll added a comment -

        Attached is a new file FileDocument that lives under the Test structure, and changes to the appropriate Tests (TestDoc and IndexTest) that use this class.

        Show
        Grant Ingersoll added a comment - Attached is a new file FileDocument that lives under the Test structure, and changes to the appropriate Tests (TestDoc and IndexTest) that use this class.
        Grant Ingersoll created issue -

          People

          • Assignee:
            Michael Busch
            Reporter:
            Grant Ingersoll
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development