Added unit tests for ChromePermissionMessageProvider

Added unit tests for ChromePermissionMessageProvider

Added a unittest for ChromePermissionMessageProvider that ensures
permission messages currently coalesce and form as expected. Some of the
tests are purposely for broken functionality, just to ensure this
functionality exists (and will later be fixed). The tests will be
modified once the new model changes this permission behaviour.

BUG=398257
Committed: https://crrev.com/84817f9ba08f05f361721ae2ed6567d59894265c
Cr-Commit-Position: refs/heads/master@{#297148}

[sashab, 2014-09-23 04:52:55]

sashab@chromium.org changed reviewers:
+ scheib@chromium.org

[sashab, 2014-09-23 04:52:56]

Putting unit tests in place to verify the existing functionality/problems before
changing the permissions. Will be good to have these tests too - benwells@ was
discussing the importance of testing various permission combinations in our unit
tests, to ensure they 'make sense' to the user.

https://codereview.chromium.org/594003002/diff/1/chrome/common/extensions/per...
File
chrome/common/extensions/permissions/chrome_permission_message_provider_unittest.cc
(right):

https://codereview.chromium.org/594003002/diff/1/chrome/common/extensions/per...
chrome/common/extensions/permissions/chrome_permission_message_provider_unittest.cc:23:
#include "ui/base/l10n/l10n_util.h"
Some of these #includes violate checkdeps rules:

You added one or more #includes that violate checkdeps rules.
 
chrome/common/extensions/permissions/chrome_permission_message_provider_unittest.cc
      Illegal include: "chrome/browser/extensions/extension_service.h"
    Because of "-chrome" from chrome's include_rules. \
 
chrome/common/extensions/permissions/chrome_permission_message_provider_unittest.cc
      Illegal include: "chrome/browser/extensions/extension_service_test_base.h"
    Because of "-chrome" from chrome's include_rules. \
 
chrome/common/extensions/permissions/chrome_permission_message_provider_unittest.cc
      Illegal include: "chrome/browser/extensions/permissions_updater.h"
    Because of "-chrome" from chrome's include_rules. \
 
chrome/common/extensions/permissions/chrome_permission_message_provider_unittest.cc
      Illegal include: "chrome/browser/profiles/profile.h"
    Because of "-chrome" from chrome's include_rules. \
 
chrome/common/extensions/permissions/chrome_permission_message_provider_unittest.cc
      Illegal include: "extensions/browser/extension_prefs.h"
    Because of no rule applying. \
 
chrome/common/extensions/permissions/chrome_permission_message_provider_unittest.cc
      Illegal include: "extensions/browser/extension_registry.h"
    Because of no rule applying.

Is this file exempt from those rules because its a test? Or is there somewhere
better this test should go?

[scheib, 2014-09-23 17:10:54]

https://codereview.chromium.org/594003002/diff/1/chrome/common/extensions/per...
File
chrome/common/extensions/permissions/chrome_permission_message_provider_unittest.cc
(right):

https://codereview.chromium.org/594003002/diff/1/chrome/common/extensions/per...
chrome/common/extensions/permissions/chrome_permission_message_provider_unittest.cc:23:
#include "ui/base/l10n/l10n_util.h"
On 2014/09/23 04:52:56, sasha_b wrote:
> Some of these #includes violate checkdeps rules:
> 
> You added one or more #includes that violate checkdeps rules.
>  
>
chrome/common/extensions/permissions/chrome_permission_message_provider_unittest.cc
>       Illegal include: "chrome/browser/extensions/extension_service.h"
>     Because of "-chrome" from chrome's include_rules. \
>  
>
chrome/common/extensions/permissions/chrome_permission_message_provider_unittest.cc
>       Illegal include:
"chrome/browser/extensions/extension_service_test_base.h"
>     Because of "-chrome" from chrome's include_rules. \
>  
>
chrome/common/extensions/permissions/chrome_permission_message_provider_unittest.cc
>       Illegal include: "chrome/browser/extensions/permissions_updater.h"
>     Because of "-chrome" from chrome's include_rules. \
>  
>
chrome/common/extensions/permissions/chrome_permission_message_provider_unittest.cc
>       Illegal include: "chrome/browser/profiles/profile.h"
>     Because of "-chrome" from chrome's include_rules. \
>  
>
chrome/common/extensions/permissions/chrome_permission_message_provider_unittest.cc
>       Illegal include: "extensions/browser/extension_prefs.h"
>     Because of no rule applying. \
>  
>
chrome/common/extensions/permissions/chrome_permission_message_provider_unittest.cc
>       Illegal include: "extensions/browser/extension_registry.h"
>     Because of no rule applying.
> 
> Is this file exempt from those rules because its a test? Or is there somewhere
> better this test should go?

Looking into this raises a few questions:

1. Should ChromePermissionMessageProvider be refactored out of common and into
browser? 5min of code search led me to believe this is possible, and
conceptually I don't see why  ChromePermissionMessageProvider is needed anywhere
but in the browser.

2. [Predicated on ChromePermissionMessageProvider staying in common] Decide if
these tests should be for just the  ChromePermissionMessageProvider or for the
larger system. As written now this patch is testing an ecosystem all the way
from a manifest, through PermissionsParser, to the
ChromePermissionMessageProvider. I can see an argument for more 'functional /
system' testing to increase coverage. If you want to do that, perhaps it is best
to place this test in some location with visibility for all that, e.g.
chrome/browser/extensions/permission_message_unit_tests ... and comment at the
top of ChromePermissionMessageProvider the location for tests.

The alternative is to rework these tests to really just unit test 
ChromePermissionMessageProvider, remove the other dependencies. I think this
would be fine for the types of tests I see so far, because the other systems are
really just handing over data but are not involved in much logic. But perhaps
I'm overlooking something, or there's value in future extensions to these tests
that would need to rely on the other systems.

https://codereview.chromium.org/594003002/diff/1/chrome/common/extensions/per...
chrome/common/extensions/permissions/chrome_permission_message_provider_unittest.cc:37:
// Some of these tests are currently 'anti-tests' - they demonstrate existing
Which ones? Can I search this file for TODO or 'AntiTest' or crbug.com to find
them?

https://codereview.chromium.org/594003002/diff/1/chrome/common/extensions/per...
chrome/common/extensions/permissions/chrome_permission_message_provider_unittest.cc:67:
void CreateAppWithPermissions(const std::string id_seed,
Do we need an id_seed that varies between tests? Can this be simplified to just
always use a constant, or be omitted?

https://codereview.chromium.org/594003002/diff/1/chrome/common/extensions/per...
chrome/common/extensions/permissions/chrome_permission_message_provider_unittest.cc:80:
void CreateExtensionWithPermissions(const std::string id_seed,
ditto for id_seed here

https://codereview.chromium.org/594003002/diff/1/chrome/common/extensions/per...
chrome/common/extensions/permissions/chrome_permission_message_provider_unittest.cc:137:
std::vector<base::string16> messages(
Simplify test code by having the 'required_permissions()' and other methods
return the message strings formatted for assertions?

E.g. to read like:

ASSERT_EQ(1U, require_permissions().size());
EXPECT_EQ(l10n_util::GetStringUTF8(IDS_EXTENSION_PROMPT_WARNING_USB_SERIAL),
          require_permissions()[0]));

[sashab, 2014-09-24 23:23:49]

https://codereview.chromium.org/594003002/diff/1/chrome/common/extensions/per...
File
chrome/common/extensions/permissions/chrome_permission_message_provider_unittest.cc
(right):

https://codereview.chromium.org/594003002/diff/1/chrome/common/extensions/per...
chrome/common/extensions/permissions/chrome_permission_message_provider_unittest.cc:23:
#include "ui/base/l10n/l10n_util.h"
On 2014/09/23 17:10:54, scheib wrote:
> On 2014/09/23 04:52:56, sasha_b wrote:
> > Some of these #includes violate checkdeps rules:
> > 
> > You added one or more #includes that violate checkdeps rules.
> >  
> >
>
chrome/common/extensions/permissions/chrome_permission_message_provider_unittest.cc
> >       Illegal include: "chrome/browser/extensions/extension_service.h"
> >     Because of "-chrome" from chrome's include_rules. \
> >  
> >
>
chrome/common/extensions/permissions/chrome_permission_message_provider_unittest.cc
> >       Illegal include:
> "chrome/browser/extensions/extension_service_test_base.h"
> >     Because of "-chrome" from chrome's include_rules. \
> >  
> >
>
chrome/common/extensions/permissions/chrome_permission_message_provider_unittest.cc
> >       Illegal include: "chrome/browser/extensions/permissions_updater.h"
> >     Because of "-chrome" from chrome's include_rules. \
> >  
> >
>
chrome/common/extensions/permissions/chrome_permission_message_provider_unittest.cc
> >       Illegal include: "chrome/browser/profiles/profile.h"
> >     Because of "-chrome" from chrome's include_rules. \
> >  
> >
>
chrome/common/extensions/permissions/chrome_permission_message_provider_unittest.cc
> >       Illegal include: "extensions/browser/extension_prefs.h"
> >     Because of no rule applying. \
> >  
> >
>
chrome/common/extensions/permissions/chrome_permission_message_provider_unittest.cc
> >       Illegal include: "extensions/browser/extension_registry.h"
> >     Because of no rule applying.
> > 
> > Is this file exempt from those rules because its a test? Or is there
somewhere
> > better this test should go?
> 
> Looking into this raises a few questions:
> 
> 1. Should ChromePermissionMessageProvider be refactored out of common and into
> browser? 5min of code search led me to believe this is possible, and
> conceptually I don't see why  ChromePermissionMessageProvider is needed
anywhere
> but in the browser.
> 
> 2. [Predicated on ChromePermissionMessageProvider staying in common] Decide if
> these tests should be for just the  ChromePermissionMessageProvider or for the
> larger system. As written now this patch is testing an ecosystem all the way
> from a manifest, through PermissionsParser, to the
> ChromePermissionMessageProvider. I can see an argument for more 'functional /
> system' testing to increase coverage. If you want to do that, perhaps it is
best
> to place this test in some location with visibility for all that, e.g.
> chrome/browser/extensions/permission_message_unit_tests ... and comment at the
> top of ChromePermissionMessageProvider the location for tests.
> 
> The alternative is to rework these tests to really just unit test 
> ChromePermissionMessageProvider, remove the other dependencies. I think this
> would be fine for the types of tests I see so far, because the other systems
are
> really just handing over data but are not involved in much logic. But perhaps
> I'm overlooking something, or there's value in future extensions to these
tests
> that would need to rely on the other systems. 

I'm actually really glad you brought this up - it seems I've written this test
in two minds, one from the permissions perspective and one as a unit test.

I'd like to go with Option 2, treating this as a unit test just for
ChromePermissionMessageProvider. Maybe we can refactor it into browser another
time, but for now it actually helped narrow the purpose of the test and simplify
it a lot :)

Edit: went ahead and just made two sets of tests - one as a unit test for the
message provider, one as an integration test. The use cases separate nicely this
way.

https://codereview.chromium.org/594003002/diff/1/chrome/common/extensions/per...
chrome/common/extensions/permissions/chrome_permission_message_provider_unittest.cc:37:
// Some of these tests are currently 'anti-tests' - they demonstrate existing
On 2014/09/23 17:10:54, scheib wrote:
> Which ones? Can I search this file for TODO or 'AntiTest' or http://crbug.com
to find
> them?

Preceded them with 'AntiTest_'.

https://codereview.chromium.org/594003002/diff/1/chrome/common/extensions/per...
chrome/common/extensions/permissions/chrome_permission_message_provider_unittest.cc:67:
void CreateAppWithPermissions(const std::string id_seed,
On 2014/09/23 17:10:54, scheib wrote:
> Do we need an id_seed that varies between tests? Can this be simplified to
just
> always use a constant, or be omitted?

Now uses a constant.

https://codereview.chromium.org/594003002/diff/1/chrome/common/extensions/per...
chrome/common/extensions/permissions/chrome_permission_message_provider_unittest.cc:80:
void CreateExtensionWithPermissions(const std::string id_seed,
On 2014/09/23 17:10:54, scheib wrote:
> ditto for id_seed here

Done.

https://codereview.chromium.org/594003002/diff/1/chrome/common/extensions/per...
chrome/common/extensions/permissions/chrome_permission_message_provider_unittest.cc:137:
std::vector<base::string16> messages(
On 2014/09/23 17:10:54, scheib wrote:
> Simplify test code by having the 'required_permissions()' and other methods
> return the message strings formatted for assertions?
> 
> E.g. to read like:
> 
> ASSERT_EQ(1U, require_permissions().size());
> EXPECT_EQ(l10n_util::GetStringUTF8(IDS_EXTENSION_PROMPT_WARNING_USB_SERIAL),
>           require_permissions()[0]));

Good idea. Done :)

[scheib, 2014-09-25 17:55:45]

lgtm with some requests to improve comments and naming.

https://codereview.chromium.org/594003002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/permission_messages_unittest.cc (right):

https://codereview.chromium.org/594003002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/permission_messages_unittest.cc:34: // Some of these
tests are currently 'anti-tests' - they demonstrate existing
Call out the naming convention:
"Some of these tests are prefixed "AntiTest_", they demonstrate ...

https://codereview.chromium.org/594003002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/permission_messages_unittest.cc:38: class
PermissionMessagesUnittest : public ExtensionServiceTestBase {
Cite the existence of this test suite at the top of the chrome permission
message provider class, so that people know to go looking in a far off place for
it.

https://codereview.chromium.org/594003002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/permission_messages_unittest.cc:81:
std::vector<base::string16> RequestOptionalPermissions() {
This name seems a tad confusing to me still. Perhaps RequestedPermissions?

https://codereview.chromium.org/594003002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/permission_messages_unittest.cc:200: // granted
permission.
Call out more explicitly what is 'AntiTest' about this. Why is it wrong, or what
should it do? Possibly prefix with something encouraging it to be fixed:
"""
AntiTest: This behavior should be changed and improved.
If an app requests 'tabs' permission, but already has 'history', a prompt is
displayed. Something should have happened instead. It should be like this other
way.
"""

[sashab, 2014-09-29 08:03:36]

https://codereview.chromium.org/594003002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/permission_messages_unittest.cc (right):

https://codereview.chromium.org/594003002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/permission_messages_unittest.cc:34: // Some of these
tests are currently 'anti-tests' - they demonstrate existing
On 2014/09/25 17:55:44, scheib wrote:
> Call out the naming convention:
> "Some of these tests are prefixed "AntiTest_", they demonstrate ...

Done.

https://codereview.chromium.org/594003002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/permission_messages_unittest.cc:38: class
PermissionMessagesUnittest : public ExtensionServiceTestBase {
On 2014/09/25 17:55:44, scheib wrote:
> Cite the existence of this test suite at the top of the chrome permission
> message provider class, so that people know to go looking in a far off place
for
> it.

Good idea. Done.

https://codereview.chromium.org/594003002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/permission_messages_unittest.cc:81:
std::vector<base::string16> RequestOptionalPermissions() {
On 2014/09/25 17:55:44, scheib wrote:
> This name seems a tad confusing to me still. Perhaps RequestedPermissions? 

Changed to GetOptionalPermissionMessages() :)

https://codereview.chromium.org/594003002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/permission_messages_unittest.cc:200: // granted
permission.
On 2014/09/25 17:55:44, scheib wrote:
> Call out more explicitly what is 'AntiTest' about this. Why is it wrong, or
what
> should it do? Possibly prefix with something encouraging it to be fixed:
> """
> AntiTest: This behavior should be changed and improved.
> If an app requests 'tabs' permission, but already has 'history', a prompt is
> displayed. Something should have happened instead. It should be like this
other
> way.
> """

Thanks. Updated both.

[sashab, 2014-09-29 08:03:40]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2014-09-29 08:04:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/594003002/40001

[commit-bot: I haz the power, 2014-09-29 08:07:51]

Committed patchset #3 (id:40001) as 7eb79c4c5f019a40782d59d3aa55eb55faf74cae

[commit-bot: I haz the power, 2014-09-29 08:08:40]

Patchset 3 (id:??) landed as
https://crrev.com/84817f9ba08f05f361721ae2ed6567d59894265c
Cr-Commit-Position: refs/heads/master@{#297148}