InsightLOCOTest Refactor #412
Conversation
…ogrifAI into LocoTestRefactor merge master
AdamChit
requested review from
gerashegalov,
Jauntbox,
leahmcguire,
tovbinm and
wsuchy
as code owners
|
Thanks for the contribution! It looks like @AdamChit is an internal user so signing the CLA is not required. However, we need to confirm this. |
Codecov Report
@@ Coverage Diff @@
## master #412 +/- ##
==========================================
- Coverage 86.99% 83.31% -3.68%
==========================================
Files 337 337
Lines 11078 11078
Branches 369 597 +228
==========================================
- Hits 9637 9230 -407
- Misses 1441 1848 +407
Continue to review full report at Codecov.
|
| testData: RecordInsightsTestData[LogisticRegressionModel] | ||
| ): Unit = { | ||
| private def assertAggregatedWithPredicate(predicate: OpVectorColumnHistory => Boolean, | ||
| testData: RecordInsightsTestData[LogisticRegressionModel]): Unit = { |
There was a problem hiding this comment.
Can you revert the formatting here to the way it was before to match scala style guidelines?
| @@ -202,7 +202,8 @@ class RecordInsightsLOCOTest extends FlatSpec with TestSparkContext with RecordI | |||
| } | |||
| } | |||
|
|
|||
| it should "return the most predictive features for data generated with a strong relation to the label" in { | |||
| describe("data strongly related to label. ") { | |||
There was a problem hiding this comment.
This description doesn't seem to match the following section. Can you add something more descriptive?
| @@ -96,7 +96,7 @@ class RecordInsightsLOCOTest extends FlatSpec with TestSparkContext with RecordI | |||
| ) | |||
| ) | |||
| // scalastyle:on | |||
| Spec[RecordInsightsLOCO[_]] should "work with randomly generated features and binary logistic regression" in { | |||
| it ("should work with randomly generated features and binary logistic regression") { | |||
There was a problem hiding this comment.
replace it with Spec[RecordInsightsLOCO[_]]
There was a problem hiding this comment.
Can't directly do that with FunSpec, what worked best was wrapping describe(Spec[RecordInsightsLOCO[_]]) around the tests.
What the updated tests look like:
| @@ -202,7 +202,8 @@ class RecordInsightsLOCOTest extends FlatSpec with TestSparkContext with RecordI | |||
| } | |||
| } | |||
|
|
|||
| it should "return the most predictive features for data generated with a strong relation to the label" in { | |||
| describe("return the most predictive features for data that is strongly related to label. ") { | |||
| // Generate the data | |||
There was a problem hiding this comment.
maybe we should wrap data generation in an informative describe or add informers info. This part still feels too long
|
code coverage decrease not due to changes in this PR |
Related issues
Long test that needs to be refactored with the main goal of making it more readable and breaking it up into separate tests
Describe the proposed solution
First Change from FlatSpec to FunSpec then break up the test into 5 different test cases
Describe alternatives you've considered
could of put the data generation code into a function, but generating data is expensive so we wouldn't want to make repeated calls to it