Github user JoshRosen commented on the pull request:
https://github.com/apache/spark/pull/4135#issuecomment-91069166
On closer inspection, my [previous
comment](https://github.com/apache/spark/pull/4135#discussion_r26727249) about
testing still stands
> I notice that this test harness the logic from the real receiver tracker.
Is this test suite actually covering the changes made to ReceiverTracker? In
other words, if I were to go in and delete the callback registration code that
you added to the real tracker, would that cause any tests to fail? If not, then
I think we need to do additional work to test this, since the test isn't really
testing anything if it can't expose bugs in the real code.
> It seems like we'd like to test against a real ReceiverTracker using mock
implementations of ReceiverSupervisor and Receiver. I'm not super-familiar with
this corner of the streaming code, though, so maybe @tdas can chime in with
suggestions on how to test this?
Since the changes in this patch modify ReceiverTracker, it makes sense to
test the actual ReceiverTracker while mocking other components. The updated
test still doesn't exercise the real receiver tracker and thus isn't actually
covering the streaming changes in this patch.
I understand that properly testing ReceiverTracker might not be the easiest
test to write, so perhaps this would require refactoring of ReceiverTracker to
make the individual components more testable. I'll let @tdas chime in to
decide on the importance of writing a real test for this. Sorry to be
pedantic, but I'm a bit of a stickler for the importance of writing tests that
actually exercise the code being modified, since I think it's very rare that
it's truly impossible to test changes like this.
---
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]
