[ 
https://issues.apache.org/jira/browse/MRUNIT-91?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13240094#comment-13240094
 ] 

Jim Donofrio commented on MRUNIT-91:
------------------------------------

Math.abs is unnecessary on line 272, actualPositionList.size() must be > 
expectedPositionList.size()

Why in 2 places do you say Received unexpected output without the position but 
in one place you give the position, I would say you should give the position, 
the more info the better. If you add this you should check if orderMatters at 
the end of the last for loop where you only output unexpected output with the 
remainer of the actualPositions. I think you could then abstract the logic in 
that last for loop where you check for unexpected output and the logic in the 
second to last for loop where you check for unexpected expected output into one 
method that you pass the relevant lists in along with the string Missing 
expected output vs Received unexpected output. This will also help to reduce 
the large amount of code in this single method.

I think for (int i = 0; ; i++) { would be cleaner as a while loop:
while (expectedPositionList.size() > i || actualPositionList.size() > i) {
  if (expectedPositionList.size() > i && actualPositionList.size() > i) {

  } else if (expectedPositionList.size() > i) {
  
  } else {

  }
}

Otherwise maybe adding some more comments would be useful given how complicated 
this method is but overall a great contribution.
                
> runTest() should optionally ignore output order
> -----------------------------------------------
>
>                 Key: MRUNIT-91
>                 URL: https://issues.apache.org/jira/browse/MRUNIT-91
>             Project: MRUnit
>          Issue Type: Improvement
>    Affects Versions: 0.8.1
>            Reporter: William McNeill
>            Priority: Minor
>              Labels: order
>             Fix For: 0.9.0
>
>         Attachments: MRUNIT-91-1.patch, MRUNIT-91-2.patch, MRUNIT-91.patch
>
>
> Currently MapDriver.runTest() assumes that the order of pairs emitted by the 
> mapper matches the order of the MapDriver.addOutput() calls. However, there 
> are valid mappers that for a given input pair produce output pairs whose 
> order is unspecified for testing purposes. (For example, if the mapper being 
> tested uses a set object for deduplication before emission.) runTest() cannot 
> be used to test these kinds of mappers.
> A workaround is to not use runTest() but instead put the output of run() into 
> a Set and assert that the contents of the set are correct, bypassing MRUnit's 
> validation code.
> A possible improvement would be to add a boolean orderMatters parameter to 
> MapDriver.runTest(), invoking an order-insensitive version of 
> TestDriver.validate() when orderMatters is false and the existing version 
> otherwise.
> For clarity's sake only mappers are discussed in this feature request, but 
> the same applies to reducers as well.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: 
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira

        

Reply via email to