Move many of the Autofill unittests into components_unittests.

Move many of the Autofill unittests into components_unittests.

The Autofill unittests that remain in unit_tests all have non-trivial //chrome
dependencies that must be removed before they can be moved to
components_unittests.

This CL also extends the components unittests to load resources, as many of the
Autofill tests assume that resources are loaded. Currently //chrome's resources
are loaded; in the long term this should be changed so that
components_unittests has its own pakfile.

TBR=ben

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=254745

[blundell, 2014-02-19 16:44:47]

Hi Thiago,

Do you have any idea why the resources aren't being loaded on several platforms
here? I'm not very familiar with how the resource bundle works. Thanks!

[blundell, 2014-02-19 16:45:10]

+joi@ in case you have any insight

[Jói, 2014-02-19 17:27:20]

I don't remember the specifics but I seem to recall that you might
need to modify //components/test/run_all_unittests.cc for Android to
load certain resources (the JNI bindings bits). Not sure about Mac,
but note that components_tests.gyp has a Windows-specific section that
is declaring dependencies on resource and strings files...

I'm sorry I don't remember this better off the top of my head. I can
try to dig some more tomorrow if needed.

Cheers,
Jói


On Wed, Feb 19, 2014 at 4:45 PM,  <blundell@chromium.org> wrote:
> +joi@ in case you have any insight
>
> https://codereview.chromium.org/172473002/
>
> --
> You received this message because you are subscribed to the Google Groups
> "browser-components-watch" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to browser-components-watch+unsubscribe@chromium.org.
> To post to this group, send email to browser-components-watch@chromium.org.
> To view this discussion on the web visit
>
https://groups.google.com/a/chromium.org/d/msgid/browser-components-watch/e89....
> For more options, visit
> https://groups.google.com/a/chromium.org/groups/opt_out.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ilya Sherman, 2014-02-19 20:02:33]

Drive-by: Can we create an autofill_unittests suite, rather than bundling all
the components together?  One of the expected benefits of componentization is
smaller test suite binaries which are faster to compile.  I'd love to see that
dream realized :)

[tfarina, 2014-02-19 22:11:27]

Hi Colin, I'm looking now.

[tfarina, 2014-02-19 22:21:03]

Colin, lets try this incantation:

  base::FilePath resources_pack_path;    
#if defined(OS_MACOSX) && !defined(OS_IOS)     
  PathService::Get(base::DIR_MODULE, &resources_pack_path);    
  resources_pack_path =    
      resources_pack_path.Append(FILE_PATH_LITERAL("resources.pak"));    
#else     
  PathService::Get(chrome::FILE_RESOURCES_PACK, &resources_pack_path);    
#endif     
  ResourceBundle::GetSharedInstance().AddDataPackFromPath(    
      resources_pack_path, ui::SCALE_FACTOR_NONE);

Could you put this after InitSharedInstanceWithLocale() and see if it fixes the
failures?

You may need Mac only code too.

[Jói, 2014-02-20 10:59:09]

> Drive-by: Can we create an autofill_unittests suite, rather than bundling
> all the components together?  [...]

There's a trade-off in that every new test suite means some fairly
involved work to get trybots and bots on the waterfall to run that
test suite, and there's also a trade-off in how long tests take to
run; a larger test suite can more effectively be parallelized and pays
its start-up cost only once.

If you want to split things up further I would suggest coordinating
with the infrastructure team first, to see what they think.
Personally, I think the current situation is the right trade-off.

Cheers,
Jói


On Wed, Feb 19, 2014 at 8:02 PM,  <isherman@chromium.org> wrote:
> Drive-by: Can we create an autofill_unittests suite, rather than bundling
> all
> the components together?  One of the expected benefits of componentization
> is
> smaller test suite binaries which are faster to compile.  I'd love to see
> that
> dream realized :)
>
>
> https://codereview.chromium.org/172473002/
>
> --
> You received this message because you are subscribed to the Google Groups
> "browser-components-watch" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to browser-components-watch+unsubscribe@chromium.org.
> To post to this group, send email to browser-components-watch@chromium.org.
> To view this discussion on the web visit
>
https://groups.google.com/a/chromium.org/d/msgid/browser-components-watch/089....
>
> For more options, visit
> https://groups.google.com/a/chromium.org/groups/opt_out.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[blundell, 2014-02-20 15:39:57]

Hi Thiago,

Thanks for the pointer! I made some progress: things work on Mac but not on
other platforms. Unfortunately, I only have a Mac to build on locally, and the
failures on Linux/Windows are quite cryptic.

In any case, it looks like adding the //chrome dependencies to
components_tests.gyp hoses Android and iOS. Thus, I think the more productive
path forward is to directly attack the problem of having components_tests load
its own pakfile. How can I help with that? I'm highly motivated to get this
working as its absence is becoming a blocker for moving tests from unit_tests to
components_unittests.

Thanks,

Colin

[blundell, 2014-02-20 15:42:54]

Ilya,

I agree that there would be benefits to having the components each have their
own separate test suite. As joi@ mentions, there would also be a lot of overhead
to doing that. I wonder if a good middle ground would be to have the convention
that each components' tests get "namespaced" somehow so that it would be easy to
run only that suite via --gtest_filter (e.g., AutofillComponent*). The ship
might have already sailed on that though.

[tfarina, 2014-02-20 18:42:56]

Hi Colin,

On 2014/02/20 15:39:57, blundell wrote:
> Hi Thiago,
> 
> Thanks for the pointer! I made some progress: things work on Mac but not on
> other platforms. Unfortunately, I only have a Mac to build on locally, and the
> failures on Linux/Windows are quite cryptic.
> 
> In any case, it looks like adding the //chrome dependencies to
> components_tests.gyp hoses Android and iOS. Thus, I think the more productive
> path forward is to directly attack the problem of having components_tests load
> its own pakfile. How can I help with that? I'm highly motivated to get this
> working as its absence is becoming a blocker for moving tests from unit_tests
to
> components_unittests.
> 
We will need to create our own pak file irrc. This is tricky because we need to
know which resource files are necessary.

I can give you three examples where we can look and see if we can replicate
here:

https://code.google.com/p/chromium/codesearch#chromium/src/content/content_sh...
https://code.google.com/p/chromium/codesearch#chromium/src/ui/resources/ui_re...
https://codereview.chromium.org/173883002/diff/1/apps/apps.gypi

Even rolling our own pak file will not make it easier to make this work on
Android and iOS. If I remember, last time I tried doing this, this was where I
got blocked.

[Ilya Sherman, 2014-02-20 23:04:12]

On 2014/02/20 10:59:09, Jói wrote:
> > Drive-by: Can we create an autofill_unittests suite, rather than bundling
> > all the components together?  [...]
> 
> There's a trade-off in that every new test suite means some fairly
> involved work to get trybots and bots on the waterfall to run that
> test suite, and there's also a trade-off in how long tests take to
> run; a larger test suite can more effectively be parallelized and pays
> its start-up cost only once.
> 
> If you want to split things up further I would suggest coordinating
> with the infrastructure team first, to see what they think.
> Personally, I think the current situation is the right trade-off.

I'm sure we could find a way to make the cost of adding a new test suite pretty
trivial in terms of bot configuration time, perhaps by having a virtual
"components" suite that the bots run.

In terms of start-up cost and parallelization, I'm not really convinced.  Even
if we assign one test suite per component, we're only talking about dozens of
test suites total, with hundreds or thousands of tests each.  They should still
be easy to parallelize, and start-up time should be trivial compared to the
total runtime of the suite.

I do agree that it would be wise to loop in the infrastructure folks; but as a
developer, I think the developer productivity improvement here is worth
pursuing.

