You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tinkerpop.apache.org by spmallette <gi...@git.apache.org> on 2016/09/16 20:22:37 UTC
[GitHub] tinkerpop pull request #426: TINKERPOP-1437 Added tests for dedup(Scope,Stri...
GitHub user spmallette opened a pull request:
https://github.com/apache/tinkerpop/pull/426
TINKERPOP-1437 Added tests for dedup(Scope,String...) overload in DedupTest
https://issues.apache.org/jira/browse/TINKERPOP-1437
Pretty simple - just added a couple of tests - but didn't CTR as I'd feel better with a couple of extra eyes on this as we've had bad tests slip into the suite in the past.
Runs nicely with `mvn clean install` - doing a `docker/build.sh -t -i -n` now. If anything bad happens I'll post back here.
VOTE +1
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/apache/tinkerpop TINKERPOP-1437
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/tinkerpop/pull/426.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 #426
----
commit 9666bb3dc2f593b66095b3e764678ccfc35630a5
Author: Stephen Mallette <sp...@genoprime.com>
Date: 2016-09-16T20:17:33Z
Added tests for dedup(Scope,String...) overload in DedupTest
----
---
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 #426: TINKERPOP-1437 Added tests for dedup(Scope,Stri...
Posted by okram <gi...@git.apache.org>.
Github user okram commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/426#discussion_r79258124
--- Diff: gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/DedupTest.java ---
@@ -241,6 +279,16 @@ public void g_V_both_both_dedup_byXoutE_countX_name() {
public static class Traversals extends DedupTest {
@Override
+ public Traversal<Vertex, String> get_g_V_out_in_valuesXnameX_fold_dedupXlocalX_unfold() {
+ return g.V().out().in().values("name").fold().dedup(Scope.local).unfold();
+ }
+
+ @Override
+ public Traversal<Vertex, Map<String, String>> get_g_V_out_in_asXxX_in_asXyX_selectXx_yX_byXnameX_fold_dedupXlocal_x_yX_unfold() {
--- End diff --
You named the method wrong. Should be `get_g_V_out_asXxX_in_asXyX.....`
---
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 #426: TINKERPOP-1437 Added tests for dedup(Scope,Stri...
Posted by okram <gi...@git.apache.org>.
Github user okram commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/426#discussion_r79258468
--- Diff: gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/DedupTest.java ---
@@ -73,6 +77,40 @@
public abstract Traversal<Vertex, String> get_g_V_both_both_dedup_byXoutE_countX_name();
+ public abstract Traversal<Vertex, String> get_g_V_out_in_valuesXnameX_fold_dedupXlocalX_unfold();
+
+ public abstract Traversal<Vertex, Map<String, String>> get_g_V_out_in_asXxX_in_asXyX_selectXx_yX_byXnameX_fold_dedupXlocal_x_yX_unfold();
+
+ @Test
+ @LoadGraphWith(MODERN)
+ public void g_V_out_in_valuesXnameX_fold_dedupXlocalX_unfold() {
+ final Traversal<Vertex, String> traversal = get_g_V_out_in_valuesXnameX_fold_dedupXlocalX_unfold();
+ final List<String> names = traversal.toList();
+ assertEquals(3, names.size());
+ assertThat(names, containsInAnyOrder("marko", "josh", "peter"));
--- End diff --
Note we have `checkResults()`... e.g.
```
@Test
@LoadGraphWith(MODERN)
public void g_V_whereXnotXoutXcreatedXXX_name() {
final Traversal<Vertex, String> traversal = get_g_V_whereXnotXoutXcreatedXXX_name();
printTraversalForm(traversal);
checkResults(Arrays.asList("vadas", "lop", "ripple"), traversal);
}
```
---
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 #426: TINKERPOP-1437 Added tests for dedup(Scope,String...) ...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/426
I typically prefer JUnit for all simple assertions except for `assertTrue` and `assertFalse`. Those two really stink compared to hamcrest and `assertThat(something(), is(true))` or `assertThat(something(), is(false))` because they print better output than the former that makes it easier to immediately see the problem with the assertion on failure. I convert those `assertTrue` and `assertFalse` as I see opportunity to do so. I also use hamcrest when the assertion logic is complex and there is a better simple one liner operation with hamcrest that makes the assertion more readable with a cleaner output message than junit. So, no, I don't think we have to prefer Hamcrest to Junit across the board, but we should consider it when it produces a more readable output or more readable code.
---
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 #426: TINKERPOP-1437 Added tests for dedup(Scope,String...) ...
Posted by okram <gi...@git.apache.org>.
Github user okram commented on the issue:
https://github.com/apache/tinkerpop/pull/426
Made a few comments to ensure your test code is consistent with the others. Besides minor cosmetics -- 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 issue #426: TINKERPOP-1437 Added tests for dedup(Scope,String...) ...
Posted by dkuppitz <gi...@git.apache.org>.
Github user dkuppitz commented on the issue:
https://github.com/apache/tinkerpop/pull/426
VOTE: +1
Question: For future tests, should we always prefer Hamcrest over JUnit?
---
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 #426: TINKERPOP-1437 Added tests for dedup(Scope,Stri...
Posted by okram <gi...@git.apache.org>.
Github user okram commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/426#discussion_r79258254
--- Diff: gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/DedupTest.java ---
@@ -73,6 +77,40 @@
public abstract Traversal<Vertex, String> get_g_V_both_both_dedup_byXoutE_countX_name();
+ public abstract Traversal<Vertex, String> get_g_V_out_in_valuesXnameX_fold_dedupXlocalX_unfold();
+
+ public abstract Traversal<Vertex, Map<String, String>> get_g_V_out_in_asXxX_in_asXyX_selectXx_yX_byXnameX_fold_dedupXlocal_x_yX_unfold();
+
+ @Test
+ @LoadGraphWith(MODERN)
+ public void g_V_out_in_valuesXnameX_fold_dedupXlocalX_unfold() {
+ final Traversal<Vertex, String> traversal = get_g_V_out_in_valuesXnameX_fold_dedupXlocalX_unfold();
+ final List<String> names = traversal.toList();
+ assertEquals(3, names.size());
+ assertThat(names, containsInAnyOrder("marko", "josh", "peter"));
+ }
+
+ @Test
+ @LoadGraphWith(MODERN)
+ public void g_V_out_in_asXxX_in_asXyX_selectXx_yX_byXnameX_fold_dedupXlocal_x_yX_unfold() {
+ final Traversal<Vertex, Map<String, String>> traversal = get_g_V_out_in_asXxX_in_asXyX_selectXx_yX_byXnameX_fold_dedupXlocal_x_yX_unfold();
+ final List<Map<String,String>> names = traversal.toList();
+ assertEquals(6, names.size());
--- End diff --
You can use the `checkResult` and `MapMaker` to make this cleaner if you like. Check `WhereTest` for examples:
```
@Test
@LoadGraphWith(MODERN)
public void g_V_asXaX_outEXcreatedX_asXbX_inV_asXcX_inXcreatedX_asXdX_whereXa_ltXbX_orXgtXcXX_andXneqXdXXX_byXageX_byXweightX_byXinXcreatedX_valuesXageX_minX_selectXa_c_dX() {
final Traversal<Vertex, Map<String, String>> traversal = get_g_V_asXaX_outEXcreatedX_asXbX_inV_asXcX_inXcreatedX_asXdX_whereXa_ltXbX_orXgtXcXX_andXneqXdXXX_byXageX_byXweightX_byXinXcreatedX_valuesXageX_minX_selectXa_c_dX();
printTraversalForm(traversal);
checkResults(makeMapList(3,
"a", "peter", "c", "lop", "d", "josh",
"a", "peter", "c", "lop", "d", "marko",
"a", "josh", "c", "lop", "d", "marko",
"a", "josh", "c", "lop", "d", "peter"), traversal);
}
```
---
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 #426: TINKERPOP-1437 Added tests for dedup(Scope,Stri...
Posted by okram <gi...@git.apache.org>.
Github user okram commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/426#discussion_r79258319
--- Diff: gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/DedupTest.java ---
@@ -73,6 +77,40 @@
public abstract Traversal<Vertex, String> get_g_V_both_both_dedup_byXoutE_countX_name();
+ public abstract Traversal<Vertex, String> get_g_V_out_in_valuesXnameX_fold_dedupXlocalX_unfold();
+
+ public abstract Traversal<Vertex, Map<String, String>> get_g_V_out_in_asXxX_in_asXyX_selectXx_yX_byXnameX_fold_dedupXlocal_x_yX_unfold();
+
+ @Test
+ @LoadGraphWith(MODERN)
+ public void g_V_out_in_valuesXnameX_fold_dedupXlocalX_unfold() {
+ final Traversal<Vertex, String> traversal = get_g_V_out_in_valuesXnameX_fold_dedupXlocalX_unfold();
+ final List<String> names = traversal.toList();
+ assertEquals(3, names.size());
+ assertThat(names, containsInAnyOrder("marko", "josh", "peter"));
+ }
+
+ @Test
+ @LoadGraphWith(MODERN)
+ public void g_V_out_in_asXxX_in_asXyX_selectXx_yX_byXnameX_fold_dedupXlocal_x_yX_unfold() {
--- End diff --
This has a bad method name --- see my other comment and then fix it here 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.
---