Add test cases for concurrent reductions #150
Conversation
khatchad
left a comment
khatchad
left a comment
There was a problem hiding this comment.
You are missing the column we discussed from Table 3.
There was a problem hiding this comment.
Can we be more specific here? Concurrent what? Likewise for non-concurrent.
There was a problem hiding this comment.
Do I make more specific for CONVERT_TO_SEQUENTIAL? I thought CONVERT_TO_SEQUENTIAL and CONVERT_TO_CONCURRENT have the same format.
There was a problem hiding this comment.
No, you can leave those. Let's make the new code specific.
There was a problem hiding this comment.
CONVERT_COLLECTOR_TO_CONCURRENT? Is that what you mean?
There was a problem hiding this comment.
Please use EnumSet.of(). It should be more efficient.
There was a problem hiding this comment.
Other test cases also accept Collections.singleton for execution mode and ordering. Should I change them all?
There was a problem hiding this comment.
No. Please focus on the new code.
Also test |
There was a problem hiding this comment.
CONVERT_COLLECTOR_TO_CONCURRENT? Is that what you mean?
Yes. |
|
Please go with that.
On Fri, 2018-01-05 at 02:18 +0000, Grace Tang wrote:
CONVERT_COLLECTOR_TO_CONCURRENT? Is that what you mean?
Yes.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub<#150 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AB9DPz4XO1XIxEDxbgy3A9Jf3F6zun4sks5tHYZbgaJpZM4RTUzA>.
|
I think the only column I am missing is |
khatchad
left a comment
khatchad
left a comment
There was a problem hiding this comment.
This is poor work overall. You must learn how to reuse existing code.
There was a problem hiding this comment.
No. There should be a field returned here.
There was a problem hiding this comment.
No way. Way too much duplication. Please explain the reason you need an entire separate class for this.
There was a problem hiding this comment.
Absolutely not. No copy and pasting.
|
Actually you do not need to overload since we are passing a single object. You can just add a field to the expected result class.
|
khatchad
left a comment
khatchad
left a comment
There was a problem hiding this comment.
Better but there's still to much duplication.
There was a problem hiding this comment.
Remove the null. It's null be default.
There was a problem hiding this comment.
Still too much duplication here.
There was a problem hiding this comment.
I've changed constructor.
|
Thank you! |
* add test cases and fail the stream * calculate LOC * Revert "calculate LOC" This reverts commit 034b677. * revert changing functional code * change test cases * change test * change test cases * change refactoring * delete useless importation * Remove extra space. * change name and collections * Add collector kind * remove duplication * improve format * remove overloading helper * format * remove duplication * Remove whitespace addition.
* add test cases and fail the stream * calculate LOC * Revert "calculate LOC" This reverts commit 034b677. * revert changing functional code * change test cases * change test * change test cases * change refactoring * delete useless importation * Remove extra space. * change name and collections * Add collector kind * remove duplication * improve format * remove overloading helper * format * remove duplication * Remove whitespace addition.
2 participants
Fix #138.
The old pull request: #143