Added tests for CSVInOut util #42

ajayborra · 2018-08-08T16:18:23Z

Describe the proposed solution

Added tests for CSVInOut class to improve coverage.

tovbinm

so far so good, please see comments.

tovbinm · 2018-08-08T17:33:11Z

utils/src/test/scala/com/salesforce/op/utils/io/csv/CSVInOutTest.scala

+ private val csvReader = new CSVInOut(CSVOptions())
+ private val f1 = resourceFile(name = "test_sample.csv")
+
+ "CSVInOut" should "throw AnalysisException for bad file paths" in {


use Spec for naming the tests

dont mention the error in the test name, cause it might change:
I.e.

Spec[CSVInOut] should "throw an error for bad file paths" in { ... }

tovbinm · 2018-08-08T17:40:36Z

utils/src/test/scala/com/salesforce/op/utils/io/csv/CSVInOutTest.scala

+ var error = intercept[AnalysisException] {
+ csvReader.readDataFrame("/bad/file/path/read/dataframe")
+ }
+ error.getMessage().contains("Path does not exist: file:/bad/file/path/read/dataframe") shouldBe true


avoid using var-s, unless absolutely mandatory.

please use a more specific should matcher, cause this would make a test failure message nicer:

val error = intercept[AnalysisException](csvReader.readDataFrame("/bad/file/path/read/dataframe")) error.getMessage should endWith "Path does not exist: file:/bad/file/path/read/dataframe"

tovbinm · 2018-08-08T17:43:35Z

utils/src/test/scala/com/salesforce/op/utils/io/csv/CSVInOutTest.scala

+ error.getMessage().contains("Path does not exist: file:/bad/file/path/read/rdd") shouldBe true
+ }
+
+ it should "read a CSV file to DataFrame/RDD" in {


these two should be two separate test cases:

it should "read a CSV file to DataFrame" in { val res = csvReader.readDataFrame(f1.getAbsolutePath) res shouldBe a[DataFrame] res.count shouldBe 2 } it should "read a CSV file to RDD" in { val res = csvReader.readRDD(f1.getAbsolutePath) res shouldBe a[RDD[Seq[String]]] res.count shouldBe 2 }

tovbinm · 2018-08-08T17:45:05Z

utils/src/test/resources/test_sample.csv

@@ -0,0 +1,2 @@
+a,b,c,d


Please also add a bit larger csv file as well. and a one with headers. and perhaps some special characters.

codecov · 2018-08-08T18:14:05Z

Codecov Report

Merging #42 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           master      #42    +/-   ##
========================================
  Coverage   83.98%   83.98%            
========================================
  Files         296      296            
  Lines        9679     9679            
  Branches      351      544   +193     
========================================
  Hits         8129     8129            
  Misses       1550     1550

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8aa559c...b210251. Read the comment docs.

ajayborra · 2018-08-08T18:29:41Z

Made the requested changes, Code looks much cleaner now 👍

tovbinm · 2018-08-08T18:35:49Z

utils/src/test/resources/test_sample.csv

@@ -0,0 +1,101 @@
+Id,Description,Name,Item0,Item1,Item2,Item3,Vendor,Category,Probability


what's he source of this dataset? maybe pick a simple csv file from here - https://github.com/awesomedata/awesome-public-datasets

Was using the data from this source https://www.sample-videos.com/download-sample-csv.php.
Will pick a simpler one from the above source.

Great. Thank you. We need to make sure its a public dataset with no copyright restrictions.

Planning to add a scaled-down version of this dataset https://www.kaggle.com/grubenm/austin-weather, Its license is GPL v2. Sounds good?

Let me check. I will let you know.

Let's just use an existing csv file we already have. You can load it like this in your test:

def testDataPath: String = { Some(new File("test-data")) filter (_.isDirectory) getOrElse new File("../test-data") getPath } val csvFile: String = s"$testDataPath/PassengerDataAllWithHeader.csv"

Observation:

Some(new File("test-data")) filter (_.isDirectory) getOrElse new File("../test-data") getPath

would fail if we create a test-data dir under utils in future. Symlinking ../test-data into utils directory can solve the issue. But, Symlinking comes with its own cross platform issues like windows file systems doesn't support linking etc. We can ignore this details for now i guess.

Good point. Not an issue right now though.

tovbinm · 2018-08-08T18:36:22Z

utils/src/test/scala/com/salesforce/op/utils/io/csv/CSVInOutTest.scala

+
+ it should "read a CSV file to RDD" in {
+ val res = csvReader.readRDD(f1.getAbsolutePath)
+ res shouldBe a[RDD[Seq[String]]]


redundant space

Do you know if there is a good plugin for auto-linting/formatting for IntelliJ?

Intellij support code formating out of the box. You can import the Intellij settings xml from gradle/intellij-codestyle-config.xml file

tovbinm

Lgtm

Added tests for CSVInOut Class

1c41383

ajayborra requested review from leahmcguire and tovbinm as code owners August 8, 2018 16:18

salesforce-cla bot added the cla:signed label Aug 8, 2018

ajayborra changed the title ~~Added path validation & Tests for CSVInOut util~~ Added tests for CSVInOut util Aug 8, 2018

tovbinm reviewed Aug 8, 2018

View reviewed changes

Cleaned up code based on review comments

5e8ad48

tovbinm reviewed Aug 8, 2018

View reviewed changes

tovbinm and others added 3 commits August 8, 2018 18:29

Merge branch 'master' into coverage/improve_coverage_for_csv_util

7e78df3

Replaced usage of local csv test resource with already existing csv

230347b

Merge branch 'master' into coverage/improve_coverage_for_csv_util

b210251

tovbinm approved these changes Aug 9, 2018

View reviewed changes

tovbinm merged commit c2086e5 into salesforce:master Aug 9, 2018

ajayborra deleted the coverage/improve_coverage_for_csv_util branch August 9, 2018 14:26

ericwayman pushed a commit that referenced this pull request Feb 8, 2019

Added tests for CSVInOut util (#42)

d6d822c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added tests for CSVInOut util #42

Added tests for CSVInOut util #42

ajayborra commented Aug 8, 2018

tovbinm left a comment

tovbinm Aug 8, 2018

tovbinm Aug 8, 2018

tovbinm Aug 8, 2018

tovbinm Aug 8, 2018

codecov bot commented Aug 8, 2018 •

edited

Loading

ajayborra commented Aug 8, 2018

tovbinm Aug 8, 2018

ajayborra Aug 8, 2018

tovbinm Aug 8, 2018

ajayborra Aug 8, 2018

tovbinm Aug 8, 2018

tovbinm Aug 8, 2018

ajayborra Aug 9, 2018 •

edited

Loading

tovbinm Aug 9, 2018

tovbinm Aug 8, 2018

ajayborra Aug 8, 2018

tovbinm Aug 8, 2018 •

edited

Loading

tovbinm left a comment

		@@ -0,0 +1,101 @@
		Id,Description,Name,Item0,Item1,Item2,Item3,Vendor,Category,Probability

Added tests for CSVInOut util #42

Added tests for CSVInOut util #42

Conversation

ajayborra commented Aug 8, 2018

tovbinm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 8, 2018 • edited Loading

Codecov Report

ajayborra commented Aug 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajayborra Aug 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tovbinm Aug 8, 2018 • edited Loading

Choose a reason for hiding this comment

tovbinm left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 8, 2018 •

edited

Loading

ajayborra Aug 9, 2018 •

edited

Loading

tovbinm Aug 8, 2018 •

edited

Loading