Uploaded image for project: 'TinkerPop'
  1. TinkerPop
  2. TINKERPOP-984

Use GraphProvider for id conversion in Groovy Environment test suite

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 3.0.2-incubating
    • Fix Version/s: 3.1.1-incubating
    • Component/s: test-suite
    • Labels:
      None

      Description

      Groovy related tests rely on ID.toString(), which can cause problems with any implementation that uses non-primitive ids:

      Script1.groovy: 1: unexpected char: '#' @ line 1, column 5.
         g.V(#11:0).next()
      

        Activity

        Hide
        okram Marko A. Rodriguez added a comment -

        #11:0 is not a legal term. You probably need to do something like g.V(ID.of(#11:0)). I assume you are using OrientDB. Review their docs on how they recommend constructing the respective Java object for #11:0.

        Show
        okram Marko A. Rodriguez added a comment - #11:0 is not a legal term. You probably need to do something like g.V(ID.of(#11:0)) . I assume you are using OrientDB. Review their docs on how they recommend constructing the respective Java object for #11:0 .
        Hide
        okram Marko A. Rodriguez added a comment -

        The user needs to construct a Java object. Thus, this is not a bug.

        Show
        okram Marko A. Rodriguez added a comment - The user needs to construct a Java object. Thus, this is not a bug.
        Hide
        githubbot ASF GitHub Bot added a comment -

        GitHub user velo opened a pull request:

        https://github.com/apache/incubator-tinkerpop/pull/150

        TINKERPOP3-984

        Made possible to use GraphProvider to convert ids into Strings

        The idea is to prevent this:
        ````
        Script1.groovy: 1: unexpected char: '#' @ line 1, column 5.
        g.V(#11:0).next()
        ````

        You can merge this pull request into a Git repository by running:

        $ git pull https://github.com/velo/incubator-tinkerpop TINKERPOP3-984

        Alternatively you can review and apply these changes as the patch at:

        https://github.com/apache/incubator-tinkerpop/pull/150.patch

        To close this pull request, make a commit to your master/trunk branch
        with (at least) the following in the commit message:

        This closes #150


        commit de6899fee944658ae4e89ffe067b10b702afe31a
        Author: Marvin Froeder <velo.br@gmail.com>
        Date: 2015-11-20T23:27:00Z

        TINKERPOP3-984 - made possible to use GraphProvider to convert ids into Strings


        Show
        githubbot ASF GitHub Bot added a comment - GitHub user velo opened a pull request: https://github.com/apache/incubator-tinkerpop/pull/150 TINKERPOP3-984 Made possible to use GraphProvider to convert ids into Strings The idea is to prevent this: ```` Script1.groovy: 1: unexpected char: '#' @ line 1, column 5. g.V(#11:0).next() ```` You can merge this pull request into a Git repository by running: $ git pull https://github.com/velo/incubator-tinkerpop TINKERPOP3-984 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-tinkerpop/pull/150.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #150 commit de6899fee944658ae4e89ffe067b10b702afe31a Author: Marvin Froeder <velo.br@gmail.com> Date: 2015-11-20T23:27:00Z TINKERPOP3-984 - made possible to use GraphProvider to convert ids into Strings
        Hide
        velobr Marvin Froeder added a comment -

        If the string is properly quoted it works.

        I just tried and works like a charm. Just need a way to tell groovy suite how to turn the ID object into a String

        Show
        velobr Marvin Froeder added a comment - If the string is properly quoted it works. I just tried and works like a charm. Just need a way to tell groovy suite how to turn the ID object into a String
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user okram commented on the pull request:

        https://github.com/apache/incubator-tinkerpop/pull/150#issuecomment-158556964

        This is not how this should be done. There is a bug in the `GremlinGroovyScriptEngineOverGraphTest.java` test. Please see how we do it in the, for example, `GroovyGroupTest.groovy` in `gremlin-groovy-test`. Thus, instead of changing `GraphProvider`, we need to change `GremlinGroovyScriptEngineOverGraphTest` to use the standard model.

        Show
        githubbot ASF GitHub Bot added a comment - Github user okram commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/150#issuecomment-158556964 This is not how this should be done. There is a bug in the `GremlinGroovyScriptEngineOverGraphTest.java` test. Please see how we do it in the, for example, `GroovyGroupTest.groovy` in `gremlin-groovy-test`. Thus, instead of changing `GraphProvider`, we need to change `GremlinGroovyScriptEngineOverGraphTest` to use the standard model.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user velo commented on the pull request:

        https://github.com/apache/incubator-tinkerpop/pull/150#issuecomment-158558319

        ok, seems to be a good alternative... I will send another PR in a few

        Show
        githubbot ASF GitHub Bot added a comment - Github user velo commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/150#issuecomment-158558319 ok, seems to be a good alternative... I will send another PR in a few
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user velo commented on the pull request:

        https://github.com/apache/incubator-tinkerpop/pull/150#issuecomment-158559059

        question, shall I do this for ALL engine.eval() or just for the ones that pass ID as parameters?

        Show
        githubbot ASF GitHub Bot added a comment - Github user velo commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/150#issuecomment-158559059 question, shall I do this for ALL engine.eval() or just for the ones that pass ID as parameters?
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user okram commented on the pull request:

        https://github.com/apache/incubator-tinkerpop/pull/150#issuecomment-158562841

        Now that I think it through more – you can probably just do a `Bindings` and then `engine.eval("g.V(id).blah.blah",bindings)` where `bindings.set("id",convertToId("marko"))`.

        Show
        githubbot ASF GitHub Bot added a comment - Github user okram commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/150#issuecomment-158562841 Now that I think it through more – you can probably just do a `Bindings` and then `engine.eval("g.V(id).blah.blah",bindings)` where `bindings.set("id",convertToId("marko"))`.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user velo commented on the pull request:

        https://github.com/apache/incubator-tinkerpop/pull/150#issuecomment-158564739

        Yeap, that is what I did, just re-running tests and as soon I get a green light I will update this PR

        Show
        githubbot ASF GitHub Bot added a comment - Github user velo commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/150#issuecomment-158564739 Yeap, that is what I did, just re-running tests and as soon I get a green light I will update this PR
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user spmallette commented on the pull request:

        https://github.com/apache/incubator-tinkerpop/pull/150#issuecomment-158568005

        I wondered why Titan never had problems with the Groovy environment tests - doesn't look like they are implemented - i guess that explains it.

        Show
        githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/150#issuecomment-158568005 I wondered why Titan never had problems with the Groovy environment tests - doesn't look like they are implemented - i guess that explains it.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user velo commented on the pull request:

        https://github.com/apache/incubator-tinkerpop/pull/150#issuecomment-158800766

        Travis is trying to run rvm... I guess it is not that reliable right now...

        Show
        githubbot ASF GitHub Bot added a comment - Github user velo commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/150#issuecomment-158800766 Travis is trying to run rvm... I guess it is not that reliable right now...
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user okram commented on the pull request:

        https://github.com/apache/incubator-tinkerpop/pull/150#issuecomment-158805278

        If you are happy with your PR @velo, we can all do our respective reviews and VOTE. Just please tell us when you are ready.

        Show
        githubbot ASF GitHub Bot added a comment - Github user okram commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/150#issuecomment-158805278 If you are happy with your PR @velo, we can all do our respective reviews and VOTE. Just please tell us when you are ready.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user velo commented on the pull request:

        https://github.com/apache/incubator-tinkerpop/pull/150#issuecomment-158808126

        @okram feel free to call voters upon this PR

        Show
        githubbot ASF GitHub Bot added a comment - Github user velo commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/150#issuecomment-158808126 @okram feel free to call voters upon this PR
        Hide
        spmallette stephen mallette added a comment -

        I guess this should be re-opened as there is a bit of a bug at play in the tests suite for which a PR was submitted.

        Show
        spmallette stephen mallette added a comment - I guess this should be re-opened as there is a bit of a bug at play in the tests suite for which a PR was submitted.
        Hide
        spmallette stephen mallette added a comment -

        Updated title to better reflect the issue being solved.

        Show
        spmallette stephen mallette added a comment - Updated title to better reflect the issue being solved.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user okram commented on the pull request:

        https://github.com/apache/incubator-tinkerpop/pull/150#issuecomment-158962321

        I reviewed the code and ran `mvn clean install -DincludeNeo4j` and `BUILD SUCCESSFUL`. Yes, @velo , we simply had a set of cases that we not using bindings in an intelligent way. Thanks for fixing them up.

        When we merge this, I will make a TinkerGraph provider/test that uses an object id that does not have a nice `toString()` so we make sure this doesn't pop up again.

        VOTE +1.

        Show
        githubbot ASF GitHub Bot added a comment - Github user okram commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/150#issuecomment-158962321 I reviewed the code and ran `mvn clean install -DincludeNeo4j` and `BUILD SUCCESSFUL`. Yes, @velo , we simply had a set of cases that we not using bindings in an intelligent way. Thanks for fixing them up. When we merge this, I will make a TinkerGraph provider/test that uses an object id that does not have a nice `toString()` so we make sure this doesn't pop up again. VOTE +1.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user spmallette commented on the pull request:

        https://github.com/apache/incubator-tinkerpop/pull/150#issuecomment-158966996

        I merged this locally and did a `mvn clean install -DincludeNeo4j` and `mvn clean install -DskipIntegrationTests=false -pl tinkergraph-gremlin` to at least exercise the integration tests for groovy. Of course, neither of these is a perfect tests here because neither Neo4j nor TinkerGraph use complex objects for identifiers. If OrientDB is good with this change then this should be good to go.

        VOTE: +1

        Show
        githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/150#issuecomment-158966996 I merged this locally and did a `mvn clean install -DincludeNeo4j` and `mvn clean install -DskipIntegrationTests=false -pl tinkergraph-gremlin` to at least exercise the integration tests for groovy. Of course, neither of these is a perfect tests here because neither Neo4j nor TinkerGraph use complex objects for identifiers. If OrientDB is good with this change then this should be good to go. VOTE: +1
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user okram commented on the pull request:

        https://github.com/apache/incubator-tinkerpop/pull/150#issuecomment-158971500

        At @spmallette — Once we merge this, I will make a `TinkerGraphUUIDProvider` that will allow us to make sure all our tests are good for OrientDB (and others) moving forward. This way, we will be confident OrientDB will work.

        Show
        githubbot ASF GitHub Bot added a comment - Github user okram commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/150#issuecomment-158971500 At @spmallette — Once we merge this, I will make a `TinkerGraphUUIDProvider` that will allow us to make sure all our tests are good for OrientDB (and others) moving forward. This way, we will be confident OrientDB will work.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user velo commented on the pull request:

        https://github.com/apache/incubator-tinkerpop/pull/150#issuecomment-159011789

        Nice @okram

        Show
        githubbot ASF GitHub Bot added a comment - Github user velo commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/150#issuecomment-159011789 Nice @okram
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user dkuppitz commented on the pull request:

        https://github.com/apache/incubator-tinkerpop/pull/150#issuecomment-159107744

        Tests pass and code looks good. However, I wonder why you've used `graphProvider.convertId(1, Vertex.class)` instead of `convertToVertexId("marko")`. But I guess I'm missing something here, since @okram mentioned that he reviewed the code and he's fine with it. Hence:

        VOTE: +1

        Show
        githubbot ASF GitHub Bot added a comment - Github user dkuppitz commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/150#issuecomment-159107744 Tests pass and code looks good. However, I wonder why you've used `graphProvider.convertId(1, Vertex.class)` instead of `convertToVertexId("marko")`. But I guess I'm missing something here, since @okram mentioned that he reviewed the code and he's fine with it. Hence: VOTE: +1
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user velo commented on the pull request:

        https://github.com/apache/incubator-tinkerpop/pull/150#issuecomment-159109187

        @dkuppitz the original code was ````g.V(1)```` them I just used graphProvider to turn this into a proper ID

        Show
        githubbot ASF GitHub Bot added a comment - Github user velo commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/150#issuecomment-159109187 @dkuppitz the original code was ````g.V(1)```` them I just used graphProvider to turn this into a proper ID
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user dkuppitz commented on the pull request:

        https://github.com/apache/incubator-tinkerpop/pull/150#issuecomment-159110866

        I saw that, but I still think that `convertToVertexId("marko")` is the cleaner (cleanest?) way to do it, especially if the test uses the modern graph.

        Show
        githubbot ASF GitHub Bot added a comment - Github user dkuppitz commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/150#issuecomment-159110866 I saw that, but I still think that `convertToVertexId("marko")` is the cleaner (cleanest?) way to do it, especially if the test uses the modern graph.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user okram commented on the pull request:

        https://github.com/apache/incubator-tinkerpop/pull/150#issuecomment-159115748

        The problem is that these are not MODERN tests. When I merge, I'll fiddle to see if we can make these full modern. Else, I think we have to go with what @velo has. I'll handle this tomorrow and report back.

        Show
        githubbot ASF GitHub Bot added a comment - Github user okram commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/150#issuecomment-159115748 The problem is that these are not MODERN tests. When I merge, I'll fiddle to see if we can make these full modern. Else, I think we have to go with what @velo has. I'll handle this tomorrow and report back.
        Show
        githubbot ASF GitHub Bot added a comment - Github user dkuppitz commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/150#issuecomment-159117925 > The problem is that these are not MODERN tests. [It looks pretty MODERN] ( https://github.com/velo/incubator-tinkerpop/blob/TINKERPOP3-984/gremlin-groovy-test/src/main/groovy/org/apache/tinkerpop/gremlin/groovy/loaders/SugarLoaderTest.groovy#L75 ) to me.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user okram commented on the pull request:

        https://github.com/apache/incubator-tinkerpop/pull/150#issuecomment-159118488

        Ah, you are right. Good find. Okay, tomorrow on merge, I'll clean it up along with adding `TinkerGraphUUIDProvider` so we don't have to rely on OrientDB to test this.

        Show
        githubbot ASF GitHub Bot added a comment - Github user okram commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/150#issuecomment-159118488 Ah, you are right. Good find. Okay, tomorrow on merge, I'll clean it up along with adding `TinkerGraphUUIDProvider` so we don't have to rely on OrientDB to test this.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user asfgit closed the pull request at:

        https://github.com/apache/incubator-tinkerpop/pull/150

        Show
        githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/incubator-tinkerpop/pull/150

          People

          • Assignee:
            okram Marko A. Rodriguez
            Reporter:
            velobr Marvin Froeder
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development