On 2014/02/20 15:42:54, blundell wrote:
> Ilya,
> 
> I agree that there would be benefits to having the components each have their
> own separate test suite. As joi@ mentions, there would also be a lot of
overhead
> to doing that. I wonder if a good middle ground would be to have the
convention
> that each components' tests get "namespaced" somehow so that it would be easy
to
> run only that suite via --gtest_filter (e.g., AutofillComponent*). The ship
> might have already sailed on that though.

IMO, by the time you get to the point where --gtest_filter is usable, it's too
late.  My concern is that test suites currently take ages to build and link. 
That means that they code-compile-test cycle time is significantly slowed down
waiting for code to compile.  I already make heavy use of --gtest_filter, but I
still feel like I'm losing a lot of development time just because we have this
colossal monolithic test suite.

[Jói, 2014-02-21 10:29:34]

I think the components_unittests executable builds pretty quickly, but
that may change over time. Please pursue your ideas about splitting
things up etc. if and when you feel it is worth it.

Cheers,
Jói


On Thu, Feb 20, 2014 at 11:04 PM,  <isherman@chromium.org> wrote:
> On 2014/02/20 10:59:09, Jói wrote:
>>
>> > Drive-by: Can we create an autofill_unittests suite, rather than
>> > bundling
>> > all the components together?  [...]
>
>
>> There's a trade-off in that every new test suite means some fairly
>> involved work to get trybots and bots on the waterfall to run that
>> test suite, and there's also a trade-off in how long tests take to
>> run; a larger test suite can more effectively be parallelized and pays
>> its start-up cost only once.
>
>
>> If you want to split things up further I would suggest coordinating
>> with the infrastructure team first, to see what they think.
>> Personally, I think the current situation is the right trade-off.
>
>
> I'm sure we could find a way to make the cost of adding a new test suite
> pretty
> trivial in terms of bot configuration time, perhaps by having a virtual
> "components" suite that the bots run.
>
> In terms of start-up cost and parallelization, I'm not really convinced.
> Even
> if we assign one test suite per component, we're only talking about dozens
> of
> test suites total, with hundreds or thousands of tests each.  They should
> still
> be easy to parallelize, and start-up time should be trivial compared to the
> total runtime of the suite.
>
> I do agree that it would be wise to loop in the infrastructure folks; but as
> a
> developer, I think the developer productivity improvement here is worth
> pursuing.
>
>
> On 2014/02/20 15:42:54, blundell wrote:
>>
>> Ilya,
>
>
>> I agree that there would be benefits to having the components each have
>> their
>> own separate test suite. As joi@ mentions, there would also be a lot of
>
> overhead
>>
>> to doing that. I wonder if a good middle ground would be to have the
>
> convention
>>
>> that each components' tests get "namespaced" somehow so that it would be
>> easy
>
> to
>>
>> run only that suite via --gtest_filter (e.g., AutofillComponent*). The
>> ship
>> might have already sailed on that though.
>
>
> IMO, by the time you get to the point where --gtest_filter is usable, it's
> too
> late.  My concern is that test suites currently take ages to build and link.
> That means that they code-compile-test cycle time is significantly slowed
> down
> waiting for code to compile.  I already make heavy use of --gtest_filter,
> but I
> still feel like I'm losing a lot of development time just because we have
> this
> colossal monolithic test suite.
>
> https://codereview.chromium.org/172473002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tfarina, 2014-02-21 19:20:34]

https://codereview.chromium.org/172473002/diff/420001/components/components_t...
File components/components_tests.gyp (right):

https://codereview.chromium.org/172473002/diff/420001/components/components_t...
components/components_tests.gyp:173:
'../webkit/webkit_resources.gyp:webkit_resources',
we need to see if webkit needs to load these Autofill Credit Card resources.

https://code.google.com/p/chromium/codesearch#chromium/src/webkit/child/webki...

https://code.google.com/p/chromium/codesearch#chromium/src/webkit/child/webki...

https://code.google.com/p/chromium/codesearch#chromium/src/webkit/child/webki...

Ilya, Chris, Evan, Dean, one might know this.

