You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tinkerpop.apache.org by pluradj <gi...@git.apache.org> on 2016/06/15 12:55:35 UTC
[GitHub] tinkerpop pull request #335: TINKERPOP-1319: fix incorrect FeatureRequiremen...
GitHub user pluradj opened a pull request:
https://github.com/apache/tinkerpop/pull/335
TINKERPOP-1319: fix incorrect FeatureRequirement annotations
https://issues.apache.org/jira/browse/TINKERPOP-1319
Passed `mvn clean install -DincludeNeo4j` and integration tests cleanly.
May be CTR worthy. Let me know.
VOTE: +1
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/apache/tinkerpop TINKERPOP-1319
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/tinkerpop/pull/335.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 #335
----
commit 6fe943ff67cea8383afc6f046c175d89092601da
Author: Jason Plurad <pl...@us.ibm.com>
Date: 2016-06-15T12:51:43Z
fix incorrect FeatureRequirement annotations
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop pull request #335: TINKERPOP-1319: fix incorrect FeatureRequiremen...
Posted by pluradj <gi...@git.apache.org>.
Github user pluradj commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/335#discussion_r67160705
--- Diff: gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/structure/FeatureSupportTest.java ---
@@ -587,7 +587,7 @@ public void shouldSupportRemoveEdgesIfEdgeCanBeRemoved() throws Exception {
public void shouldSupportRemovePropertyIfAPropertyCanBeRemoved() throws Exception {
try {
final Vertex v = graph.addVertex();
- final Edge e = v.addEdge("self", v);
+ final Edge e = v.addEdge("self", v, "name", "foo");
--- End diff --
Agreed. I started with all features set to `false` then started turning things on one at a time and in different combinations.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #335: TINKERPOP-1319: fix incorrect FeatureRequirement annot...
Posted by okram <gi...@git.apache.org>.
Github user okram commented on the issue:
https://github.com/apache/tinkerpop/pull/335
VOTE +1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop pull request #335: TINKERPOP-1319: fix incorrect FeatureRequiremen...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/tinkerpop/pull/335
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #335: TINKERPOP-1319: fix incorrect FeatureRequirement annot...
Posted by pluradj <gi...@git.apache.org>.
Github user pluradj commented on the issue:
https://github.com/apache/tinkerpop/pull/335
```
[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary:
[INFO]
[INFO] Apache TinkerPop .................................. SUCCESS [49.025s]
[INFO] Apache TinkerPop :: Gremlin Shaded ................ SUCCESS [10.134s]
[INFO] Apache TinkerPop :: Gremlin Core .................. SUCCESS [33.447s]
[INFO] Apache TinkerPop :: Gremlin Test .................. SUCCESS [8.743s]
[INFO] Apache TinkerPop :: Gremlin Groovy ................ SUCCESS [1:28.938s]
[INFO] Apache TinkerPop :: Gremlin Groovy Test ........... SUCCESS [9.088s]
[INFO] Apache TinkerPop :: TinkerGraph Gremlin ........... SUCCESS [4:05.011s]
[INFO] Apache TinkerPop :: Hadoop Gremlin ................ SUCCESS [6:23.000s]
[INFO] Apache TinkerPop :: Spark Gremlin ................. SUCCESS [13:38.044s]
[INFO] Apache TinkerPop :: Giraph Gremlin ................ SUCCESS [2:01:30.357s]
[INFO] Apache TinkerPop :: Neo4j Gremlin ................. SUCCESS [31:45.126s]
[INFO] Apache TinkerPop :: Gremlin Driver ................ SUCCESS [8.683s]
[INFO] Apache TinkerPop :: Gremlin Server ................ SUCCESS [12:03.655s]
[INFO] Apache TinkerPop :: Gremlin Console ............... SUCCESS [1:57.427s]
[INFO] Apache TinkerPop :: Gremlin Archetype ............. SUCCESS [0.222s]
[INFO] Apache TinkerPop :: Archetype - TinkerGraph ....... SUCCESS [16.966s]
[INFO] Apache TinkerPop :: Archetype - Server ............ SUCCESS [13.702s]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 3:15:22.725s
[INFO] Finished at: Wed Jul 06 21:31:50 UTC 2016
[INFO] Final Memory: 106M/275M
[INFO] ------------------------------------------------------------------------
Untagged: tinkerpop:build-1467828977
Deleted: sha256:fc0be61b8011cafa062bca435b24f0f1fa407d4891c2f93ca831ae5dd17c4184
Deleted: sha256:125d80aa8cb9f1988c9545af45434b45e997fa197a05e21ff0be06d31eb5c9f9
Deleted: sha256:ad8bc8dc69b8cf2e2d1a45d71ec3ceac476b10d9d410741157940a990ea54050
Deleted: sha256:4343aa626aa513ab75e38a1605adeb64bf8db026f4cc56b42b343b4798319a15
Deleted: sha256:94de2ed6571c94cbd0f154b71c7ca3f04d32e77228b79ed5922396ab4b340d62
Deleted: sha256:1915ffba96ed51ac0a85ba201be2c77eada5cb587b225841b0ae2841cb4403eb
```
Worked for me. Is this something I should wait on before committing?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop pull request #335: TINKERPOP-1319: fix incorrect FeatureRequiremen...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/335#discussion_r67163346
--- Diff: gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/structure/FeatureSupportTest.java ---
@@ -587,7 +587,7 @@ public void shouldSupportRemoveEdgesIfEdgeCanBeRemoved() throws Exception {
public void shouldSupportRemovePropertyIfAPropertyCanBeRemoved() throws Exception {
try {
final Vertex v = graph.addVertex();
- final Edge e = v.addEdge("self", v);
+ final Edge e = v.addEdge("self", v, "name", "foo");
--- End diff --
In all your thinking on this PR, did you ever think of any way to "test our tests" for proper feature assignments? perhaps that's a bit advanced and impossible a thing to do, but anything else we could do to make this process of adding features less error prone? or do we just need to rely on good code reviews of tests to be sure features go in the right way?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #335: TINKERPOP-1319: fix incorrect FeatureRequirement annot...
Posted by pluradj <gi...@git.apache.org>.
Github user pluradj commented on the issue:
https://github.com/apache/tinkerpop/pull/335
Just confirmed that I can see the same problem occur intermittently on `tp31` without the fixes from this PR.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #335: TINKERPOP-1319: fix incorrect FeatureRequirement annot...
Posted by pluradj <gi...@git.apache.org>.
Github user pluradj commented on the issue:
https://github.com/apache/tinkerpop/pull/335
https://issues.apache.org/jira/browse/TINKERPOP-1360
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #335: TINKERPOP-1319: fix incorrect FeatureRequirement annot...
Posted by dkuppitz <gi...@git.apache.org>.
Github user dkuppitz commented on the issue:
https://github.com/apache/tinkerpop/pull/335
`docker/build.sh -t -i -n` fails for me in Spark Gremlin.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #335: TINKERPOP-1319: fix incorrect FeatureRequirement annot...
Posted by pluradj <gi...@git.apache.org>.
Github user pluradj commented on the issue:
https://github.com/apache/tinkerpop/pull/335
It appears intermittently for me after several consecutive attempts.
I didn't make any changes to that failing class, and looking and the test case itself, I'm not sure why any of the included fixes would affect this.
https://github.com/apache/tinkerpop/blob/TINKERPOP-1319/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/PropertiesTest.java#L103
Any ideas?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #335: TINKERPOP-1319: fix incorrect FeatureRequirement annot...
Posted by dkuppitz <gi...@git.apache.org>.
Github user dkuppitz commented on the issue:
https://github.com/apache/tinkerpop/pull/335
Hmm, that's odd. Maybe just another random error on my side. I'll try it one more time and let it run over night.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop pull request #335: TINKERPOP-1319: fix incorrect FeatureRequiremen...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/335#discussion_r67159933
--- Diff: gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/structure/FeatureSupportTest.java ---
@@ -587,7 +587,7 @@ public void shouldSupportRemoveEdgesIfEdgeCanBeRemoved() throws Exception {
public void shouldSupportRemovePropertyIfAPropertyCanBeRemoved() throws Exception {
try {
final Vertex v = graph.addVertex();
- final Edge e = v.addEdge("self", v);
+ final Edge e = v.addEdge("self", v, "name", "foo");
--- End diff --
good bug fix - i guess we haven't run afoul of that because most graphs will support property removal on edge.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #335: TINKERPOP-1319: fix incorrect FeatureRequirement annot...
Posted by pluradj <gi...@git.apache.org>.
Github user pluradj commented on the issue:
https://github.com/apache/tinkerpop/pull/335
I'll open a JIRA for that issue.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #335: TINKERPOP-1319: fix incorrect FeatureRequirement annot...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/335
VOTE +1 - pending update to upgrade docs
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop pull request #335: TINKERPOP-1319: fix incorrect FeatureRequiremen...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/335#discussion_r67158389
--- Diff: gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/structure/VertexTest.java ---
@@ -280,10 +280,9 @@ public void shouldHaveExceptionConsistencyWhenIdNotSupportedForAddEdge() throws
}
@Test
- @FeatureRequirementSet(FeatureRequirementSet.Package.VERTICES_ONLY)
- @FeatureRequirement(featureClass = VertexPropertyFeatures.class, feature = FEATURE_INTEGER_VALUES)
+ @FeatureRequirement(featureClass = Graph.Features.VertexFeatures.class, feature = Graph.Features.VertexFeatures.FEATURE_ADD_VERTICES)
public void shouldHaveStandardStringRepresentation() {
- final Vertex v = graph.addVertex("name", "marko", "age", 34);
+ final Vertex v = graph.addVertex();
--- End diff --
This might be changing the semantics of the test a little. We want to validate that a vertex looks a certain way with `toString()`. By adding properties, it introduces some data to the vertex object and allows the test to validate that none of that state shows up in the `toString()`. Maybe an off-chance of that happening I suppose. I guess the benefit is that we widen the test footprint for providers who don't support adding properties (but that's a really super basic thing). is there some other benefit? wdyt?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #335: TINKERPOP-1319: fix incorrect FeatureRequirement annot...
Posted by pluradj <gi...@git.apache.org>.
Github user pluradj commented on the issue:
https://github.com/apache/tinkerpop/pull/335
Trying the docker build now. Not sure why that would be failing with the changes I made.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #335: TINKERPOP-1319: fix incorrect FeatureRequirement annot...
Posted by pluradj <gi...@git.apache.org>.
Github user pluradj commented on the issue:
https://github.com/apache/tinkerpop/pull/335
I'll rerun it too
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #335: TINKERPOP-1319: fix incorrect FeatureRequirement annot...
Posted by dkuppitz <gi...@git.apache.org>.
Github user dkuppitz commented on the issue:
https://github.com/apache/tinkerpop/pull/335
Sorry, but it failed again with the same error. Is nobody else getting this?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #335: TINKERPOP-1319: fix incorrect FeatureRequirement annot...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/335
I would recommend adding something to the upgrade docs for graph providers, as this change will possibly open up some tests for folks and maybe introduce some breaks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop pull request #335: TINKERPOP-1319: fix incorrect FeatureRequiremen...
Posted by pluradj <gi...@git.apache.org>.
Github user pluradj commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/335#discussion_r67160532
--- Diff: gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/structure/VertexTest.java ---
@@ -280,10 +280,9 @@ public void shouldHaveExceptionConsistencyWhenIdNotSupportedForAddEdge() throws
}
@Test
- @FeatureRequirementSet(FeatureRequirementSet.Package.VERTICES_ONLY)
- @FeatureRequirement(featureClass = VertexPropertyFeatures.class, feature = FEATURE_INTEGER_VALUES)
+ @FeatureRequirement(featureClass = Graph.Features.VertexFeatures.class, feature = Graph.Features.VertexFeatures.FEATURE_ADD_VERTICES)
public void shouldHaveStandardStringRepresentation() {
- final Vertex v = graph.addVertex("name", "marko", "age", 34);
+ final Vertex v = graph.addVertex();
--- End diff --
Yeah, I thought about just adding the `FEATURE_INTEGER_VALUES` annotation, but it seemed to me like the test didn't have anything to do with properties. I can add another test to test with `toString()` with properties.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #335: TINKERPOP-1319: fix incorrect FeatureRequirement annot...
Posted by dkuppitz <gi...@git.apache.org>.
Github user dkuppitz commented on the issue:
https://github.com/apache/tinkerpop/pull/335
Alright, in that case...
VOTE: +1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---