Github user squito commented on a diff in the pull request:
https://github.com/apache/spark/pull/13454#discussion_r65713698
--- Diff:
core/src/test/scala/org/apache/spark/scheduler/SchedulerIntegrationSuite.scala
---
@@ -133,6 +133,11 @@ abstract class SchedulerIntegrationSuite[T <:
MockBackend: ClassTag] extends Spa
// when the job succeeds
assert(taskScheduler.runningTaskSets.isEmpty)
assert(!backend.hasTasks)
+ } else {
+ // Note that we CANNOT check for empty results on a failure -- the
resultHandler will
--- End diff --
Yeah I agree this is a little weird, its not really going to help somebody
writing a test, only if somebody looks at the entire framework here a little
more closely. I thought of passing in expected results or expected failure,
but maybe in some test you don't want to check the expectation exactly. I
guess somebody writing a test at least needs to understand how to use
`assertDataStructuresEmpty`, `results`, and `failure`. I'll put comments there
instead, someone writing a test is at least more likely to read those comments.
I'm not totally sure I know what you mean about "job failure after 4
attempts" -- I did remove the `assert(results.isEmpty)` there. Actually, that
assert is fine (all tasks fail, so there are never any results), but I thought
it would be better to remove it in case someone copies that code into another
test without thinking about it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
