robolectric/robolectric — pull request #1142 by kriegfrj (2014-06-10 05:01:46+00:00)
Title: Sanity enforcement for foreground flag in NotificationManager

Made some enhancements centered around enforcing the correct state of the FLAG_FOREGROUND_SERVICE notification flag. The standard Android notify() method does not honour this flag as it should only be set through Service.setForeground(), and so I have modified the code to do the same - if this flag is set by the caller then ShadowNotificationManager will clear it. A separate method has been provided for test code (such as ShadowService.setForeground()) that needs to bypass this sanity enforcement.

A possible future enhancement will be for ShadowNotificationManager to optionally assert if notify() is called with this flag set, because if ordinary Android code has this flag set it likely indicates a programming error and Android won't do what the developer might have been hoping to achieve.

One possible pushback to this PR is the fact that it is potentially a breaking change - if any test code out there relies on the loose implementation of notify(), then it might break. There are two ways in which it might break tests:

1. If someone has production code that relies on the old behaviour, or
2. If someone has test code that relies on the old behaviour.

I'm less concerned about upsetting people of the first type because we would be helping them uncover a bug that had hitherto gone unnoticed. However, if someone is calling notify() from a test method to (eg) emulate a foreground notification in their test, this will fail until they change their code to use notifyRaw() instead. Happy to take feedback on this issue from the wider team (who are likely to have more experience in this area) as to whether this possibility is likely enough to worry about, and if so how to handle it. Perhaps I could make the sanity enforcement a configurable option for the ShadowNotificationManager, and leave default behaviour as it currently is? This same configuration mechanism could be extended to do allow a mode that asserts later.