Skip to content
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

Added test coverage for CSVToAvro util #50

Merged
merged 5 commits into from
Aug 13, 2018
Merged

Added test coverage for CSVToAvro util #50

merged 5 commits into from
Aug 13, 2018

Conversation

ajayborra
Copy link
Contributor

  • Added test coverage for CSVToAvro util
  • Added tests to RichGenericRecord.scala

@@ -0,0 +1,47 @@
{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have too many of the same or slighly different schema and data files already.

  1. Can we not reuse tthe existing ones in this test?
  2. If not, them them give them a better names than simply adding 2.
  3. Perhaps move these new “bad schema” files to utils/test/resources

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 1:Re: To improve coverage, had to modify the data to cover negative cases. (Limited the data copies to 5 rows)
  • 2:Re: Done, Renamed files, Check them out.
  • 3:Re: Done, Moved the files to the resource folder.


it should "convert GenericRecord to RichGenericRecord" in {
val res = RichGenericRecord(firstRow)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to explicitly convert it - its an implicit class.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is more of a coverage test, will remove it.

val avroSchema: String = loadFile(avroSchemaPath)
val csvFile: String = s"$testDataDir/PassengerDataAllV2.csv"
val csvReader: CSVInOut = new CSVInOut(CSVOptions(header = true))
val csvRDD: RDD[Seq[String]] = csvReader.readRDD(csvFile)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make csvRDD and csvFileRecordCount lazy vals


Spec(CSVToAvro.getClass) should "convert RDD[Seq[String]] to RDD[GenericRecord]" in {
val res = CSVToAvro.toAvro(csvRDD, avroSchema)
assert(res.isInstanceOf[RDD[GenericRecord]])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

res should be a[RDD[_]]


it should "convert RDD[Seq[String]] to RDD[T]" in {
val res = CSVToAvro.toAvroTyped[Passenger](csvRDD, avroSchema)
assert(res.isInstanceOf[RDD[Passenger]])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

@codecov
Copy link

codecov bot commented Aug 13, 2018

Codecov Report

Merging #50 into master will increase coverage by 0.3%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master      #50     +/-   ##
=========================================
+ Coverage   84.46%   84.77%   +0.3%     
=========================================
  Files         298      298             
  Lines        9751     9752      +1     
  Branches      355      560    +205     
=========================================
+ Hits         8236     8267     +31     
+ Misses       1515     1485     -30
Impacted Files Coverage Δ
...m/salesforce/op/utils/avro/RichGenericRecord.scala 38.46% <100%> (+7.69%) ⬆️
...main/scala/com/salesforce/op/test/TestCommon.scala 43.75% <100%> (+3.75%) ⬆️
...ala/com/salesforce/op/utils/io/csv/CSVToAvro.scala 87.87% <0%> (+87.87%) ⬆️

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 f379975...0dd9ee6. Read the comment docs.

/**
* Returns the resource directory path
*/
protected def resourceDir = _resourceDir
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to create a private val _resourceDir = "src/test/resources" simply do:

protected def resourceDir: String ="src/test/resources"

Copy link
Collaborator

@tovbinm tovbinm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! one minor comment

@tovbinm tovbinm merged commit d6f3d1b into salesforce:master Aug 13, 2018
@ajayborra ajayborra deleted the ab/coverage branch August 13, 2018 21:37
ericwayman pushed a commit that referenced this pull request Feb 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants