2015-06-16

Pull request #1320

Addressing
protected access of tag_mobile_push_notification::sendNotification;
static::endNotification rather than self::endNotification in send_mobile_push_notification_internal;
servicePayloadTest.

servicePayloadTest is very simple:
1) given a specific input to send_mobile_push_notification_internal
2) mobile_push_service_gcm/apns:generatePayload generates a spefici output.

Setup
* A child of tag_mobile_push_service is created, test_tag_mobile_push_service;
* send_notification is overriden on the child;
* the protected methods test_tag_mobile_push_service_xxx::generatePayload are expoxed via test_tag_mobile_push_service::get_apns/gcm_payload;
* certain things are mocked, such as users, and calls to DBs.

Test
1) Call test_tag_mobile_push_service::send_mobile_push_notification_internal with a set of params
2) call test_tag_mobile_push_service_xxx::get_apns/gcm_payload; and compare actual and expected payloads.

The advantages of this approach
1. I need only mock a few things,
2. there is only one exception where variables other than the input to send_mobile_push_notifical_internal are tweaked,
3. as a result send_mobile_push_notifical_internal is allowd to behave "naturally",
3. access to the parameters passed to sendNotification without risk of polutuon because the test itself never handles them,
4. access to generatePayload which is protected without intrspection.

Requirements
* tag_mobile_push_service::sendNotification must be protected so that it can be overriden,
* tag_mobile_push_service::send_mobile_push_notification_internal must use because late static bindings when calling sendNotification, ie static:: rather than self::.

Fragility 

This test will break if
* the profile of send_mobile_push_notification_internal, or generatePayload changes, or
* if the payloads themselves change.

If the first is true then we are revamping push notifications in a way that probably makes the test redudant, if the second is true, then the test does exactly what it's meant to: detects changes in the payload.

I do not understand
a) why the test is a problem, it is not more compilcated than serviceTest or the mock framework we have, in fact it's simpler and allows for more control;
b) why static and protected are a problem, neither effect the code in any negative way;
c) why the test was was removed;
d) why I have had to defend this code so exhaustively in the face of absolutely no substantive problem or negative consequnce being raised,

e) why this pullrequest has not been approved.

Please, review the pull request and explain to me why this code is wrong.

No comments:

Post a Comment