Solr
  1. Solr
  2. SOLR-5764

Fix recently added tests to *not* use absolute paths to load resources, use SolrTestCaseJ4.getFile() and getResource() instead; fix morphlines/map-reduce to not duplicate test resources and fix dependencies among them

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 4.7
    • Fix Version/s: 4.8, 6.0
    • Component/s: None
    • Labels:
      None

      Description

      Tests that were recently added have the problems that some of them were using ExternalPaths.SOURCE_HOME and adding the absolute location of modules. This makes it impossible to move modules around or run them standalone.

      This issue fixes those tests to use SolrTestCaseJ4.getFile() or a simple openResourceAsStream from classpath, because all test-files are located in classpath. ExternalPaths.SOURCE_HOME will be hidden (private) so nothing can use it anymore - so no new tests should appear using them again.

      While doing this, I recognized that the recently added morphlines and mapreduce contribs are duplicating modified solr example folders into their test-files folder, while morphlines-core was referring from inside the tests to map-reduce/test-files (ignoring the whole megabytes of test-files of morphlines!!! Easy to see, because the config files in it were just broken and referring to non-existent classes). This is the reverse of the natural dependency, where map-reduce uses morphlines as dep.

      The fix here is to completely delete/empty the test-files folders of map-reduce and morphlines-cell and let those reuse the one from morphlines-core. Morphlines-core is now the only source folder for test-files, shared by all three contribs. Its contents are now the previous map-reduce/test-files folder.

      I will provide a patch with the code changes, but the cleanup of morphlines test-files cannot be described here, it is too crazy. I will just heavy commit the changes, sorry!

      1. SOLR-5764.patch
        30 kB
        Uwe Schindler

        Activity

        Hide
        Mark Miller added a comment -

        Thanks Uwe! Yeah, there was a bunch of duplication because the initial code was not developed in the project. I focused pretty much just on getting something committed that could pass all our checks and tests, so there is probably a fair amount of cleanup yet to do. That's part of the driving force behind marking it experimental. It would also be good to take into account other developers opinions on the API's and such.

        Show
        Mark Miller added a comment - Thanks Uwe! Yeah, there was a bunch of duplication because the initial code was not developed in the project. I focused pretty much just on getting something committed that could pass all our checks and tests, so there is probably a fair amount of cleanup yet to do. That's part of the driving force behind marking it experimental. It would also be good to take into account other developers opinions on the API's and such.
        Hide
        Uwe Schindler added a comment -

        This is a patch of the code changes to fix analytics and morphline/mapreduce tests. This patch also hides the constant to get source home dir, so tests cannot use it.

        The file moves in morphlines are not included.

        This patch also removes the copy of src/test-files to build/test-files (which was a hack to prevent broken test from overriding test-files). The security manager prevents this, so this is no longer needed.

        Show
        Uwe Schindler added a comment - This is a patch of the code changes to fix analytics and morphline/mapreduce tests. This patch also hides the constant to get source home dir, so tests cannot use it. The file moves in morphlines are not included. This patch also removes the copy of src/test-files to build/test-files (which was a hack to prevent broken test from overriding test-files). The security manager prevents this, so this is no longer needed.
        Hide
        Uwe Schindler added a comment -

        I will commit this soon and backport to 4.x. 4.7 will stay as-is.

        Show
        Uwe Schindler added a comment - I will commit this soon and backport to 4.x. 4.7 will stay as-is.
        Hide
        ASF subversion and git services added a comment -

        Commit 1570898 from Uwe Schindler in branch 'dev/trunk'
        [ https://svn.apache.org/r1570898 ]

        SOLR-5764: Fix recently added tests to not use absolute paths to load test-files, use SolrTestCaseJ4.getFile() and getResource() instead; fix morphlines/map-reduce to not duplicate test resources and fix dependencies among them.

        Show
        ASF subversion and git services added a comment - Commit 1570898 from Uwe Schindler in branch 'dev/trunk' [ https://svn.apache.org/r1570898 ] SOLR-5764 : Fix recently added tests to not use absolute paths to load test-files, use SolrTestCaseJ4.getFile() and getResource() instead; fix morphlines/map-reduce to not duplicate test resources and fix dependencies among them.
        Hide
        ASF subversion and git services added a comment -

        Commit 1570910 from Uwe Schindler in branch 'dev/branches/branch_4x'
        [ https://svn.apache.org/r1570910 ]

        Merged revision(s) 1570898 from lucene/dev/trunk:
        SOLR-5764: Fix recently added tests to not use absolute paths to load test-files, use SolrTestCaseJ4.getFile() and getResource() instead; fix morphlines/map-reduce to not duplicate test resources and fix dependencies among them.

        Show
        ASF subversion and git services added a comment - Commit 1570910 from Uwe Schindler in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1570910 ] Merged revision(s) 1570898 from lucene/dev/trunk: SOLR-5764 : Fix recently added tests to not use absolute paths to load test-files, use SolrTestCaseJ4.getFile() and getResource() instead; fix morphlines/map-reduce to not duplicate test resources and fix dependencies among them.
        Hide
        Uwe Schindler added a comment -

        I fixed this for now.

        Ideally, we should review the test-resources folder of morphlines-core and maybe correctly split it up so every module has its own folder with only the minimum amount of files.

        I am not sure if the Maven build works correctly, because I cannot fully test it: it complains about some missing jersey artifact here. Nevertheless, the map-reduce and morphlines-cell already depend on morphlines-core with scope=test, so the shared test-files folder should work correctly.

        Show
        Uwe Schindler added a comment - I fixed this for now. Ideally, we should review the test-resources folder of morphlines-core and maybe correctly split it up so every module has its own folder with only the minimum amount of files. I am not sure if the Maven build works correctly, because I cannot fully test it: it complains about some missing jersey artifact here. Nevertheless, the map-reduce and morphlines-cell already depend on morphlines-core with scope=test, so the shared test-files folder should work correctly.
        Hide
        ASF subversion and git services added a comment -

        Commit 1570928 from Uwe Schindler in branch 'dev/trunk'
        [ https://svn.apache.org/r1570928 ]

        SOLR-5764: Add workaround to make Maven work (shared test-files does not seem to be included when the test-jar dependency is resolved).

        Show
        ASF subversion and git services added a comment - Commit 1570928 from Uwe Schindler in branch 'dev/trunk' [ https://svn.apache.org/r1570928 ] SOLR-5764 : Add workaround to make Maven work (shared test-files does not seem to be included when the test-jar dependency is resolved).
        Hide
        ASF subversion and git services added a comment -

        Commit 1570929 from Uwe Schindler in branch 'dev/branches/branch_4x'
        [ https://svn.apache.org/r1570929 ]

        Merged revision(s) 1570928 from lucene/dev/trunk:
        SOLR-5764: Add workaround to make Maven work (shared test-files does not seem to be included when the test-jar dependency is resolved).

        Show
        ASF subversion and git services added a comment - Commit 1570929 from Uwe Schindler in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1570929 ] Merged revision(s) 1570928 from lucene/dev/trunk: SOLR-5764 : Add workaround to make Maven work (shared test-files does not seem to be included when the test-jar dependency is resolved).
        Hide
        Uwe Schindler added a comment -

        Steve Rowe: I added a hack into the maven poms. The dependnecy does not seem to include the shared test-files folder, which is provided by solr-morphlines-core. solr-morphlines-cell and solr-map-reduce fail to lookup the resource. Maybe you can fix this by including the test-files folder to the test artifact.

        Show
        Uwe Schindler added a comment - Steve Rowe : I added a hack into the maven poms. The dependnecy does not seem to include the shared test-files folder, which is provided by solr-morphlines-core. solr-morphlines-cell and solr-map-reduce fail to lookup the resource. Maybe you can fix this by including the test-files folder to the test artifact.
        Hide
        ASF subversion and git services added a comment -

        Commit 1570931 from Uwe Schindler in branch 'dev/trunk'
        [ https://svn.apache.org/r1570931 ]

        SOLR-5764: Make error message of SolrTestCaseJ4.getFile() more readable.

        Show
        ASF subversion and git services added a comment - Commit 1570931 from Uwe Schindler in branch 'dev/trunk' [ https://svn.apache.org/r1570931 ] SOLR-5764 : Make error message of SolrTestCaseJ4.getFile() more readable.
        Hide
        ASF subversion and git services added a comment -

        Commit 1570932 from Uwe Schindler in branch 'dev/branches/branch_4x'
        [ https://svn.apache.org/r1570932 ]

        Merged revision(s) 1570931 from lucene/dev/trunk:
        SOLR-5764: Make error message of SolrTestCaseJ4.getFile() more readable.

        Show
        ASF subversion and git services added a comment - Commit 1570932 from Uwe Schindler in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1570932 ] Merged revision(s) 1570931 from lucene/dev/trunk: SOLR-5764 : Make error message of SolrTestCaseJ4.getFile() more readable.
        Hide
        ASF subversion and git services added a comment -

        Commit 1570937 from Uwe Schindler in branch 'dev/trunk'
        [ https://svn.apache.org/r1570937 ]

        SOLR-5764: More improvements.

        Show
        ASF subversion and git services added a comment - Commit 1570937 from Uwe Schindler in branch 'dev/trunk' [ https://svn.apache.org/r1570937 ] SOLR-5764 : More improvements.
        Hide
        ASF subversion and git services added a comment -

        Commit 1570938 from Uwe Schindler in branch 'dev/branches/branch_4x'
        [ https://svn.apache.org/r1570938 ]

        Merged revision(s) 1570937 from lucene/dev/trunk:
        SOLR-5764: More improvements.

        Show
        ASF subversion and git services added a comment - Commit 1570938 from Uwe Schindler in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1570938 ] Merged revision(s) 1570937 from lucene/dev/trunk: SOLR-5764 : More improvements.
        Hide
        ASF subversion and git services added a comment -

        Commit 1570955 from Mark Miller in branch 'dev/trunk'
        [ https://svn.apache.org/r1570955 ]

        SOLR-5764: Set eol-style on test resources

        Show
        ASF subversion and git services added a comment - Commit 1570955 from Mark Miller in branch 'dev/trunk' [ https://svn.apache.org/r1570955 ] SOLR-5764 : Set eol-style on test resources
        Hide
        ASF subversion and git services added a comment -

        Commit 1570960 from Mark Miller in branch 'dev/branches/branch_4x'
        [ https://svn.apache.org/r1570960 ]

        SOLR-5764: Set eol-style on test resources

        Show
        ASF subversion and git services added a comment - Commit 1570960 from Mark Miller in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1570960 ] SOLR-5764 : Set eol-style on test resources
        Hide
        Steve Rowe added a comment -

        Steve Rowe: I added a hack into the maven poms. The dependnecy does not seem to include the shared test-files folder, which is provided by solr-morphlines-core. solr-morphlines-cell and solr-map-reduce fail to lookup the resource. Maybe you can fix this by including the test-files folder to the test artifact.

        Uwe, I checked, and the test-files folder is already included in the morphlines-core test artifact.

        They don't fail to lookup the resource, they find it, but they find it in the jar, and so print out this message:

        java.lang.RuntimeException: Resource was found on classpath, but cannot be resolved to a normal file (maybe it is part of a JAR file): morphlines-core.marker
        	at org.apache.solr.SolrTestCaseJ4.getFile(SolrTestCaseJ4.java:1673)
        	at org.apache.solr.hadoop.MorphlineBasicMiniMRTest.<clinit>(MorphlineBasicMiniMRTest.java:67)
        

        I think your hack is the best we can do, since some of the tests I looked at require files to be located on the filesystem. Putting in some Maven-only logic to unpack the test jar to the filesystem seems to me like overkill given that your hack works.

        Show
        Steve Rowe added a comment - Steve Rowe: I added a hack into the maven poms. The dependnecy does not seem to include the shared test-files folder, which is provided by solr-morphlines-core. solr-morphlines-cell and solr-map-reduce fail to lookup the resource. Maybe you can fix this by including the test-files folder to the test artifact. Uwe, I checked, and the test-files folder is already included in the morphlines-core test artifact. They don't fail to lookup the resource, they find it, but they find it in the jar, and so print out this message: java.lang.RuntimeException: Resource was found on classpath, but cannot be resolved to a normal file (maybe it is part of a JAR file): morphlines-core.marker at org.apache.solr.SolrTestCaseJ4.getFile(SolrTestCaseJ4.java:1673) at org.apache.solr.hadoop.MorphlineBasicMiniMRTest.<clinit>(MorphlineBasicMiniMRTest.java:67) I think your hack is the best we can do, since some of the tests I looked at require files to be located on the filesystem. Putting in some Maven-only logic to unpack the test jar to the filesystem seems to me like overkill given that your hack works.
        Hide
        Uwe Schindler added a comment -

        He Steve,
        I improved the error messages by SolrTestCaseJ4#getFile yesterday, so the above error message is now more meaningful than the one I got.
        This test is not the only one requiring a "real file", most of Solr's tests does this. So getFile() is used quite often. In Lucene, most tests work without files, because they only need streams (we have no getFile on LTC for that reason).
        Uwe

        Show
        Uwe Schindler added a comment - He Steve, I improved the error messages by SolrTestCaseJ4#getFile yesterday, so the above error message is now more meaningful than the one I got. This test is not the only one requiring a "real file", most of Solr's tests does this. So getFile() is used quite often. In Lucene, most tests work without files, because they only need streams (we have no getFile on LTC for that reason). Uwe
        Hide
        Mark Miller added a comment -

        Uwe Schindler - is there other recent work in this area that was done? This, or perhaps something related seems to have recently made it so that test files cannot be found when running from Eclipse for some tests. Trying to track the change to investigate a solution.

        Show
        Mark Miller added a comment - Uwe Schindler - is there other recent work in this area that was done? This, or perhaps something related seems to have recently made it so that test files cannot be found when running from Eclipse for some tests. Trying to track the change to investigate a solution.
        Hide
        Mark Miller added a comment -

        To add: the tests I've noticed so far are in the core module.

        Show
        Mark Miller added a comment - To add: the tests I've noticed so far are in the core module.
        Hide
        Uwe Schindler added a comment -

        No, no changes with Eclipse. For me also morphlines/map-reduce tests work from inside eclipse. Be sure to run "ant clean-jars" and then "ant eclipse" to recreate the classpath.

        Show
        Uwe Schindler added a comment - No, no changes with Eclipse. For me also morphlines/map-reduce tests work from inside eclipse. Be sure to run "ant clean-jars" and then "ant eclipse" to recreate the classpath.
        Hide
        Uwe Schindler added a comment -

        FYI, I tried it again a minute ago. I can run all tests in Eclipse after correctly updating checkout and IVY without any problems. Only HDFS fails, because I am on windows.

        Show
        Uwe Schindler added a comment - FYI, I tried it again a minute ago. I can run all tests in Eclipse after correctly updating checkout and IVY without any problems. Only HDFS fails, because I am on windows.
        Hide
        Uwe Schindler added a comment -

        To add: the tests I've noticed so far are in the core module.

        Which one?

        Show
        Uwe Schindler added a comment - To add: the tests I've noticed so far are in the core module. Which one?
        Hide
        Uwe Schindler added a comment -

        Close issue after release of 4.8.0

        Show
        Uwe Schindler added a comment - Close issue after release of 4.8.0

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development