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

shouldPersistDataOnClose makes incorrect feature check

    Details

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

      Description

      Lines 1206-1211 in GraphTest.java (gremlin-test):

      final Vertex v = graph.addVertex();
      final Vertex u = graph.addVertex();
      if (graph.features().edge().properties().supportsStringValues())

      { v.property(VertexProperty.Cardinality.single, "name", "marko"); u.property(VertexProperty.Cardinality.single, "name", "pavel"); }

      The check tests for string value properties on edges, but it should be checking string values on properties of vertices.

        Activity

        Hide
        spmallette stephen mallette added a comment -

        the fix for this was merged on this issue: https://github.com/apache/incubator-tinkerpop/pull/152

        Show
        spmallette stephen mallette added a comment - the fix for this was merged on this issue: https://github.com/apache/incubator-tinkerpop/pull/152
        Hide
        Ellitron Jonathan Ellithorpe added a comment -

        Yup, just submitted a pull request.

        In the commit I included some additional notes about the feature requirements checking for this method (primarily, that the check for ADD_PROPERTY support on vertices is implicit, because the SIMPLE feature set doesn't check for that directly, but rather for being able to add meta properties).

        Also, I just saw in the SIMPLE feature set that a check is made for Graph.Features.VertexPropertyFeatures.FEATURE_STRING_VALUES. Is this if statement check in the unit test code itself even needed then in the first place? If not, perhaps the right thing to do is to delete it all together (in which case you'll want to disregard my PR which simply fixes the if statement check).

        Jonathan

        Show
        Ellitron Jonathan Ellithorpe added a comment - Yup, just submitted a pull request. In the commit I included some additional notes about the feature requirements checking for this method (primarily, that the check for ADD_PROPERTY support on vertices is implicit, because the SIMPLE feature set doesn't check for that directly, but rather for being able to add meta properties). Also, I just saw in the SIMPLE feature set that a check is made for Graph.Features.VertexPropertyFeatures.FEATURE_STRING_VALUES. Is this if statement check in the unit test code itself even needed then in the first place? If not, perhaps the right thing to do is to delete it all together (in which case you'll want to disregard my PR which simply fixes the if statement check). Jonathan
        Hide
        spmallette stephen mallette added a comment -

        yup -that looks like a bug there - do you want to submit a small pull request to fix that? if not, i can just tack that in there. please let me know.

        Show
        spmallette stephen mallette added a comment - yup -that looks like a bug there - do you want to submit a small pull request to fix that? if not, i can just tack that in there. please let me know.

          People

          • Assignee:
            spmallette stephen mallette
            Reporter:
            Ellitron Jonathan Ellithorpe
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development