[Autofill] Rewrite Autofill unitttests to use INSTANTIATE_TEST_CASE_P

Rewrite Autofill unitttests under components to use INSTANTIATE_TEST_CASE_P

Many tests under component are like this:
const struct {
    const char* input;
    bool expected_result;
  } kTestCases[] = {
      {"ab", true},
      {"abc", true},
      {"abde", true}
  };

  for (const auto& test_case : kTestCases) {
    EXPECT_EQ(test_case.expected_result,
              SomeFunction(test_case.input));
  }

They should be rewritten using INSTANTIATE_TEST_CASE_P, it will create separate test cases for each testing input given. 

BUG=700512
TEST=components_unittests

Review-Url: https://codereview.chromium.org/2744933004
Cr-Commit-Position: refs/heads/master@{#456843}
Committed: https://chromium.googlesource.com/chromium/src/+/23ff72c5f2fc95e16f8eb996767d614976605193

[wuandy, 2017-03-12 04:04:49]

The CQ bit was checked by wuandy@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-12 04:04:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-12 04:36:55]

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

[commit-bot: I haz the power, 2017-03-12 04:36:56]

Dry run: This issue passed the CQ dry run.

[wuandy, 2017-03-13 03:58:37]

The CQ bit was checked by wuandy@google.com to run a CQ dry run

[wuandy, 2017-03-13 03:58:46]

Description was changed from

==========
rewrite autofill_util_unittest.cc to use INSTANTIATE_TEST_CASE_P

BUG=
==========

to

==========
rewrite autofill_util_unittest.cc to use INSTANTIATE_TEST_CASE_P

Many tests under component are like this:
const struct {
    const char* input;
    bool expected_result;
  } kTestCases[] = {
      {"ab", true},
      {"abc", true},
      {"abde", true}
  };

  for (const auto& test_case : kTestCases) {
    EXPECT_EQ(test_case.expected_result,
              SomeFunction(test_case.input));
  }

They should be rewritten using INSTANTIATE_TEST_CASE_P, it will create separate
test cases for each testing input given. 

BUG=700512
==========

[wuandy, 2017-03-13 03:58:46]

wuandy@google.com changed reviewers:
+ mathp@chromium.org

[commit-bot: I haz the power, 2017-03-13 03:58:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-13 04:33:20]

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

[commit-bot: I haz the power, 2017-03-13 04:33:21]

Dry run: This issue passed the CQ dry run.

[wuandy, 2017-03-13 13:33:56]

Description was changed from

==========
rewrite autofill_util_unittest.cc to use INSTANTIATE_TEST_CASE_P

Many tests under component are like this:
const struct {
    const char* input;
    bool expected_result;
  } kTestCases[] = {
      {"ab", true},
      {"abc", true},
      {"abde", true}
  };

  for (const auto& test_case : kTestCases) {
    EXPECT_EQ(test_case.expected_result,
              SomeFunction(test_case.input));
  }

They should be rewritten using INSTANTIATE_TEST_CASE_P, it will create separate
test cases for each testing input given. 

BUG=700512
==========

to

==========
rewrite unittest under components to use INSTANTIATE_TEST_CASE_P

Many tests under component are like this:
const struct {
    const char* input;
    bool expected_result;
  } kTestCases[] = {
      {"ab", true},
      {"abc", true},
      {"abde", true}
  };

  for (const auto& test_case : kTestCases) {
    EXPECT_EQ(test_case.expected_result,
              SomeFunction(test_case.input));
  }

They should be rewritten using INSTANTIATE_TEST_CASE_P, it will create separate
test cases for each testing input given. 

BUG=700512
==========

[sebsg, 2017-03-13 14:56:10]

sebsg@chromium.org changed reviewers:
+ sebsg@chromium.org

[sebsg, 2017-03-13 14:56:11]

Nice!

Just a couple comments. Could you add a constructor for the struct when there
are multiple members? I saw you did it in a couple of places.

Also make sure to append "Test" at the end of the test case name (mentioned it
inline).

Thanks :)

https://codereview.chromium.org/2744933004/diff/40001/components/autofill/cor...
File components/autofill/core/browser/autofill_data_util_unittest.cc (right):

https://codereview.chromium.org/2744933004/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_data_util_unittest.cc:28:
AutofillDataUtil,
AutofillDataUtil -> AutofilDataUtilTest

Thanks!

https://codereview.chromium.org/2744933004/diff/40001/components/autofill/cor...
File components/autofill/core/browser/autofill_field_unittest.cc (right):

https://codereview.chromium.org/2744933004/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_field_unittest.cc:313: AutofillField,
Could you replace by "AutofillFieldTest" here and in the rest of the file?

https://codereview.chromium.org/2744933004/diff/40001/components/autofill/cor...
File
components/autofill/core/browser/webdata/autofill_profile_syncable_service_unittest.cc
(right):

https://codereview.chromium.org/2744933004/diff/40001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_profile_syncable_service_unittest.cc:1195:
size_t local_use_count;
Could you add a constructor here too?

Thanks!