[Ilya Sherman, 2014-02-22 00:30:14]

https://codereview.chromium.org/172473002/diff/420001/components/components_t...
File components/components_tests.gyp (right):

https://codereview.chromium.org/172473002/diff/420001/components/components_t...
components/components_tests.gyp:173:
'../webkit/webkit_resources.gyp:webkit_resources',
On 2014/02/21 19:20:34, tfarina wrote:
> we need to see if webkit needs to load these Autofill Credit Card resources.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/webkit/child/webki...
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/webkit/child/webki...
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/webkit/child/webki...
> 
> Ilya, Chris, Evan, Dean, one might know this.

Blink no longer needs to load these resources; they should be moved into the
Autofill component.

[tfarina, 2014-02-22 01:40:46]

On 2014/02/22 00:30:14, Ilya Sherman wrote:
>
https://codereview.chromium.org/172473002/diff/420001/components/components_t...
> File components/components_tests.gyp (right):
> 
>
https://codereview.chromium.org/172473002/diff/420001/components/components_t...
> components/components_tests.gyp:173:
> '../webkit/webkit_resources.gyp:webkit_resources',
> On 2014/02/21 19:20:34, tfarina wrote:
> > we need to see if webkit needs to load these Autofill Credit Card resources.
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/webkit/child/webki...
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/webkit/child/webki...
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/webkit/child/webki...
> > 
> > Ilya, Chris, Evan, Dean, one might know this.
> 
> Blink no longer needs to load these resources; they should be moved into the
> Autofill component.

I have uploaded https://codereview.chromium.org/165673007, so we can address
this separately.

[blundell, 2014-02-25 15:33:07]

On 2014/02/22 01:40:46, tfarina wrote:
> On 2014/02/22 00:30:14, Ilya Sherman wrote:
> >
>
https://codereview.chromium.org/172473002/diff/420001/components/components_t...
> > File components/components_tests.gyp (right):
> > 
> >
>
https://codereview.chromium.org/172473002/diff/420001/components/components_t...
> > components/components_tests.gyp:173:
> > '../webkit/webkit_resources.gyp:webkit_resources',
> > On 2014/02/21 19:20:34, tfarina wrote:
> > > we need to see if webkit needs to load these Autofill Credit Card
resources.
> > > 
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/webkit/child/webki...
> > > 
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/webkit/child/webki...
> > > 
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/webkit/child/webki...
> > > 
> > > Ilya, Chris, Evan, Dean, one might know this.
> > 
> > Blink no longer needs to load these resources; they should be moved into the
> > Autofill component.
> 
> I have uploaded https://codereview.chromium.org/165673007, so we can address
> this separately.

I have access to a Linux machine now, so I'm seeing if I can repro the failures
on the bots and debug locally.

[blundell, 2014-02-26 12:16:56]

Hi Jói,

The bots seem to be happy now :). If you're OK with the approach taken in this
CL, I'll file a bug for moving from using //chrome's resources to having a
separate pakfile for components_unittests and reference it where I currently
have the TODOs about creating such a bug.

Thanks,

Colin

[tfarina, 2014-02-26 12:51:43]

https://codereview.chromium.org/172473002/diff/520001/components/test/run_all...
File components/test/run_all_unittests.cc (right):

https://codereview.chromium.org/172473002/diff/520001/components/test/run_all...
components/test/run_all_unittests.cc:65:
resources_pack_path.Append(FILE_PATH_LITERAL("resources.pak"));
you can inline this in AddDataPackFromPath call as:

resource_path_path.AppendASCII("resources.pak");

