-
Notifications
You must be signed in to change notification settings - Fork 7
Add test cases for concurrent reductions #150
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are missing the column we discussed from Table 3.
CONVERT_TO_SEQUENTIAL | ||
|
||
CONVERT_TO_SEQUENTIAL, | ||
CONVERT_TO_CONCURRENT, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we be more specific here? Concurrent what? Likewise for non-concurrent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, you can leave those. Let's make the new code specific.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CONVERT_COLLECTOR_TO_CONCURRENT
? Is that what you mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please go with that
*/ | ||
public void testConcurrentReduction() throws Exception { | ||
helper(new StreamAnalysisExpectedResult("orderedWidgets.stream()", | ||
Collections.singleton(ExecutionMode.SEQUENTIAL), Collections.singleton(Ordering.ORDERED), false, false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use EnumSet.of()
. It should be more efficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other test cases also accept Collections.singleton
for execution mode and ordering. Should I change them all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. Please focus on the new code.
Also test |
CONVERT_TO_SEQUENTIAL | ||
|
||
CONVERT_TO_SEQUENTIAL, | ||
CONVERT_TO_CONCURRENT, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is poor work overall. You must learn how to reuse existing code.
@@ -634,6 +636,10 @@ public PreconditionSuccess getPassingPrecondition() { | |||
return possibleOrderings.stream().map(e -> e == null ? initialOrdering : e).collect(Collectors.toSet()); | |||
} | |||
|
|||
public CollectorKind getCollectorKind() { | |||
return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. There should be a field returned here.
import edu.cuny.hunter.streamrefactoring.core.analysis.Refactoring; | ||
import edu.cuny.hunter.streamrefactoring.core.analysis.TransformationAction; | ||
|
||
public class ComplexStreamAnalysisExpectedResult { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No way. Way too much duplication. Please explain the reason you need an entire separate class for this.
* Runs a single analysis test. | ||
* Test for #138. Test complex mutable reduction. | ||
*/ | ||
private void helper(ComplexStreamAnalysisExpectedResult... expectedResults) throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better but there's still to much duplication.
@@ -167,6 +167,8 @@ private static String getMethodIdentifier(IMethodBinding methodBinding) { | |||
private final MethodDeclaration enclosingMethodDeclaration; | |||
|
|||
private final TypeDeclaration enclosingTypeDeclaration; | |||
|
|||
private CollectorKind collectorKind = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the null
. It's null be default.
/** | ||
* Overloading constructor to test complex mutable reduction | ||
*/ | ||
public StreamAnalysisExpectedResult(String expectedCreation, Set<ExecutionMode> expectedExecutionModes, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still too much duplication here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you.
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.
Fix #138.
The old pull request: #143