Lucene - Core
  1. Lucene - Core
  2. LUCENE-2318

Add System.getProperty("tempDir") as final static to LuceneTestCase(J4)

    Details

    • Type: Test Test
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.0-ALPHA
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New, Patch Available

      Description

      Almost every test calls System.getProperty("tempDir") and some of them check the return value for null. In other cases the test simply fails from within eclipse.

      We should add this to LuceneTestCase(J4) as a static final constant. For enabling tests run in eclipse, we can add a fallback to ".", if the Sysprop is not defined.

      1. LUCENE-2318.patch
        34 kB
        Uwe Schindler
      2. LUCENE-2318.patch
        38 kB
        Uwe Schindler

        Issue Links

          Activity

          Hide
          Shai Erera added a comment -

          Uwe, can you default to "java.io.tmpdir" instead? "." is not properly defined. It will create indexes in the current directory where the tests run from, which is different if I run "ant test" from <root>, <root>/contrib and <root>/benchmark ...

          Or, we can tweak common-build.xml to fallback to <root>/test. In fact, looking in common-build.xml, I already see tempDir defaults to

          {build.dir}

          /test. Look at lines 448 (where it is set), 417 where it is used and 418 where java.io.tmpdir is set to that value.

          Maybe we need to change the definition of build.dir from location="build" to location="

          {common.dir}

          /build" so that it always references <root>/build.

          And if run from eclipse, default TEMP_DIR constant to "java.io.tmpdir"?

          Show
          Shai Erera added a comment - Uwe, can you default to "java.io.tmpdir" instead? "." is not properly defined. It will create indexes in the current directory where the tests run from, which is different if I run "ant test" from <root>, <root>/contrib and <root>/benchmark ... Or, we can tweak common-build.xml to fallback to <root>/test. In fact, looking in common-build.xml, I already see tempDir defaults to {build.dir} /test. Look at lines 448 (where it is set), 417 where it is used and 418 where java.io.tmpdir is set to that value. Maybe we need to change the definition of build.dir from location="build" to location=" {common.dir} /build" so that it always references <root>/build. And if run from eclipse, default TEMP_DIR constant to "java.io.tmpdir"?
          Hide
          Uwe Schindler added a comment -

          Uwe, can you default to "java.io.tmpdir" instead?

          The problem are only test runs outside ANT. If you run the tests from ANT there is never a problem, they use $

          {build.dir}

          /test, which is perfectly defined. But the tempDir system property is not set when running the tests from eclipse. So Your suggestion to fallback to java.io.tmpdir is a good idea, maybe with "/lucene-test" appended.

          Maybe we need to change the definition of build.dir from location="build" to location="{common.dir}/build" so that it always references <root>/build.

          This would break contrib. Its defined correctly at the moment, as ANT guarantees that "." or the current dir defaults to the `dirname build.xml`.

          Or, we can tweak common-build.xml to fallback to <root>/test.

          common-build never needs a fallback, as its always defined. and <root>/test is exactly what we do not want to have as it will not be cleaned up on "ant clean"

          Show
          Uwe Schindler added a comment - Uwe, can you default to "java.io.tmpdir" instead? The problem are only test runs outside ANT. If you run the tests from ANT there is never a problem, they use $ {build.dir} /test, which is perfectly defined. But the tempDir system property is not set when running the tests from eclipse. So Your suggestion to fallback to java.io.tmpdir is a good idea, maybe with "/lucene-test" appended. Maybe we need to change the definition of build.dir from location="build" to location="{common.dir}/build" so that it always references <root>/build. This would break contrib. Its defined correctly at the moment, as ANT guarantees that "." or the current dir defaults to the `dirname build.xml`. Or, we can tweak common-build.xml to fallback to <root>/test. common-build never needs a fallback, as its always defined. and <root>/test is exactly what we do not want to have as it will not be cleaned up on "ant clean"
          Hide
          Uwe Schindler added a comment -

          Here the patch.

          I removed almost all System.getProperty calls from tests. Only remaining are the special properties in benchmark (maybe that can be solved, too) and System.getProperty("line.separator") - but that's fine.

          LuceneTestCaseJ4 now has two fields of type java.io.File:

          • TEMP_DIR that is retrieved from system property "tempDir", set by ANT or, if not available from "java.io.tmpdir". By this tests will also run correctly from eclipse. This is static.
          • DATA_DIR (deprecated) that is used by some contrib tests. This should not be used, instead the tests should use this.getClass().getResource/getResourceAsStream, which is also relative to the current tests. I did not change the tests to use this. DATA_DIR is detected from system property (set by ANT), else it is loaded from classpath of the current test class, because of that it is not static (but final) and initialized in the class ctor.
          Show
          Uwe Schindler added a comment - Here the patch. I removed almost all System.getProperty calls from tests. Only remaining are the special properties in benchmark (maybe that can be solved, too) and System.getProperty("line.separator") - but that's fine. LuceneTestCaseJ4 now has two fields of type java.io.File: TEMP_DIR that is retrieved from system property "tempDir", set by ANT or, if not available from "java.io.tmpdir". By this tests will also run correctly from eclipse. This is static. DATA_DIR (deprecated) that is used by some contrib tests. This should not be used, instead the tests should use this.getClass().getResource/getResourceAsStream, which is also relative to the current tests. I did not change the tests to use this. DATA_DIR is detected from system property (set by ANT), else it is loaded from classpath of the current test class, because of that it is not static (but final) and initialized in the class ctor.
          Hide
          Uwe Schindler added a comment -

          New patch, removed usage of DATA_DIR again.

          All tests that simply needed an InputStream were converted to getResourceAsStream(). Other tests that really need a File instance can use LuceneTestCase(J4).getDataFile(String), which was heavily borrowed from the PorterTestCase, I moved the code there. Its only few tests that use this function.

          This is now ready to commit.

          Show
          Uwe Schindler added a comment - New patch, removed usage of DATA_DIR again. All tests that simply needed an InputStream were converted to getResourceAsStream(). Other tests that really need a File instance can use LuceneTestCase(J4).getDataFile(String), which was heavily borrowed from the PorterTestCase, I moved the code there. Its only few tests that use this function. This is now ready to commit.
          Hide
          Shai Erera added a comment -

          Patch looks good.

          I see that you've documented TEST_VERSION_CURRENT in LuceneTestCase(J4) as: /** Use this constant when creating Analyzers. */. It's used already for creating IndexWriterConfig,and I assume it will be used for other components as we'll introduce it in them (LUCENE-2305). So perhaps generalize the jdoc?

          It bothered me for a long time that I cannot run some tests from eclipse because of this 'tempDir'. I'm glad you fixed it !

          Show
          Shai Erera added a comment - Patch looks good. I see that you've documented TEST_VERSION_CURRENT in LuceneTestCase(J4) as: /** Use this constant when creating Analyzers. */. It's used already for creating IndexWriterConfig,and I assume it will be used for other components as we'll introduce it in them ( LUCENE-2305 ). So perhaps generalize the jdoc? It bothered me for a long time that I cannot run some tests from eclipse because of this 'tempDir'. I'm glad you fixed it !
          Hide
          Uwe Schindler added a comment -

          OK, I fixed the javadocs here! Thanks.

          Show
          Uwe Schindler added a comment - OK, I fixed the javadocs here! Thanks.
          Hide
          Uwe Schindler added a comment -

          Committed revision: 922886

          Show
          Uwe Schindler added a comment - Committed revision: 922886

            People

            • Assignee:
              Uwe Schindler
              Reporter:
              Uwe Schindler
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development