robreeves edited a comment on pull request #35637: URL: https://github.com/apache/spark/pull/35637#issuecomment-1055756392
> Can we add a specific unit test to validate that the sign is not flipping ? The test `SPARK-26260: summary should contain only successful tasks' metrics (store = disk leveldb)` in `AppStatusStoreSuite` would catch this because it would mean a task is either included or excluded when it shouldn't be. This test is more like a mini-integration test. After researching if we could write a lower level test directly for the index value I am leaning towards the above test being the appropriate level to test at, but am open to changing this if you disagree. - `TaskDataWrapper` is only a data class so we could pass in whatever field values we want for the test, including making the sign flip. A test here would not really test any meaningful behavior. Additionally, we'd be making the fields `shuffleTotalReads` and `shuffleTotalBlocks` public just for testing. Not a huge deal and I am not dogmatic on this, but I generally consider exposing data/logic just for testing an anti-pattern and feel unit tests should focus on what a class, or classes, needs to expose publicly. - `LiveTask` is also private. I could change its access level to `LiveTask` and `TaskDataWrapper` to test it, but it is well covered in the `AppStatusStoreSuite` test. Since the behavior in `AppStatusStore` is what we really are concerned about and these are more helper classes I felt this is acceptable. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