https://codereview.chromium.org/2744933004/diff/40001/components/autofill/cor...
File components/autofill/core/browser/webdata/autofill_table_unittest.cc
(right):

https://codereview.chromium.org/2744933004/diff/40001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table_unittest.cc:1960: const
char* const field_suggestion[kMaxCount];
Could you add a constructor here too? You did it in a couple other files and I
think it's good.

Thank you!

https://codereview.chromium.org/2744933004/diff/40001/components/autofill/cor...
File components/autofill/core/common/autofill_util_unittest.cc (right):

https://codereview.chromium.org/2744933004/diff/40001/components/autofill/cor...
components/autofill/core/common/autofill_util_unittest.cc:63: AutofillUtil,
AutofillUtilTest here and in the rest of the file.

Thanks!

[sebsg, 2017-03-13 15:00:50]

Also, could you do the same thing for tests in credit_card_unittest.cc and
personal_data_manager_unittest.cc?

Very appreciated :)

[sebsg, 2017-03-13 15:00:51]

Also, could you do the same thing for tests in credit_card_unittest.cc and
personal_data_manager_unittest.cc?

Very appreciated :)

[wuandy, 2017-03-13 18:12:10]

The CQ bit was checked by wuandy@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-13 18:12:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-13 18:27:12]

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

[commit-bot: I haz the power, 2017-03-13 18:27:13]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[wuandy, 2017-03-13 18:33:39]

The CQ bit was checked by wuandy@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-13 18:34:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-13 18:46:32]

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

[commit-bot: I haz the power, 2017-03-13 18:46:33]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[wuandy, 2017-03-13 19:31:00]

The CQ bit was checked by wuandy@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-13 19:32:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[wuandy, 2017-03-13 19:47:29]

On 2017/03/13 15:00:51, sebsg wrote:
> Also, could you do the same thing for tests in credit_card_unittest.cc and
> personal_data_manager_unittest.cc?
> 
> Very appreciated :)

Added those.

[commit-bot: I haz the power, 2017-03-13 19:49:23]

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

[commit-bot: I haz the power, 2017-03-13 19:49:24]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[wuandy, 2017-03-13 20:02:41]

windows builds are failing for some reason..not sure if this is related to my
patch set.

[wuandy, 2017-03-13 22:13:15]

The CQ bit was checked by wuandy@google.com to run a CQ dry run

[wuandy, 2017-03-13 22:13:52]

The CQ bit was checked by wuandy@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-13 22:14:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-13 22:36:07]

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

[commit-bot: I haz the power, 2017-03-13 22:36:08]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Mathieu, 2017-03-14 00:08:28]

Thanks Andy, wow I didn't know there were so many! 

Since Seb already started the review (Thanks!) I'll let him finish and move
myself to Cc. The code looks great

Thanks again

[Mathieu, 2017-03-14 00:09:06]

Description was changed from

==========
rewrite unittest under components to use INSTANTIATE_TEST_CASE_P

Many tests under component are like this:
const struct {
    const char* input;
    bool expected_result;
  } kTestCases[] = {
      {"ab", true},
      {"abc", true},
      {"abde", true}
  };

  for (const auto& test_case : kTestCases) {
    EXPECT_EQ(test_case.expected_result,
              SomeFunction(test_case.input));
  }

They should be rewritten using INSTANTIATE_TEST_CASE_P, it will create separate
test cases for each testing input given. 

BUG=700512
==========

to

==========
Rewrite Autofill unitttests under components to use INSTANTIATE_TEST_CASE_P

Many tests under component are like this:
const struct {
    const char* input;
    bool expected_result;
  } kTestCases[] = {
      {"ab", true},
      {"abc", true},
      {"abde", true}
  };

  for (const auto& test_case : kTestCases) {
    EXPECT_EQ(test_case.expected_result,
              SomeFunction(test_case.input));
  }

They should be rewritten using INSTANTIATE_TEST_CASE_P, it will create separate
test cases for each testing input given. 

BUG=700512
TEST=components_unittests
==========

[wuandy, 2017-03-14 18:59:58]

The CQ bit was checked by wuandy@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 19:00:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[wuandy, 2017-03-14 19:05:36]

commented out test failing to compile on windows and addressed other comments.

https://codereview.chromium.org/2744933004/diff/40001/components/autofill/cor...
File components/autofill/core/browser/autofill_data_util_unittest.cc (right):

https://codereview.chromium.org/2744933004/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_data_util_unittest.cc:28:
AutofillDataUtil,
On 2017/03/13 14:56:10, sebsg wrote:
> AutofillDataUtil -> AutofilDataUtilTest
> 
> Thanks!

Done.

https://codereview.chromium.org/2744933004/diff/40001/components/autofill/cor...
File components/autofill/core/browser/autofill_field_unittest.cc (right):

https://codereview.chromium.org/2744933004/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_field_unittest.cc:313: AutofillField,
On 2017/03/13 14:56:10, sebsg wrote:
> Could you replace by "AutofillFieldTest" here and in the rest of the file?

Done.

https://codereview.chromium.org/2744933004/diff/40001/components/autofill/cor...
File
components/autofill/core/browser/webdata/autofill_profile_syncable_service_unittest.cc
(right):

