Skip to content

TINKERPOP-1865 Run Gremlin.Net tests with GraphSON 3.0#820

Merged
asfgit merged 3 commits intotp33from
TINKERPOP-1865
Mar 22, 2018
Merged

TINKERPOP-1865 Run Gremlin.Net tests with GraphSON 3.0#820
asfgit merged 3 commits intotp33from
TINKERPOP-1865

Conversation

@FlorianHockmann
Copy link
Copy Markdown
Member

https://issues.apache.org/jira/browse/TINKERPOP-1865

The tests simply failed because of the traversal used in ScenarioData.GetEdges(). I didn't dig deep into why it failed, but it had something to do with the deserialization of the keys of the received dictionary. I also couldn't get the traversal used there to work with gremlin-python, so I simply replaced it by two simpler traversals. This increases the runtime a bit as the second traversal now has to be executed for every edge, but I think that it improves the readability a lot.

Unfortunately, I had to add 3 more tests to the list of ignored ones. One failed due to the missing T deserializer (TINKERPOP-1866) and two failed because the data received from the server didn't match the expected data:

  • g_V_storeXaX_byXoutEXcreatedX_countX_out_out_storeXaX_byXinEXcreatedX_weight_sumX: We receive a g:Set of 8 values here, including 4 duplicates. This is deserialized to a HashSet by Gremlin.Net which means that the 8 values are reduced to 4 unique values, but the scenario expects a collection of 8 values.
  • g_VX1X_hasXlabel_personX_mapXmapXint_ageXX_orderXlocalX_byXvalues_decrX_byXkeys_incrX: Here we receive a map with g:Int32 keys (at least when my understanding of the g:Map type is correct), but the scenario expects string keys.

To me, this looks like mistakes in the scenarios, but it would be good if someone could verify that. I tried to modify the scenarios accordingly, but then both gremlin-python and gremlin-javascript failed for the scenario with the g:Set containing duplicates. So either the deserialization of g:Set is flawed in those two GLVs or my understanding of sets is wrong.

FlorianHockmann and others added 3 commits March 20, 2018 09:37
This required the rewrite of the GetEdges() function for the Gherkin
test runner. Additionally, 3 tests had to be ignored: 1 has the
T deserialization problem and 2 get data returned from the server that
doesn't match the expected data.
@jorgebay
Copy link
Copy Markdown
Contributor

I've rebased this branch to get the fixes for P.within() from #817.

Also, I've switched to use a lambda (now that we support it :) ) to obtain all the edges for the scenario data, that way we use the same traversal as Python. It would be nice to backport the same traversal for tp32 but it's not necessary for now.

About g_VX1X_hasXlabel_personX_mapXmapXint_ageXX_orderXlocalX_byXvalues_decrX_byXkeys_incrX, I've changed the lambda to return a Map<string, int>.

Regarding g_V_storeXaX_byXoutEXcreatedX_countX_out_out_storeXaX_byXinEXcreatedX_weight_sumX, its still ignored as the returned data doesn't match, it returns:

{ '@type': 'g:Set',
  '@value': 
   [ { '@type': 'g:Int64', '@value': 1 },
     { '@type': 'g:Int64', '@value': 1 },
     { '@type': 'g:Int64', '@value': 0 },
     { '@type': 'g:Int64', '@value': 0 },
     { '@type': 'g:Int64', '@value': 0 },
     { '@type': 'g:Int64', '@value': 2 },
     { '@type': 'g:Double', '@value': 1 },
     { '@type': 'g:Double', '@value': 1 }

Which is deserialized into a Set<object> containing only the different values (4 values in total), so it's a scenario design issue (not related to Gremlin.NET).

@FlorianHockmann I hope you don't mind that I've directly pushed to the dev branch, feel free to revert the commits if needed.

@FlorianHockmann
Copy link
Copy Markdown
Member Author

Also, I've switched to use a lambda (now that we support it :) ) to obtain all the edges for the scenario data,

That lambda really makes the traversal much easier to read and understand :)

For g_V_storeXaX_byXoutEXcreatedX_countX_out_out_storeXaX_byXinEXcreatedX_weight_sumX,
I'd say that we should change the scenario to only expect the 4 unique values and then fix gremlin-python and gremlin-javascript as they seem to deserialize sets incorrectly at the moment. (Otherwise they had to fail for this test at the moment.) But I'm not sure if we want to do all that in this PR.

@FlorianHockmann I hope you don't mind that I've directly pushed to the dev branch, feel free to revert the commits if needed.

Certainly not, thanks for helping me with this PR!

@jorgebay
Copy link
Copy Markdown
Contributor

I agree, we should fix g_V_storeXaX_byXoutEXcreatedX_countX_out_out_storeXaX_byXinEXcreatedX_weight_sumX outside of this pr.

Having the gherkin based tests running on GraphSON3 for Gremlin.NET is a huge improvement, VOTE +1

@FlorianHockmann
Copy link
Copy Markdown
Member Author

Ok, I created TINKERPOP-1927 to fix the scenarios.

VOTE +1

@spmallette
Copy link
Copy Markdown
Contributor

VOTE +1

@spmallette
Copy link
Copy Markdown
Contributor

I think that I need this one merged to properly test out TINKERPOP-1866 as T serialization only works right on GraphSON 3.0

@asfgit asfgit merged commit 4c64bc0 into tp33 Mar 22, 2018
@asfgit asfgit deleted the TINKERPOP-1865 branch April 14, 2018 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants