STORM-1290: port backtype.storm.local-state-test to java #1949
Conversation
changes done STORM-1292: port backtype.storm.messaging-test to java
| ls2.put("b",globalStreamId_d); | ||
| Assert.assertEquals(globalStreamId_d, ls2.get("b")); | ||
| } | ||
|
|
There was a problem hiding this comment.
I think testEmptyState is redundant. All these features are already tested in above testLocalState.
There was a problem hiding this comment.
I also guess so. You can remove it.
|
Could you create a new branch based on current master and work from there, and submit PR? |
HeartSaVioR
left a comment
HeartSaVioR
left a comment
There was a problem hiding this comment.
I've reviewed for the first pass and left comments.
|
|
||
| @Test | ||
| public void testLocalState() throws IOException{ | ||
| TmpPath dir1_tmp = new TmpPath(); |
There was a problem hiding this comment.
Let's use try-with-resource to make it compatible with with-open in Clojure. It should cover dir1_tmp and dir2_tmp.
|
|
||
| ls1.put("a",globalStreamId_a); | ||
| ls1.put("b",globalStreamId_b); | ||
| Assert.assertTrue(ls1.snapshot().containsKey("a")); |
There was a problem hiding this comment.
Below comparisons are broken: {"a" globalStreamId_b "b" globalStreamId_a} will pass.
I guess you can compare two Maps directly since Map.equals defines how implementation should compare each other properly.
https://docs.oracle.com/javase/7/docs/api/java/util/Map.html#equals(java.lang.Object)
| Assert.assertTrue(ls1.snapshot().containsValue(globalStreamId_a)); | ||
| Assert.assertTrue(ls1.snapshot().containsKey("b")); | ||
| Assert.assertTrue(ls1.snapshot().containsValue(globalStreamId_b)); | ||
|
|
There was a problem hiding this comment.
You seemed to miss below lines:
- (is (= {} (.snapshot ls2)))
- (is (= gs-a (.get ls1 "a")))
- (is (= nil (.get ls1 "c")))
- (is (= gs-b (.get ls1 "b")))
- (is (= {"a" gs-a "b" gs-b} (.snapshot (LocalState. dir1))))
| ls2.put("b",globalStreamId_d); | ||
| Assert.assertEquals(globalStreamId_d, ls2.get("b")); | ||
| } | ||
|
|
There was a problem hiding this comment.
I also guess so. You can remove it.
|
@HeartSaVioR ,my apology for not responding to this early as some urgent travel came up for me. Thanks for reviewing the PR. As you suggested I'll apply the review comments and submit a separate PR. |
|
@kamleshbhatt Any updates? |
|
My sincere apology for the delay on this. As you suggested I am closing this PR and submit the revised one soon. Thanks for all your help. |
2 participants
No description provided.