TINKERPOP-1784 GLV Test Framework #747
Conversation
| @@ -36,15 +36,27 @@ namespace Gremlin.Net.Driver.Remote | |||
| public class DriverRemoteConnection : IRemoteConnection, IDisposable | |||
| { | |||
| private readonly IGremlinClient _client; | |||
| private readonly string _traversalSource; | |||
gremlin-python/pom.xml
Outdated
| failonerror="true"> | ||
| <env key="PYTHONPATH" value=""/> | ||
| <arg line="-e -t -b ${project.build.directory}/python2/radish ${project.basedir}/../gremlin-test/features/"/> <!-- -no-line-jump --> | ||
| </exec> |
There was a problem hiding this comment.
Nit: undesired additional spaces after exec close tag.
|
|
||
| @then("nothing should happen because") | ||
| def nothing_happening(step): | ||
| return |
There was a problem hiding this comment.
Shouldn't this be marked as ignore instead of success?
There was a problem hiding this comment.
I'm not terribly pleased with how I did that actually. I just wanted a way to mark the test as "migrated" with some comment as to why it couldn't be fully implemented. Perhaps that can be improved in some way....do you think it is important for this PR?
There was a problem hiding this comment.
I think including steps on the feature to denote that can't be migrated is OK and the format is OK. I was commenting on the way python feature handles it, but it's not directly related to the test suite.
I agree that its not important for this patch.
|
|
||
| @Test | ||
| @Ignore("As it stands we won't have all of these tests migrated initially so there is no point to running this in full - it can be flipped on later") | ||
| public void shouldImplementAllProcessTestsAsFeatures() throws Exception { |
There was a problem hiding this comment.
This is a good way to verify how much of the tests have been migrated, but as it should be run once / few times, maybe a script / gist outside of the repo would be better, to avoid maintaining this source code in the future.
There was a problem hiding this comment.
As long as we continue to rely on the java process test suite I think it should run on every build. I can see people adding tests to the process suite but not to the GLV suite. Better to just catch it on mvn clean install than on a periodic test run to ensure the suites stay in sync. It's not a lengthy test I don't think....pretty fast to do the comparisons.
| { | ||
| _client = client ?? throw new ArgumentNullException(nameof(client)); | ||
| _traversalSource = traversalSource; |
There was a problem hiding this comment.
We could add null validation in the same form:
_traversalSource = traversalSource ?? throw new ArgumentNullException(nameof(traversalSource));|
|
||
| b.update(params) | ||
|
|
||
| # print _translate(step.text + " - " + str(b)) |
There was a problem hiding this comment.
Nit picking: remove commented line
|
@jorgebay fixed all the nits - thanks. |
|
Fixes look good to me! |
|
VOTE: +1 |
|
hm... with the new server start scripts, when running from within a directory like: I get the following issues: Also, a different failure for Running integration tests within a directory used to work and allowed fast builds... can we look into making it work? |
|
I think that there are a lot of path issues when building from outside of the root directory - gremlin-dotnet is not the only one. i typically just do this: If you think it's important to fix the path problems you get when running from a sub-directory, I think it's best to create an issue in JIRA and I can come back to it later. |
|
Ah, I see its possible to run integration test using: I don't think its necessary to allow builds from a subdir, so don't mind... as there is a way to build the projects themselves with a profile that includes integration tests Thanks, my maven-fu is not strong at all. |
|
I'm getting a path related issue on Gremlin.Net - Test module, while executing: From the root directory, I get: The same issue appears for |
|
This is strange. That command works for me. I'm not sure what could be wrong..... |
| When iterated to list | ||
| Then the result should be unordered | ||
| | result | | ||
| | m[{"name":["marko"], "age":[29]}] | |
There was a problem hiding this comment.
We should specify the expected numeric type when describing map results because it has different meaning across different scenarios, here is an int32, below it's also a int64 (ie: groupCount()) and also a double.
The json literal for numbers could be reserved for double (JS Number underlying representation) and the rest we could use something like:
m[{"age":"d[29]"}], value is anint32.m[{"age":"d[29L]"}], value is anint64.m[{"age":29}], value is andouble.
There was a problem hiding this comment.
sigh - stupid numbers. i wondered when that would be a problem. obviously the problem is sooner than later. I think will implement in a manner similar to how i designated identifiers on vertices, by adding a "modulator" after the "d" specification. More specifically:
d[29].i= int32d[29].l= int64d[29].f= floatd[29].d= double
| When iterated to list | ||
| Then the result should be unordered | ||
| | result | | ||
| | d[3.5].d | |
There was a problem hiding this comment.
The type of the result is BigDecimal
gremlin> g.withSack(0.0).V().outE().sack(Operator.sum).by("weight").inV().sack().sum().next().getClass()
==>class java.math.BigDecimalWe should introduce another suffix for decimals.
| When iterated to list | ||
| Then the result should be unordered | ||
| | result | | ||
| | m[{"ripple":"d[1.0]".d,"lop":"d[0.3333333333333333].d"}] | |
There was a problem hiding this comment.
Typo: "d[1.0]".d, use "d[1.0].d" instead.
| When iterated to list | ||
| Then the result should be unordered | ||
| | result | | ||
| | m[{"software":"d[0].i", "person":"d[3.5].d}] | |
| When iterated to list | ||
| Then the result should be unordered | ||
| | result | | ||
| | m[{"ripple":"d[1.0].d","lop":"d[0.2]d"}] | |
There was a problem hiding this comment.
Typo: should be "d[0.2].d"
|
yeah - i'm just realizing how i flubbed a bunch of things on the last commit. i was so focused on your .net problems i only ran tests for .net 😞 |
|
@jorgebay On which OS do you get the test errors? |
|
@robertdale thanks for looking into it. I'm running This is the output for |
| When iterated to list | ||
| Then the result should be unordered | ||
| | result | | ||
| | d[123].l | |
There was a problem hiding this comment.
The expected value is an int.
| When iterated to list | ||
| Then the result should be unordered | ||
| | result | | ||
| | d[2.0].d | |
There was a problem hiding this comment.
These two are also big decimal...
|
VOTE: +1 |
… suite Uses Gherkin to write test specifications that will be implemented by the various GLVs. Provided a basic implementation for gremlin-python.
This matches he pattern of the java test suite.
Developed methods for vertices/maps and a way to assert unordered results.
… vertex object This might be the pattern to use across the board. We'll see if there is a better idea floating about though so may not be final.
Included infrastructure for validating maps and refactored other related code.
This is not complete, but it is enough for now.
Included Cardinality in imports for test logic
Tests were failing as a result of the change to using the mix server configuration that had all the graphs. On the way to dealing with that, I noticed the driver didn't seem to have aliasing capability which prevented it from choosing the correct graph traversal source on the server. For some reason, asserting longs on ids seemed to be a problem as well after this change and I'm pretty sure it has something to do with the configuration of the TinkerGraph in this new mixed mode configuration and not something in serialization.
Ensures that a RemoteConnection in .NET is properly initialized.
asfgit
force-pushed
the
TINKERPOP-1784
branch
from
e03cbd2
to
c0a3ce0
Compare
|
I've rebased this on top of I'm changing my VOTE to -1 until this is resolved. |
|
Interesting. Those errors don't seem to affect the build though - it still passes. I guess it's because we don't run any tests against those particular graphs on the "secure" server? or is it failing and i'm misinterpreting the build output? |
|
Once I realized that the failure was not preventing the build it was easy to spot the problem. Just pushed a fix. |
|
Nice! lgtm VOTE +1 |
https://issues.apache.org/jira/browse/TINKERPOP-1784
This PR is for the GLV Test Framework. It contains an implementation for gremlin-python. It is not a complete porting of all the process test suite, but does provide coverage for almost all steps. I think I've built enough here to provide enough for evaluation of the framework itself - my intention is to backfill tests after this merges. I do think I have enough tests here to yield confidence in GLVs that implement it, thus allowing baseline for official release.
All tests pass with
docker/build.sh -t -n -iVOTE +1