You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tinkerpop.apache.org by "xiazcy (via GitHub)" <gi...@apache.org> on 2023/11/23 01:31:23 UTC
[PR] Fix flaky traversal test due to duplicated key generated for project step [tinkerpop]
xiazcy opened a new pull request, #2361:
URL: https://github.com/apache/tinkerpop/pull/2361
The random strings generated for method arguments can sometimes contain duplicate, which will fail the new requirement for project step to have unique keys only (introduced in https://github.com/apache/tinkerpop/pull/2217).
This PR adds processing to remove duplicates after the string args are generated.
(Note this is not needed for the current release).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix flaky traversal test due to duplicated key generated for project step [tinkerpop]
Posted by "xiazcy (via GitHub)" <gi...@apache.org>.
xiazcy merged PR #2361:
URL: https://github.com/apache/tinkerpop/pull/2361
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix flaky traversal test due to duplicated key generated for project step [tinkerpop]
Posted by "xiazcy (via GitHub)" <gi...@apache.org>.
xiazcy commented on code in PR #2361:
URL: https://github.com/apache/tinkerpop/pull/2361#discussion_r1403674590
##########
gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversalTest.java:
##########
@@ -174,8 +175,13 @@ else if (boolean.class.isAssignableFrom(type))
else if (String[].class.isAssignableFrom(type)) {
arguments[i] = new String[random.nextInt(10) + 1];
for (int j = 0; j < ((String[]) arguments[i]).length; j++) {
- list.add(((String[]) arguments[i])[j] = randomString(random));
+ list.add(((String[]) arguments[i])[j] = arguments[0] + randomString(random)); // adds the first string to all to avoid duplicates
}
+ // makes sure that no duplicated random string is created by removing duplicates
+ arguments[i] = Arrays.stream((String[]) arguments[i]).distinct().toArray(String[]::new);
+ final Set<Object> tempSet = new LinkedHashSet<>(list);
+ list.clear();
+ list.addAll(tempSet);
Review Comment:
updated in https://github.com/apache/tinkerpop/pull/2361/commits/701245111c6d72c762d3ef9635b0e2046fcf165e
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix flaky traversal test due to duplicated key generated for project step [tinkerpop]
Posted by "xiazcy (via GitHub)" <gi...@apache.org>.
xiazcy commented on code in PR #2361:
URL: https://github.com/apache/tinkerpop/pull/2361#discussion_r1403650779
##########
gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversalTest.java:
##########
@@ -174,8 +175,13 @@ else if (boolean.class.isAssignableFrom(type))
else if (String[].class.isAssignableFrom(type)) {
arguments[i] = new String[random.nextInt(10) + 1];
for (int j = 0; j < ((String[]) arguments[i]).length; j++) {
- list.add(((String[]) arguments[i])[j] = randomString(random));
+ list.add(((String[]) arguments[i])[j] = arguments[0] + randomString(random)); // adds the first string to all to avoid duplicates
}
+ // makes sure that no duplicated random string is created by removing duplicates
+ arguments[i] = Arrays.stream((String[]) arguments[i]).distinct().toArray(String[]::new);
+ final Set<Object> tempSet = new LinkedHashSet<>(list);
+ list.clear();
+ list.addAll(tempSet);
Review Comment:
There is actually two collections here, the argument array to be passed into the method and the list saved for the equality comparison part of the test. I didn't want to change the list adding part, but you are correct I could add them to a temp set first then add to the list. I'll update that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix flaky traversal test due to duplicated key generated for project step [tinkerpop]
Posted by "xiazcy (via GitHub)" <gi...@apache.org>.
xiazcy commented on PR #2361:
URL: https://github.com/apache/tinkerpop/pull/2361#issuecomment-1824806394
> Duplicates for String[] should probably be allowed (just make an exception for ProjectStep) so I would have preferred to do it that way but I think this approach is likely fine as well.
True, I'll special case project then.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix flaky traversal test due to duplicated key generated for project step [tinkerpop]
Posted by "kenhuuu (via GitHub)" <gi...@apache.org>.
kenhuuu commented on code in PR #2361:
URL: https://github.com/apache/tinkerpop/pull/2361#discussion_r1404661207
##########
gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversalTest.java:
##########
@@ -158,6 +159,16 @@ else if (stepMethod.getName().equals("to") || stepMethod.getName().equals("from"
}
} else if (stepMethod.getName().equals("math")) {
list.add(arguments[0] = random.nextInt(100) + " + " + random.nextInt(100));
+ } else if (stepMethod.getName().equals("project")) {
+ // project has two arguments [String, String[]]
+ list.add(arguments[0] = randomString(random));
+ arguments[1] = new String[random.nextInt(10) + 1];
+ for (int j = 0; j < ((String[]) arguments[1]).length; j++) {
+ ((String[]) arguments[1])[j] = arguments[0] + randomString(random); // adds argument[0] to avoid getting its duplicate in argument[1]
+ }
+ // remove duplicates in argument[1] if any
+ arguments[1] = Arrays.stream((String[]) arguments[1]).distinct().toArray(String[]::new);
+ list.addAll(Arrays.stream((String[]) arguments[1]).collect(Collectors.toList()));
Review Comment:
Might be able to do something similar to
```suggestion
list.addAll(Arrays.asList(argument[1]);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix flaky traversal test due to duplicated key generated for project step [tinkerpop]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #2361:
URL: https://github.com/apache/tinkerpop/pull/2361#issuecomment-1823728060
## [Codecov](https://app.codecov.io/gh/apache/tinkerpop/pull/2361?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Comparison is base [(`a9ff025`)](https://app.codecov.io/gh/apache/tinkerpop/commit/a9ff025c8319330bf51c5c7df562f7f2d7a260a0?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 75.20% compared to head [(`ffe673d`)](https://app.codecov.io/gh/apache/tinkerpop/pull/2361?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 71.23%.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## 3.6-dev #2361 +/- ##
=============================================
- Coverage 75.20% 71.23% -3.97%
=============================================
Files 1057 25 -1032
Lines 63450 3772 -59678
Branches 6935 0 -6935
=============================================
- Hits 47716 2687 -45029
+ Misses 13169 898 -12271
+ Partials 2565 187 -2378
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/tinkerpop/pull/2361?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix flaky traversal test due to duplicated key generated for project step [tinkerpop]
Posted by "kenhuuu (via GitHub)" <gi...@apache.org>.
kenhuuu commented on PR #2361:
URL: https://github.com/apache/tinkerpop/pull/2361#issuecomment-1824772984
Duplicates for String[] should probably be allowed (just make an exception for ProjectStep) so I would have preferred to do it that way but I think this approach is likely fine as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix flaky traversal test due to duplicated key generated for project step [tinkerpop]
Posted by "xiazcy (via GitHub)" <gi...@apache.org>.
xiazcy commented on code in PR #2361:
URL: https://github.com/apache/tinkerpop/pull/2361#discussion_r1403649505
##########
gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversalTest.java:
##########
@@ -174,8 +175,13 @@ else if (boolean.class.isAssignableFrom(type))
else if (String[].class.isAssignableFrom(type)) {
arguments[i] = new String[random.nextInt(10) + 1];
for (int j = 0; j < ((String[]) arguments[i]).length; j++) {
- list.add(((String[]) arguments[i])[j] = randomString(random));
+ list.add(((String[]) arguments[i])[j] = arguments[0] + randomString(random)); // adds the first string to all to avoid duplicates
Review Comment:
No, this array is strictly argument[1]. The way this is set up, the actual argument array is [`String`, `String[]`], where argument[0] is of type `String` and gets processed (turned into a random string) in the previous if case, and only argument[1] gets to this if case. As I think there is still a chance that whatever string was generated for argument[0] to be duplicated in the arrays of string generate for `String[]`, so the simplest way to avoid that is to just append argument[0] to all the string inside of argument[1].
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix flaky traversal test due to duplicated key generated for project step [tinkerpop]
Posted by "kenhuuu (via GitHub)" <gi...@apache.org>.
kenhuuu commented on PR #2361:
URL: https://github.com/apache/tinkerpop/pull/2361#issuecomment-1826113950
VOTE +1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix flaky traversal test due to duplicated key generated for project step [tinkerpop]
Posted by "Cole-Greer (via GitHub)" <gi...@apache.org>.
Cole-Greer commented on code in PR #2361:
URL: https://github.com/apache/tinkerpop/pull/2361#discussion_r1412666068
##########
gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversalTest.java:
##########
@@ -38,6 +38,7 @@
import java.util.Random;
import java.util.Set;
import java.util.function.Consumer;
+import java.util.stream.Collectors;
Review Comment:
This looks like an unused import
```suggestion
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix flaky traversal test due to duplicated key generated for project step [tinkerpop]
Posted by "Cole-Greer (via GitHub)" <gi...@apache.org>.
Cole-Greer commented on PR #2361:
URL: https://github.com/apache/tinkerpop/pull/2361#issuecomment-1836927180
VOTE +1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix flaky traversal test due to duplicated key generated for project step [tinkerpop]
Posted by "kenhuuu (via GitHub)" <gi...@apache.org>.
kenhuuu commented on code in PR #2361:
URL: https://github.com/apache/tinkerpop/pull/2361#discussion_r1403635304
##########
gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversalTest.java:
##########
@@ -174,8 +175,13 @@ else if (boolean.class.isAssignableFrom(type))
else if (String[].class.isAssignableFrom(type)) {
arguments[i] = new String[random.nextInt(10) + 1];
for (int j = 0; j < ((String[]) arguments[i]).length; j++) {
- list.add(((String[]) arguments[i])[j] = randomString(random));
+ list.add(((String[]) arguments[i])[j] = arguments[0] + randomString(random)); // adds the first string to all to avoid duplicates
}
+ // makes sure that no duplicated random string is created by removing duplicates
+ arguments[i] = Arrays.stream((String[]) arguments[i]).distinct().toArray(String[]::new);
+ final Set<Object> tempSet = new LinkedHashSet<>(list);
+ list.clear();
+ list.addAll(tempSet);
Review Comment:
This logic seems overly complex. The simplest way to do this would probably be to just generate the arguments and ensure they are unique. Then, add that unique array to list after.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix flaky traversal test due to duplicated key generated for project step [tinkerpop]
Posted by "kenhuuu (via GitHub)" <gi...@apache.org>.
kenhuuu commented on code in PR #2361:
URL: https://github.com/apache/tinkerpop/pull/2361#discussion_r1403630644
##########
gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversalTest.java:
##########
@@ -174,8 +175,13 @@ else if (boolean.class.isAssignableFrom(type))
else if (String[].class.isAssignableFrom(type)) {
arguments[i] = new String[random.nextInt(10) + 1];
for (int j = 0; j < ((String[]) arguments[i]).length; j++) {
- list.add(((String[]) arguments[i])[j] = randomString(random));
+ list.add(((String[]) arguments[i])[j] = arguments[0] + randomString(random)); // adds the first string to all to avoid duplicates
Review Comment:
It's not obvious to me how adding arguments[0] avoids duplicates. At first glance it seems to just make longer duplicates.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org