Conversation
|
@jerryjiangabc |
guimingTang
left a comment
guimingTang
left a comment
There was a problem hiding this comment.
Thanks for contributing to the test cases! AndFilterTest already exists under com.twitter.graphjet.algorithms.filter. Could you merge the new test cases with the existing tests?
|
@guimingTang |
| } | ||
|
|
||
| @Test | ||
| public void testFilterResultReturningFalseOne() { |
There was a problem hiding this comment.
This test is almost equivalent to testNoFilter() where the socialProof array and resultNode values are changed. Can we reuse the existing setup and merge them under the same test function?
| } | ||
|
|
||
| @Test | ||
| public void testFilterResultReturningFalseTwo() { |
There was a problem hiding this comment.
s/testFilterResultReturningFalseTwo()/testWithRecentTweetFilter()
There was a problem hiding this comment.
Renamed method.
I consider your comment requesting to change the name of the method.
|
|
||
| @Test | ||
| public void testFilterResultReturningFalseTwo() { | ||
| List<ResultFilter> linkedList = new LinkedList<ResultFilter>(); |
There was a problem hiding this comment.
Variables should generally be named by its designed purpose, i.e. in this case the var linkedList should be called something like "filters"
| StatsReceiver statsReceiver = new NullStatsReceiver(); | ||
| RecentTweetFilter recentTweetFilter = new RecentTweetFilter(2147483639L, statsReceiver); | ||
| linkedList.add(recentTweetFilter); | ||
| ANDFilters aNDFilters = new ANDFilters(linkedList, statsReceiver); |
There was a problem hiding this comment.
s/aNDFilters/andFilters
| RecentTweetFilter recentTweetFilter = new RecentTweetFilter(2147483639L, statsReceiver); | ||
| linkedList.add(recentTweetFilter); | ||
| ANDFilters aNDFilters = new ANDFilters(linkedList, statsReceiver); | ||
| SmallArrayBasedLongToDoubleMap[] smallArrayBasedLongToDoubleMapArray = new SmallArrayBasedLongToDoubleMap[5]; |
There was a problem hiding this comment.
s/smallArrayBasedLongToDoubleMapArray/socialProofs
| * @see SalsaRequestBuilder | ||
| **/ | ||
| public class SalsaRequestBuilderTest { | ||
| @Test |
There was a problem hiding this comment.
The GraphJet codebase adopts a 2-space convention. Could you trim the spacing in this file?
|
@guimingTang |
|
Obviously, accidentally closed this pull request after approving but before merging. |
|
|
3 participants
Added new Unit Tests to increase code coverage.