https://codereview.chromium.org/172473002/diff/520001/components/test/run_all...
components/test/run_all_unittests.cc:66:
ResourceBundle::GetSharedInstance().AddDataPackFromPath(
nit: ui::ResourceBundle

[Jói, 2014-02-26 13:12:21]

Yes, the approach LGTM, sounds good to file that bug.

On Wed, Feb 26, 2014 at 12:51 PM,  <tfarina@chromium.org> wrote:
>
>
https://codereview.chromium.org/172473002/diff/520001/components/test/run_all...
> File components/test/run_all_unittests.cc (right):
>
>
https://codereview.chromium.org/172473002/diff/520001/components/test/run_all...
> components/test/run_all_unittests.cc:65:
> resources_pack_path.Append(FILE_PATH_LITERAL("resources.pak"));
> you can inline this in AddDataPackFromPath call as:
>
> resource_path_path.AppendASCII("resources.pak");
>
>
https://codereview.chromium.org/172473002/diff/520001/components/test/run_all...
> components/test/run_all_unittests.cc:66:
> ResourceBundle::GetSharedInstance().AddDataPackFromPath(
> nit: ui::ResourceBundle
>
> https://codereview.chromium.org/172473002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[blundell, 2014-03-03 14:57:51]

Addressed review comments, added bug references, and made some fixes to enable
the resources to load on iOS. Jói, could you take one more look?

Thanks,

Colin

https://codereview.chromium.org/172473002/diff/520001/components/test/run_all...
File components/test/run_all_unittests.cc (right):

https://codereview.chromium.org/172473002/diff/520001/components/test/run_all...
components/test/run_all_unittests.cc:65:
resources_pack_path.Append(FILE_PATH_LITERAL("resources.pak"));
On 2014/02/26 12:51:44, tfarina wrote:
> you can inline this in AddDataPackFromPath call as:
> 
> resource_path_path.AppendASCII("resources.pak");

Done.

https://codereview.chromium.org/172473002/diff/520001/components/test/run_all...
components/test/run_all_unittests.cc:66:
ResourceBundle::GetSharedInstance().AddDataPackFromPath(
On 2014/02/26 12:51:44, tfarina wrote:
> nit: ui::ResourceBundle

Done.

[Jói, 2014-03-03 14:59:34]

LGTM

[blundell, 2014-03-03 20:34:10]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2014-03-03 20:35:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/blundell@chromium.org/172473002/580001

[commit-bot: I haz the power, 2014-03-03 20:35:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-03 20:35:44]

Failed to apply patch for components/components_tests.gyp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file components/components_tests.gyp
  Hunk #2 FAILED at 18.
  Hunk #5 succeeded at 168 with fuzz 2 (offset 3 lines).
  Hunk #6 succeeded at 209 (offset 6 lines).
  Hunk #7 succeeded at 231 (offset 6 lines).
  Hunk #8 succeeded at 266 (offset 6 lines).
  1 out of 8 hunks FAILED -- saving rejects to file
components/components_tests.gyp.rej

Patch:       components/components_tests.gyp
Index: components/components_tests.gyp
diff --git a/components/components_tests.gyp b/components/components_tests.gyp
index
ea83ae60c7c76ba79010459987ca55be26b5be5c..a95f52aac36b29716908ed50309cd6af9089cc5e
100644
--- a/components/components_tests.gyp
+++ b/components/components_tests.gyp
@@ -8,6 +8,7 @@
     # platforms to include source files on (e.g. files ending in
     # _mac.h or _mac.cc are only compiled on MacOSX).
     'chromium_code': 1,
+    'grit_out_dir': '<(SHARED_INTERMEDIATE_DIR)/components',
    },
   'conditions': [
     ['android_webview_build == 0', {
@@ -17,6 +18,29 @@
           'type': '<(gtest_target_type)',
           'sources': [
             'auto_login_parser/auto_login_parser_unittest.cc',
+            'autofill/core/browser/address_field_unittest.cc',
+            'autofill/core/browser/address_unittest.cc',
+           
'autofill/core/browser/android/auxiliary_profile_unittest_android.cc',
+            'autofill/core/browser/autofill_country_unittest.cc',
+            'autofill/core/browser/autofill_data_model_unittest.cc',
+            'autofill/core/browser/autofill_download_url_unittest.cc',
+            'autofill/core/browser/autofill_field_unittest.cc',
+            'autofill/core/browser/autofill_merge_unittest.cc',
+            'autofill/core/browser/autofill_profile_unittest.cc',
+            'autofill/core/browser/autofill_regexes_unittest.cc',
+            'autofill/core/browser/autofill_type_unittest.cc',
+            'autofill/core/browser/autofill_xml_parser_unittest.cc',
+            'autofill/core/browser/contact_info_unittest.cc',
+            'autofill/core/browser/credit_card_field_unittest.cc',
+            'autofill/core/browser/credit_card_unittest.cc',
+            'autofill/core/browser/form_field_unittest.cc',
+            'autofill/core/browser/form_structure_unittest.cc',
+            'autofill/core/browser/name_field_unittest.cc',
+            'autofill/core/browser/password_generator_unittest.cc',
+            'autofill/core/browser/phone_field_unittest.cc',
+            'autofill/core/browser/phone_number_unittest.cc',
+            'autofill/core/browser/phone_number_i18n_unittest.cc',
+            'autofill/core/browser/validation_unittest.cc',
             'autofill/core/browser/webdata/autofill_entry_unittest.cc',
             'autofill/core/browser/webdata/web_data_service_unittest.cc',
             'autofill/core/common/form_data_unittest.cc',
@@ -97,6 +121,9 @@
           'dependencies': [
             '../base/base.gyp:base_prefs_test_support',
             '../base/base.gyp:test_support_base',
+            # TODO(blundell): Eliminate this dependency by having
+            # components_unittests have its own pakfile. crbug.com/348563
+            '../chrome/chrome_resources.gyp:packed_extra_resources',
             # TODO(blundell): Eliminate the need for this dependency in code
             # that iOS shares. crbug.com/325243
             '../content/content_shell_and_tests.gyp:test_support_content',
@@ -113,6 +140,8 @@
             'components.gyp:autofill_core_browser',
             'components.gyp:autofill_core_common',
             'components.gyp:autofill_core_test_support',
+            'component_strings.gyp:component_strings',
+            '../third_party/libphonenumber/libphonenumber.gyp:libphonenumber',
 
             # Dependencies of cloud_devices
             'components.gyp:cloud_devices',
@@ -159,6 +188,11 @@
           'conditions': [
             ['OS != "ios"', {
               'dependencies': [
+                # Dependencies of
autofill/core/browser/credit_card_unittest.cc.
+                # TODO(blundell): Eliminate the need for this conditional
+                # dependence. crbug.com/328150
+                '../webkit/webkit_resources.gyp:webkit_resources',
+                
                 # Dependencies of browser_context_keyed_service
                 'components.gyp:browser_context_keyed_service',
 
@@ -192,6 +226,7 @@
                 'components.gyp:web_modal_test_support',
               ],
             }, { # 'OS == "ios"'
+              'includes': ['../chrome/chrome_ios_bundle_resources.gypi'],
               'sources/': [
                 ['exclude', '\\.cc$'],
                 ['exclude', '\\.mm$'],
@@ -213,6 +248,18 @@
                 # then re-enable this test. http://crbug.com/341429
                 ['exclude',
'^password_manager/core/browser/login_database_unittest.cc'],
               ],
+              'actions': [
+                {
+                  'action_name': 'copy_test_data',
+                  'variables': {
+                    'test_data_files': [
+                      'test/data',
+                    ],
+                    'test_data_prefix': 'components',
+                  },
+                  'includes': [ '../build/copy_test_data_ios.gypi' ],
+                },
+              ],
               'conditions': [
                 ['configuration_policy==1', {
                   'sources/': [
@@ -236,6 +283,12 @@
               ],
             }],
             ['OS == "mac"', {
+              'dependencies': [
+                # TODO(blundell): Eliminate this dependency by having
+                # ./test/run_all_unittests.cc avoid using the //chrome
+                # constant to get the framework name on OS X. crbug.com/348563
+                '../chrome/chrome.gyp:common',
+              ],
               'link_settings': {
                 'libraries': [
                   '$(SDKROOT)/System/Library/Frameworks/AddressBook.framework',

[blundell, 2014-03-03 20:55:20]

TBR=ben@ for the addition of //ui in //components/test/DEPS

[blundell, 2014-03-03 20:55:28]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2014-03-03 20:56:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/blundell@chromium.org/172473002/620001

[Ben Goodger (Google), 2014-03-03 21:03:14]

lgtm

[commit-bot: I haz the power, 2014-03-03 23:05:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-03 23:05:15]

Retried try job too often on win_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[blundell, 2014-03-04 10:11:42]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2014-03-04 10:24:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/blundell@chromium.org/172473002/620001

[commit-bot: I haz the power, 2014-03-04 11:22:50]

Change committed as 254745

[tfarina, 2014-03-05 18:28:21]

After-the-fact review.

I think this needs a follow-up.

https://codereview.chromium.org/172473002/diff/620001/components/test/run_all...
File components/test/run_all_unittests.cc (right):

https://codereview.chromium.org/172473002/diff/620001/components/test/run_all...
components/test/run_all_unittests.cc:55: PathService::Get(base::DIR_EXE, &path);
tiny-nit: I'd name this exe_path.

https://codereview.chromium.org/172473002/diff/620001/components/test/run_all...
components/test/run_all_unittests.cc:60:
base::mac::SetOverrideFrameworkBundlePath(path);
nit:

base::mac::SetOverrideFrameworkBundlePath(
  path.AppendASCII(chrome::kFrameworkName));

https://codereview.chromium.org/172473002/diff/620001/components/test/run_all...
components/test/run_all_unittests.cc:63: ui::RegisterPathProvider();
this is to register ui::DIR_LOCALES, ui::DIR_TEST_DATA and
ui::DIR_RESOURCE_PAKS_ANDROID.

Why you call this here?

https://codereview.chromium.org/172473002/diff/620001/components/test/run_all...
components/test/run_all_unittests.cc:77: base::TestSuite::Shutdown();
you also need to add the following here:

#if defined(OS_MACOSX) && !defined(OS_IOS)
  base::mac::SetOverrideFrameworkBundle(NULL);
#endif

[blundell, 2014-04-29 15:31:43]

On 2014/03/05 18:28:21, tfarina wrote:
> After-the-fact review.
> 
> I think this needs a follow-up.
> 
>
https://codereview.chromium.org/172473002/diff/620001/components/test/run_all...
> File components/test/run_all_unittests.cc (right):
> 
>
https://codereview.chromium.org/172473002/diff/620001/components/test/run_all...
> components/test/run_all_unittests.cc:55: PathService::Get(base::DIR_EXE,
&path);
> tiny-nit: I'd name this exe_path.
> 
>
https://codereview.chromium.org/172473002/diff/620001/components/test/run_all...
> components/test/run_all_unittests.cc:60:
> base::mac::SetOverrideFrameworkBundlePath(path);
> nit:
> 
> base::mac::SetOverrideFrameworkBundlePath(
>   path.AppendASCII(chrome::kFrameworkName));
> 
>
https://codereview.chromium.org/172473002/diff/620001/components/test/run_all...
> components/test/run_all_unittests.cc:63: ui::RegisterPathProvider();
> this is to register ui::DIR_LOCALES, ui::DIR_TEST_DATA and
> ui::DIR_RESOURCE_PAKS_ANDROID.
> 
> Why you call this here?
> 
>
https://codereview.chromium.org/172473002/diff/620001/components/test/run_all...
> components/test/run_all_unittests.cc:77: base::TestSuite::Shutdown();
> you also need to add the following here:
> 
> #if defined(OS_MACOSX) && !defined(OS_IOS)
>   base::mac::SetOverrideFrameworkBundle(NULL);
> #endif

I'm addressing these comments as part of
https://codereview.chromium.org/258043003/. Thanks!