Added test coverage for text, tuples, numeric, OpSparkListener #53
Conversation
ajayborra
commented
Aug 14, 2018
ajayborra
commented
- Added test coverage for text, tuples, numeric, OpSparkListener classes
Codecov Report
@@ Coverage Diff @@
## master #53 +/- ##
==========================================
+ Coverage 84.79% 85.67% +0.88%
==========================================
Files 298 298
Lines 9753 9753
Branches 329 540 +211
==========================================
+ Hits 8270 8356 +86
+ Misses 1483 1397 -86
Continue to review full report at Codecov.
|
| import org.scalatest.junit.JUnitRunner | ||
|
|
||
| @RunWith(classOf[JUnitRunner]) | ||
| class NumberTest extends FlatSpec with TestCommon { |
There was a problem hiding this comment.
Try using PropSpec. This is a classical example for property testing.
There was a problem hiding this comment.
Good point. But, Since we are already taking in a Double as a parameter here, Which already established the properties of the parameter are numerics. Seems like we are merely providing an API to validate if it is valid Number by checking for NaN (or) Infinity.
Tying to understand the ask here, Seems like the idea here is to restrict the Number type to have 2 more properties
- Number type should never be a NaN.
- Number type should never be Infinity.
Does that not require isValid to be implicitly called during type checking at compile time?
There was a problem hiding this comment.
This Number.isValud check is there to prevent invalid / unexpected computation results between numeric feature values at runtime. Property based test can be as simple as this:
@RunWith(classOf[JUnitRunner])
class NumberTest extends PropSpec {
property("validate numbers") should {
forAll(d: Double) { d => Number.isValid(d) shouldBe (!d.isNaN || !d.isInfinity) }
}
}| @RunWith(classOf[JUnitRunner]) | ||
| class OpSparkListenerTest extends FlatSpec with TestSparkContext { | ||
| val listener = new OpSparkListener(sc.appName, sc.applicationId, "testRun", Some("tag"), Some("tagValue"), true, true) | ||
| sc.addSparkListener(listener) |
There was a problem hiding this comment.
Question: can we somehow assert logs? Perhaps set a special log appender programmatically that would collect log messages. Do you its doable?
There was a problem hiding this comment.
Seems like we have a way to extract event log from EventLoggingListener will check that and get back.
There was a problem hiding this comment.
Let me know if it's doable in the scope if the test. I think that would be a great check as well: collect log messages and assert them at the end.
There was a problem hiding this comment.
Spent some time evaluating options around this and following are the observations.
EventLoggingListeneris private to spark package and doesn't seem to expose any API to read the events.- Since we are planning to assert based on the logs, Log appender looks to be the best approach here and it would be convenient if we can expose an InMemory log appender util for testing in general across the modules. Log4j doesn't provide this out of the box, We have to implement one similar to this example MemoryAppender
Once we have the above appender available, We can register it with spark context and start using it for assertions.
There was a problem hiding this comment.
MemoryAppender can be good. We can define one in tests, collect log lines, then assert the results. Lets do it in a separate PR.
| } | ||
|
|
||
| it should "not support mapping for more than 2 arguments" in { | ||
| assertDoesNotCompile("(Some(1), Some(2), Some(3)).map((x, y) => x + y)") |
There was a problem hiding this comment.
It's an extension for a Tuple2 so it's implied. I think you can remove this test case.
|
I will merge this one for now. Look some minor corrections in a separate PR. @ajayborra |
