SQOOP-1935: Sqoop2: Fix TestSqoopWritable test and make getString and setString package private

(Veena Basavaraj via Abraham Elmahrek)

execution/mapreduce/src/main/java/org/apache/sqoop/job/io/SqoopWritable.java

@@ -31,12 +31,16 @@
 
 /**
  * Writable used to load the data to the {@link #Transferable} entity TO
+ * It is used for the map output key class and the outputFormat key class in the MR engine
+ * For instance: job.setMapOutputKeyClass(..,) and  job.setOutputKeyClass(...);
  */
 
 public class SqoopWritable implements Configurable, WritableComparable<SqoopWritable> {
   private IntermediateDataFormat<?> toIDF;
   private Configuration conf;
 
+  // NOTE: You have to provide an empty default constructor in your key class
+  // Hadoop is using reflection and it can not guess any parameters to feed
   public SqoopWritable() {
     this(null);
   }
@@ -45,14 +49,11 @@ public SqoopWritable(IntermediateDataFormat<?> dataFormat) {
     this.toIDF = dataFormat;
   }
 
-  public void setString(String data) {
+  // default/package visibility for testing
+  void setString(String data) {
     this.toIDF.setCSVTextData(data);
   }
 
-  public String getString() {
-    return toIDF.getCSVTextData();
-  }
-
   @Override
   public void write(DataOutput out) throws IOException {
     //delegate
@@ -67,12 +68,12 @@ public void readFields(DataInput in) throws IOException {
 
   @Override
   public int compareTo(SqoopWritable o) {
-    return getString().compareTo(o.getString());
+    return toString().compareTo(o.toString());
   }
 
   @Override
   public String toString() {
-    return getString();
+    return toIDF.getCSVTextData();
   }
 
   @Override

execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopOutputFormatLoadExecutor.java

@@ -102,7 +102,7 @@ public void write(SqoopWritable key, NullWritable value) throws InterruptedExcep
       free.acquire();
       checkIfConsumerThrew();
       // NOTE: this is the place where data written from SqoopMapper writable is available to the SqoopOutputFormat
-      toDataFormat.setCSVTextData(key.getString());
+      toDataFormat.setCSVTextData(key.toString());
       filled.release();
     }
 

execution/mapreduce/src/test/java/org/apache/sqoop/job/io/TestSqoopWritable.java

@@ -18,6 +18,11 @@
  */
 package org.apache.sqoop.job.io;
 
+import static org.junit.Assert.*;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+
 import java.io.ByteArrayInputStream;
 import java.io.ByteArrayOutputStream;
 import java.io.DataInput;
@@ -28,61 +33,72 @@
 import java.io.InputStream;
 
 import org.apache.sqoop.connector.idf.CSVIntermediateDataFormat;
-import org.junit.Assert;
+import org.apache.sqoop.connector.idf.IntermediateDataFormat;
+import org.junit.Before;
 import org.junit.Test;
 
 public class TestSqoopWritable {
 
-  private final SqoopWritable writable = new SqoopWritable(new CSVIntermediateDataFormat());
+  private SqoopWritable writable;
+  private IntermediateDataFormat<?> idfMock;
+
+  @Before
+  public void setUp() {
+    idfMock = mock(IntermediateDataFormat.class);
+    writable = new SqoopWritable(idfMock);
+  }
 
   @Test
   public void testStringInStringOut() {
     String testData = "Live Long and prosper";
     writable.setString(testData);
-    Assert.assertEquals(testData,writable.getString());
+    verify(idfMock, times(1)).setCSVTextData(testData);
+    writable.toString();
+    verify(idfMock, times(1)).getCSVTextData();
   }
 
   @Test
-  public void testDataWritten() throws IOException {
+  public void testWrite() throws IOException {
     String testData = "One ring to rule them all";
-    writable.setString(testData);
     ByteArrayOutputStream ostream = new ByteArrayOutputStream();
+    ostream.write(testData.getBytes());
     DataOutput out = new DataOutputStream(ostream);
     writable.write(out);
-    byte[] written = ostream.toByteArray();
+    // verify that the idf method is called, that is all the test should test
-    InputStream instream = new ByteArrayInputStream(written);
+    verify(idfMock, times(1)).write(out);
-    DataInput in = new DataInputStream(instream);
+    ostream.close();
-    String readData = in.readUTF();
-    Assert.assertEquals(testData, readData);
   }
 
   @Test
-  public void testDataRead() throws IOException {
+  public void testReadFields() throws IOException {
     String testData = "Brandywine Bridge - 20 miles!";
-    ByteArrayOutputStream ostream = new ByteArrayOutputStream();
+    InputStream instream = new ByteArrayInputStream(testData.getBytes());
-    DataOutput out = new DataOutputStream(ostream);
-    out.writeUTF(testData);
-    InputStream instream = new ByteArrayInputStream(ostream.toByteArray());
     DataInput in = new DataInputStream(instream);
     writable.readFields(in);
-    Assert.assertEquals(testData, writable.getString());
+    // verify that the idf method is called, that is all the test should test
+    verify(idfMock, times(1)).read(in);
+    instream.close();
   }
 
+  // NOTE: This test is testing that the write and readFields methods work
+  // and not really testing anything about SqoopWritable. Have kept this test since
+  // it existed before.
   @Test
-  public void testWriteReadUsingStream() throws IOException {
+  public void testWriteAndReadFields() throws IOException {
     String testData = "You shall not pass";
     ByteArrayOutputStream ostream = new ByteArrayOutputStream();
     DataOutput out = new DataOutputStream(ostream);
-    writable.setString(testData);
+    SqoopWritable writableOne = new SqoopWritable(new CSVIntermediateDataFormat());
-    writable.write(out);
+    writableOne.setString(testData);
+    writableOne.write(out);
     byte[] written = ostream.toByteArray();
 
-    //Don't test what the data is, test that SqoopWritable can read it.
+    // Don't test what the data is, test that SqoopWritable can read it.
     InputStream instream = new ByteArrayInputStream(written);
-    SqoopWritable newWritable = new SqoopWritable(new CSVIntermediateDataFormat());
+    SqoopWritable writableTwo = new SqoopWritable(new CSVIntermediateDataFormat());
     DataInput in = new DataInputStream(instream);
-    newWritable.readFields(in);
+    writableTwo.readFields(in);
-    Assert.assertEquals(testData, newWritable.getString());
+    assertEquals(writableOne.toString(), writableTwo.toString());
     ostream.close();
     instream.close();
   }

execution/mapreduce/src/test/java/org/apache/sqoop/job/util/MRJobTestUtil.java

@@ -44,16 +44,16 @@ public class MRJobTestUtil {
 
   @SuppressWarnings("deprecation")
   public static boolean runJob(Configuration conf,
-      Class<? extends InputFormat<SqoopSplit, NullWritable>> input,
+      Class<? extends InputFormat<SqoopSplit, NullWritable>> inputFormatClass,
-      Class<? extends Mapper<SqoopSplit, NullWritable, SqoopWritable, NullWritable>> mapper,
+      Class<? extends Mapper<SqoopSplit, NullWritable, SqoopWritable, NullWritable>> mapperClass,
-      Class<? extends OutputFormat<SqoopWritable, NullWritable>> output) throws IOException,
+      Class<? extends OutputFormat<SqoopWritable, NullWritable>> outputFormatClass) throws IOException,
       InterruptedException, ClassNotFoundException {
     Job job = new Job(conf);
-    job.setInputFormatClass(input);
+    job.setInputFormatClass(inputFormatClass);
-    job.setMapperClass(mapper);
+    job.setMapperClass(mapperClass);
     job.setMapOutputKeyClass(SqoopWritable.class);
     job.setMapOutputValueClass(NullWritable.class);
-    job.setOutputFormatClass(output);
+    job.setOutputFormatClass(outputFormatClass);
     job.setOutputKeyClass(SqoopWritable.class);
     job.setOutputValueClass(NullWritable.class);
 
@@ -62,7 +62,7 @@ public static boolean runJob(Configuration conf,
     // Hadoop 1.0 (and 0.20) have nasty bug when job committer is not called in
     // LocalJobRuner
     if (isHadoop1()) {
-      callOutputCommitter(job, output);
+      callOutputCommitter(job, outputFormatClass);
     }
 
     return ret;