https://codereview.chromium.org/2744933004/diff/40001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_profile_syncable_service_unittest.cc:1195:
size_t local_use_count;
On 2017/03/13 14:56:10, sebsg wrote:
> Could you add a constructor here too?
> 
> Thanks!

As discussed offline, i removed constructor from autofill_util_unittest.cc
instead.

https://codereview.chromium.org/2744933004/diff/40001/components/autofill/cor...
File components/autofill/core/browser/webdata/autofill_table_unittest.cc
(right):

https://codereview.chromium.org/2744933004/diff/40001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table_unittest.cc:1960: const
char* const field_suggestion[kMaxCount];
On 2017/03/13 14:56:10, sebsg wrote:
> Could you add a constructor here too? You did it in a couple other files and I
> think it's good.
> 
> Thank you!

As discussed offline, i removed constructor from autofill_util_unittest.cc
instead.

https://codereview.chromium.org/2744933004/diff/40001/components/autofill/cor...
File components/autofill/core/common/autofill_util_unittest.cc (right):

https://codereview.chromium.org/2744933004/diff/40001/components/autofill/cor...
components/autofill/core/common/autofill_util_unittest.cc:63: AutofillUtil,
On 2017/03/13 14:56:11, sebsg wrote:
> AutofillUtilTest here and in the rest of the file.
> 
> Thanks!

Done.

[sebsg, 2017-03-14 19:15:24]

One last comment! Almost there :)

https://codereview.chromium.org/2744933004/diff/120001/components/autofill/co...
File components/autofill/core/browser/credit_card_unittest.cc (right):

https://codereview.chromium.org/2744933004/diff/120001/components/autofill/co...
components/autofill/core/browser/credit_card_unittest.cc:1003: // TODO(wuandy):
rewriting this test with INSTANTIATE_TEST_CASE_P seems to
Would you mind removing this from the file? Like saving it in a doc somewhere?
We don't usually keep big comments in the code like this, even if just for a few
days :P

[wuandy, 2017-03-14 19:27:45]

https://codereview.chromium.org/2744933004/diff/120001/components/autofill/co...
File components/autofill/core/browser/credit_card_unittest.cc (right):

https://codereview.chromium.org/2744933004/diff/120001/components/autofill/co...
components/autofill/core/browser/credit_card_unittest.cc:1003: // TODO(wuandy):
rewriting this test with INSTANTIATE_TEST_CASE_P seems to
On 2017/03/14 19:15:24, sebsg wrote:
> Would you mind removing this from the file? Like saving it in a doc somewhere?
> We don't usually keep big comments in the code like this, even if just for a
few
> days :P

Done.

[sebsg, 2017-03-14 20:01:18]

LGTM!

[wuandy, 2017-03-14 20:07:55]

The CQ bit was checked by wuandy@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 20:08:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-14 21:20:10]

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

[commit-bot: I haz the power, 2017-03-14 21:20:11]

Dry run: This issue passed the CQ dry run.

[wuandy, 2017-03-14 21:29:13]

The CQ bit was checked by wuandy@google.com

[commit-bot: I haz the power, 2017-03-14 21:29:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-14 21:41:06]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1489526953361320,
"parent_rev": "d7bfcc3fa626adb9876f2d0ef925131decd70aa8", "commit_rev":
"23ff72c5f2fc95e16f8eb996767d614976605193"}

[commit-bot: I haz the power, 2017-03-14 21:41:49]

Description was changed from

==========
Rewrite Autofill unitttests under components to use INSTANTIATE_TEST_CASE_P

Many tests under component are like this:
const struct {
    const char* input;
    bool expected_result;
  } kTestCases[] = {
      {"ab", true},
      {"abc", true},
      {"abde", true}
  };

  for (const auto& test_case : kTestCases) {
    EXPECT_EQ(test_case.expected_result,
              SomeFunction(test_case.input));
  }

They should be rewritten using INSTANTIATE_TEST_CASE_P, it will create separate
test cases for each testing input given. 

BUG=700512
TEST=components_unittests
==========

to

==========
Rewrite Autofill unitttests under components to use INSTANTIATE_TEST_CASE_P

Many tests under component are like this:
const struct {
    const char* input;
    bool expected_result;
  } kTestCases[] = {
      {"ab", true},
      {"abc", true},
      {"abde", true}
  };

  for (const auto& test_case : kTestCases) {
    EXPECT_EQ(test_case.expected_result,
              SomeFunction(test_case.input));
  }

They should be rewritten using INSTANTIATE_TEST_CASE_P, it will create separate
test cases for each testing input given. 

BUG=700512
TEST=components_unittests

Review-Url: https://codereview.chromium.org/2744933004
Cr-Commit-Position: refs/heads/master@{#456843}
Committed:
https://chromium.googlesource.com/chromium/src/+/23ff72c5f2fc95e16f8eb996767d...
==========

[commit-bot: I haz the power, 2017-03-14 21:41:54]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/23ff72c5f2fc95e16f8eb996767d...