Convert the autofill pyauto tests to browser tests, and remove all the supporting automation hooks …

Convert the autofill pyauto tests to browser tests, and remove all the supporting automation hooks that are no longer necessary.

Tests I left:
-AutofillCrowdsourcing: this is a manual, non-automated test against the autofill server. I've been told it's not used, but I left it anyways. It was composed of two parts: testing chrome autofill works and sending the resulting form to the server. The former is tested in many tests already, so I took that out.
-autofill edit address dialog tests: these are using webdriver.

Also remove the autofill tests in sync.py since they used the same automation hooks. testAutofillProfileSync is redundant because many of the tests in two_client_autofill_sync_test.cc do this. For testNoCreditCardSync, I added a test case in two_client_autofill_sync_test.cc.

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

[dyu1, 2012-08-20 21:23:26]

http://codereview.chromium.org/10855253/diff/1/chrome/test/functional/autofil...
File chrome/test/functional/autofill.py (left):

http://codereview.chromium.org/10855253/diff/1/chrome/test/functional/autofil...
chrome/test/functional/autofill.py:581: def FormFillLatencyAfterSubmit(self):
Is this partial test being covered by a browser test?

http://codereview.chromium.org/10855253/diff/1/chrome/test/functional/autofil...
chrome/test/functional/autofill.py:691: def
MergeAggregatedDuplicatedProfiles(self):
Same question here as above.

[jam, 2012-08-20 21:26:37]

http://codereview.chromium.org/10855253/diff/1/chrome/test/functional/autofil...
File chrome/test/functional/autofill.py (left):

http://codereview.chromium.org/10855253/diff/1/chrome/test/functional/autofil...
chrome/test/functional/autofill.py:581: def FormFillLatencyAfterSubmit(self):
On 2012/08/20 21:23:26, dyu1 wrote:
> Is this partial test being covered by a browser test?

yes, FormFillLatencyAfterSubmit. I also automated it. If it crashes or hangs,
the test will fail.

http://codereview.chromium.org/10855253/diff/1/chrome/test/functional/autofil...
chrome/test/functional/autofill.py:691: def
MergeAggregatedDuplicatedProfiles(self):
On 2012/08/20 21:23:26, dyu1 wrote:
> Same question here as above. 

yes MergeAggregatedDuplicatedProfiles

all the tests have been converted unless where I noted in the cl description

[jam, 2012-08-20 23:30:23]

Tim: please take a look at the sync parts (patchset 2->3).

[jam, 2012-08-20 23:50:23]

btw re the test failures: AutofillTest.ComparePhoneNumbers I've fixed (will
upload trivial fix soon, as I'm syncing). The other two are because the data
files aren't being applied properly on the bots because they don't have new
lines at the end.

[tim (not reviewing), 2012-08-21 00:05:08]

sync changes LGTM, +FYI rsimha

[Ilya Sherman, 2012-08-21 03:04:28]

Thanks again for migrating these tests :)

It looks like some of the tests are meant to be testing the WebUI settings page
at chrome://settings/autofill -- those tests will probably need to be part of a
separate browser test suite.  (I'm not actually sure how WebUI code is tested
right now...)

Also, I would prefer if you left the chrome/test/data/autofill test files in a
subdirectory -- that way the existing subdirectories stand out a bit more when
you e.g. 'ls' the directory.

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
File chrome/browser/autofill/autofill_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:97:
content::NotificationService::AllSources());
This doesn't really seem to fit with the rest of the class.  Can you move it out
to a separate WindowedNotificationObserver?

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:136: const
content::NotificationDetails& details) OVERRIDE {
nit: Please add ASSERTs to verify that the type/source of the notification are
the ones you expect.

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:204: typedef
base::hash_map<std::string, std::string> FormData;
FormData is already a type defined in webkit/forms/form_data.h, and is used
somewhat often in Autofill code, so this should be renamed.  Also, why are you
using a base::hash_map rather than an std::map?

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:205: void
FillFormAndSubmit(const std::string filename, const FormData& data) {
nit: Pass the filename by reference

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:205: void
FillFormAndSubmit(const std::string filename, const FormData& data) {
nit: Please add a comment describing this method -- in particular that it blocks
on a change to the PersonalDataManager

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:206: GURL url =
test_server()->GetURL(std::string("files/autofill/") + filename);
nit: No need to explicitly construct a C++ string from "files/autofill"; the
addition operator will take care of that implicitly.

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:239: js += field_id +
"').focus();";
nit: Move the rest of this string definition up one line?  (Looks like it will
fit.)

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:254:
CHECK(test_server()->Start());
If the CHECK fails, the whole test suite will crash, right?  Is that desirable?

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:898: }
I think this test is redundant with tests in
chrome/browser/autofill/personal_data_manager_unittest.cc

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:915: }
Ditto.

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:1014: }
If this test is not redundant with existing PersonalDataManagerTest.* tests, it
should probably be moved into that test suite.  It doesn't look like this needs
to be a browser test.

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:1036: }
I think this test is redundant with tests in
chrome/browser/autofill/personal_data_manager_unittest.cc

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:1038: // Test invalid credit
card numbers typed in prefs should be saved as-is.
This test is meant to be testing the settings page at
chrome://chrome/settings/autofill.  In fact, maybe all of the above tests that I
mentioned are probably already unit tested are actually supposed to be testing
the webui settings.  Right now, the test is skipping all the WebUI JavaScript
and handlers, which rather defeats the point.

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:1147: SetProfile(profile);
Optional nit: Perhaps re-use CreateTestProfile(), and add a country code to that
profile?

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:1181: ASSERT_EQ(i->second,
value);
Can you use ExpectFieldValue() for the body of this loop?  (In fact, you could
probably omit the map entirely, since you could just have each line above be a
call to ExpectFieldValue())

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:1239: ASSERT_EQ(2u,
personal_data_manager()->profiles().size());
Please add checks to verify that the correct profiles are getting saved.

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:1312: ASSERT_NE(email,
readonly_field_value);
nit: ExpectFieldValue(), and compare to the field's original value?  (We
shouldn't partially overwrite or clear a read-only field.)

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:1319: ASSERT_EQ(addr_line1,
addr1_field_value);
nit: ExpectFieldValue()?

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:1337: SetProfile(profile);
nit: CreateTestProfile()?

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:1376: SetProfile(profile);
nit: CreateTestProfile()?

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:1389: ASSERT_EQ("L",
middleinit_field_value);
nit: ExpectFieldValue()?

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:1393: // Entire form should be
filled with one user gesture.
This description makes me think that we should be checking the values of all of
the fields in the form, not just one of the fields.

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:1404: SetProfile(profile);
nit: CreateTestProfile()?

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:1417: ASSERT_EQ(email,
js_return_confirmemail_field);
nit: ExpectFieldValue()?

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:1480: (base::Time::Now() -
start_time).InSeconds() << " seconds.";
nit: Passing tests should be silent.  Is this log line important to the test, or
can it be removed?

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:1495: load_stop_observer.Wait();
I'd expect to see some EXPECT or ASSERT here verifying that the form submission
is reasonably fast, since that's what the test description says it's testing. 
Are you just expecting that the regular test timeout will cover this?  If so,
please add a comment indicating that.

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:1525:
static_cast<int>(personal_data_manager()->profiles().size()));
This looks like it's only testing that we throw away or merge at least one
profile, which is a weaker test than the above ones.  Is this test just
redundant with those?  If so, let's get rid of it.

[jam, 2012-08-21 06:20:06]

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
File chrome/browser/autofill/autofill_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:97:
content::NotificationService::AllSources());
On 2012/08/21 03:04:28, Ilya Sherman wrote:
> This doesn't really seem to fit with the rest of the class.  Can you move it
out
> to a separate WindowedNotificationObserver?

I had kept it like because there didn't seem to be a good reason of having yet
another PDMObserver. If another one was created, it would have to duplicate the
existing class. The (deleted) automation C++ code had one class for this stuff
btw. As such, I don't think there's a compelling reason to split it up.

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:136: const
content::NotificationDetails& details) OVERRIDE {
On 2012/08/21 03:04:28, Ilya Sherman wrote:
> nit: Please add ASSERTs to verify that the type/source of the notification are
> the ones you expect.

I think this is unnecessary at this point.. if someone adds another
notification, they will have to change this code. until then, an assert is just
verifying that the notification system works, which should be tested in one test
instead of in all

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:204: typedef
base::hash_map<std::string, std::string> FormData;
On 2012/08/21 03:04:28, Ilya Sherman wrote:
> FormData is already a type defined in webkit/forms/form_data.h, and is used
> somewhat often in Autofill code, so this should be renamed.

renamed to FormMap

>  Also, why are you using a base::hash_map rather than an std::map?

no reason in particular. switched to map

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:205: void
FillFormAndSubmit(const std::string filename, const FormData& data) {
On 2012/08/21 03:04:28, Ilya Sherman wrote:
> nit: Pass the filename by reference

Done.

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:205: void
FillFormAndSubmit(const std::string filename, const FormData& data) {
On 2012/08/21 03:04:28, Ilya Sherman wrote:
> nit: Please add a comment describing this method -- in particular that it
blocks
> on a change to the PersonalDataManager

Done.

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:206: GURL url =
test_server()->GetURL(std::string("files/autofill/") + filename);
On 2012/08/21 03:04:28, Ilya Sherman wrote:
> nit: No need to explicitly construct a C++ string from "files/autofill"; the
> addition operator will take care of that implicitly.

Done.

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:239: js += field_id +
"').focus();";
On 2012/08/21 03:04:28, Ilya Sherman wrote:
> nit: Move the rest of this string definition up one line?  (Looks like it will
> fit.)

Done.

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:254:
CHECK(test_server()->Start());
On 2012/08/21 03:04:28, Ilya Sherman wrote:
> If the CHECK fails, the whole test suite will crash, right?  Is that
desirable?

yep & yep. if the test sever can't be started, then there's no point running the
rest of the test. I would normally add ASSERT(), which does the same thing, but
that can't be used in functions that have return values since it does an early
return

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:898: }
On 2012/08/21 03:04:28, Ilya Sherman wrote:
> I think this test is redundant with tests in
> chrome/browser/autofill/personal_data_manager_unittest.cc

I have ported all the tests and tried to do a straight translation so it would
be less contentious (i.e. I did not go through the history of every test to see
if it was written as a regression test, or for other reasons). I don't know
enough about autofill to know if it's redundant with other unittests, or if
there's value in testing this in a browser test as opposed to a unit test.

if you feel strongly that we should remove this test, I'm ok with that.

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:1014: }
On 2012/08/21 03:04:28, Ilya Sherman wrote:
> If this test is not redundant with existing PersonalDataManagerTest.* tests,
it
> should probably be moved into that test suite.

I don't know all the tests there. if you find one that this is redundant with,
let me know and I can delete it.

>  It doesn't look like this needs
> to be a browser test.

Again, I don't know if there's value in testing this through an actual browser
or not. Since I don't know which tests are redundant, or unnecessary as a
browser test, I had opted for a straight translation.

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:1036: }
On 2012/08/21 03:04:28, Ilya Sherman wrote:
> I think this test is redundant with tests in
> chrome/browser/autofill/personal_data_manager_unittest.cc

ditto

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:1038: // Test invalid credit
card numbers typed in prefs should be saved as-is.
On 2012/08/21 03:04:28, Ilya Sherman wrote:
> This test is meant to be testing the settings page at
> chrome://chrome/settings/autofill.  In fact, maybe all of the above tests that
I
> mentioned are probably already unit tested are actually supposed to be testing
> the webui settings.  Right now, the test is skipping all the WebUI JavaScript
> and handlers, which rather defeats the point.

I think there is webui js harness already that is meant to test settings page,
so it seems tests can use that.

Again, I was aiming for a straight translation. If you think this is redundant
with other existing tests, then I'm happy to remove it.

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:1147: SetProfile(profile);
On 2012/08/21 03:04:28, Ilya Sherman wrote:
> Optional nit: Perhaps re-use CreateTestProfile(), and add a country code to
that
> profile?

I didn't want to change the previous autofill tests..

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:1181: ASSERT_EQ(i->second,
value);
On 2012/08/21 03:04:28, Ilya Sherman wrote:
> Can you use ExpectFieldValue() for the body of this loop?  (In fact, you could
> probably omit the map entirely, since you could just have each line above be a
> call to ExpectFieldValue())

Thanks, I didn't see that function before. also fixed below

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:1239: ASSERT_EQ(2u,
personal_data_manager()->profiles().size());
On 2012/08/21 03:04:28, Ilya Sherman wrote:
> Please add checks to verify that the correct profiles are getting saved.

Done.

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:1312: ASSERT_NE(email,
readonly_field_value);
On 2012/08/21 03:04:28, Ilya Sherman wrote:
> nit: ExpectFieldValue(), and compare to the field's original value?  (We
> shouldn't partially overwrite or clear a read-only field.)

this is ASSERT_NE

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:1319: ASSERT_EQ(addr_line1,
addr1_field_value);
On 2012/08/21 03:04:28, Ilya Sherman wrote:
> nit: ExpectFieldValue()?

Done.

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:1337: SetProfile(profile);
On 2012/08/21 03:04:28, Ilya Sherman wrote:
> nit: CreateTestProfile()?

Done.

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:1376: SetProfile(profile);
On 2012/08/21 03:04:28, Ilya Sherman wrote:
> nit: CreateTestProfile()?

Done.

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:1389: ASSERT_EQ("L",
middleinit_field_value);
On 2012/08/21 03:04:28, Ilya Sherman wrote:
> nit: ExpectFieldValue()?

Done.

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:1393: // Entire form should be
filled with one user gesture.
On 2012/08/21 03:04:28, Ilya Sherman wrote:
> This description makes me think that we should be checking the values of all
of
> the fields in the form, not just one of the fields.

please feel free to improve the tests in followup change. my goal was to
translate them so we can remove the automation hooks.

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:1417: ASSERT_EQ(email,
js_return_confirmemail_field);
On 2012/08/21 03:04:28, Ilya Sherman wrote:
> nit: ExpectFieldValue()?

Done.

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:1480: (base::Time::Now() -
start_time).InSeconds() << " seconds.";
On 2012/08/21 03:04:28, Ilya Sherman wrote:
> nit: Passing tests should be silent.  Is this log line important to the test,
or
> can it be removed?

I think one line is ok, at least in the beginning so that we know how long this
step takes on the slower bots (i.e. if it causes flakiness because it takes too
long).

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:1495: load_stop_observer.Wait();
On 2012/08/21 03:04:28, Ilya Sherman wrote:
> I'd expect to see some EXPECT or ASSERT here verifying that the form
submission
> is reasonably fast, since that's what the test description says it's testing. 
> Are you just expecting that the regular test timeout will cover this?  If so,
> please add a comment indicating that.

right. the comment said a browser hang. so this will detect it.

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:1525:
static_cast<int>(personal_data_manager()->profiles().size()));
On 2012/08/21 03:04:28, Ilya Sherman wrote:
> This looks like it's only testing that we throw away or merge at least one
> profile, which is a weaker test than the above ones.  Is this test just
> redundant with those?  If so, let's get rid of it.

again, I don't know enough about autofill to know if I should throw this out or
not. if you think we can delete this, it's fine with me.

[jam, 2012-08-21 06:33:00]

On 2012/08/21 03:04:28, Ilya Sherman wrote:
> Thanks again for migrating these tests :)
> 
> It looks like some of the tests are meant to be testing the WebUI settings
page
> at chrome://settings/autofill -- those tests will probably need to be part of
a
> separate browser test suite.  (I'm not actually sure how WebUI code is tested
> right now...)
> 
> Also, I would prefer if you left the chrome/test/data/autofill test files in a
> subdirectory -- that way the existing subdirectories stand out a bit more when
> you e.g. 'ls' the directory.

really? it seems very obvious what are directories or not, and most
subdirectories of test\data are like this. adding another layer makes the
browser test utilities more cumbersome, since they're suited for
chrome\test\data\feature\test_filename.html

[Ilya Sherman, 2012-08-21 07:24:16]

LGTM

On 2012/08/21 06:33:00, John Abd-El-Malek wrote:
> On 2012/08/21 03:04:28, Ilya Sherman wrote:
> > Thanks again for migrating these tests :)
> > 
> > It looks like some of the tests are meant to be testing the WebUI settings
> page
> > at chrome://settings/autofill -- those tests will probably need to be part
of
> a
> > separate browser test suite.  (I'm not actually sure how WebUI code is
tested
> > right now...)
> > 
> > Also, I would prefer if you left the chrome/test/data/autofill test files in
a
> > subdirectory -- that way the existing subdirectories stand out a bit more
when
> > you e.g. 'ls' the directory.
> 
> really? it seems very obvious what are directories or not, and most
> subdirectories of test\data are like this. adding another layer makes the
> browser test utilities more cumbersome, since they're suited for
> chrome\test\data\feature\test_filename.html

Meh, just a personal preference.  I don't feel strongly about it if most other
directories are set up this way.

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
File chrome/browser/autofill/autofill_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:898: }
On 2012/08/21 06:20:06, John Abd-El-Malek wrote:
> On 2012/08/21 03:04:28, Ilya Sherman wrote:
> > I think this test is redundant with tests in
> > chrome/browser/autofill/personal_data_manager_unittest.cc
> 
> I have ported all the tests and tried to do a straight translation so it would
> be less contentious (i.e. I did not go through the history of every test to
see
> if it was written as a regression test, or for other reasons). I don't know
> enough about autofill to know if it's redundant with other unittests, or if
> there's value in testing this in a browser test as opposed to a unit test.
> 
> if you feel strongly that we should remove this test, I'm ok with that.

This test is redundant with the unit test
PersonalDataManagerTest.AddProfilesAndCreditCards, and hence should be removed.

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:1014: }
On 2012/08/21 06:20:06, John Abd-El-Malek wrote:
> On 2012/08/21 03:04:28, Ilya Sherman wrote:
> > If this test is not redundant with existing PersonalDataManagerTest.* tests,
> it
> > should probably be moved into that test suite.
> 
> I don't know all the tests there. if you find one that this is redundant with,
> let me know and I can delete it.
> 
> >  It doesn't look like this needs
> > to be a browser test.
> 
> Again, I don't know if there's value in testing this through an actual browser
> or not. Since I don't know which tests are redundant, or unnecessary as a
> browser test, I had opted for a straight translation.

This test does not need to be a browser_test; it can be a regular unit test.  If
you don't want to move it over as part of this CL, please add a TODO(isherman):
to move this over to be a unit test under PersonalDataManagerTest.

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:1036: }
On 2012/08/21 06:20:06, John Abd-El-Malek wrote:
> On 2012/08/21 03:04:28, Ilya Sherman wrote:
> > I think this test is redundant with tests in
> > chrome/browser/autofill/personal_data_manager_unittest.cc
> 
> ditto

This test is meant to be a test of the WebUI at chrome://settings/autofill. 
Admittedly, that is not actually what the PyAuto code was doing.  If you don't
want to convert this over to being a WebUI test right now, please file a bug for
converting it.

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:1038: // Test invalid credit
card numbers typed in prefs should be saved as-is.
On 2012/08/21 06:20:06, John Abd-El-Malek wrote:
> On 2012/08/21 03:04:28, Ilya Sherman wrote:
> > This test is meant to be testing the settings page at
> > chrome://chrome/settings/autofill.  In fact, maybe all of the above tests
that
> I
> > mentioned are probably already unit tested are actually supposed to be
testing
> > the webui settings.  Right now, the test is skipping all the WebUI
JavaScript
> > and handlers, which rather defeats the point.
> 
> I think there is webui js harness already that is meant to test settings page,
> so it seems tests can use that.
> 
> Again, I was aiming for a straight translation. If you think this is redundant
> with other existing tests, then I'm happy to remove it.

Ditto with the above: This test is meant to be a test of the WebUI at
chrome://settings/autofill.  Admittedly, that is not actually what the PyAuto
code was doing.  If you don't want to convert this over to being a WebUI test
right now, please file a bug for converting it.

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:1312: ASSERT_NE(email,
readonly_field_value);
On 2012/08/21 06:20:06, John Abd-El-Malek wrote:
> On 2012/08/21 03:04:28, Ilya Sherman wrote:
> > nit: ExpectFieldValue(), and compare to the field's original value?  (We
> > shouldn't partially overwrite or clear a read-only field.)
> 
> this is ASSERT_NE

Please add a TODO(isherman): to fix this to be an ASSERT_EQ if you don't want to
change this right now.

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:1393: // Entire form should be
filled with one user gesture.
On 2012/08/21 06:20:06, John Abd-El-Malek wrote:
> On 2012/08/21 03:04:28, Ilya Sherman wrote:
> > This description makes me think that we should be checking the values of all
> of
> > the fields in the form, not just one of the fields.
> 
> please feel free to improve the tests in followup change. my goal was to
> translate them so we can remove the automation hooks.

Please add a TODO(isherman): to verify the entire form's contents.

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:1480: (base::Time::Now() -
start_time).InSeconds() << " seconds.";
On 2012/08/21 06:20:06, John Abd-El-Malek wrote:
> On 2012/08/21 03:04:28, Ilya Sherman wrote:
> > nit: Passing tests should be silent.  Is this log line important to the
test,
> or
> > can it be removed?
> 
> I think one line is ok, at least in the beginning so that we know how long
this
> step takes on the slower bots (i.e. if it causes flakiness because it takes
too
> long).

Ok.  In that case, please add a TODO or file a bug to remove this once we've
gathered the info from the bots.

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:1525:
static_cast<int>(personal_data_manager()->profiles().size()));
On 2012/08/21 06:20:06, John Abd-El-Malek wrote:
> On 2012/08/21 03:04:28, Ilya Sherman wrote:
> > This looks like it's only testing that we throw away or merge at least one
> > profile, which is a weaker test than the above ones.  Is this test just
> > redundant with those?  If so, let's get rid of it.
> 
> again, I don't know enough about autofill to know if I should throw this out
or
> not. if you think we can delete this, it's fine with me.

Please add a TODO(isherman): to investigate removing this test, as it is likely
redundant.

https://chromiumcodereview.appspot.com/10855253/diff/13012/chrome/browser/aut...
File chrome/browser/autofill/autofill_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10855253/diff/13012/chrome/browser/aut...
chrome/browser/autofill/autofill_browsertest.cc:915: }
This test is redundant with PersonalDataManagerTest.
AggregateTwoProfilesWithMultiValue, and hence should be removed.

[jam, 2012-08-21 14:49:10]

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
File chrome/browser/autofill/autofill_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:898: }
On 2012/08/21 07:24:17, Ilya Sherman wrote:
> On 2012/08/21 06:20:06, John Abd-El-Malek wrote:
> > On 2012/08/21 03:04:28, Ilya Sherman wrote:
> > > I think this test is redundant with tests in
> > > chrome/browser/autofill/personal_data_manager_unittest.cc
> > 
> > I have ported all the tests and tried to do a straight translation so it
would
> > be less contentious (i.e. I did not go through the history of every test to
> see
> > if it was written as a regression test, or for other reasons). I don't know
> > enough about autofill to know if it's redundant with other unittests, or if
> > there's value in testing this in a browser test as opposed to a unit test.
> > 
> > if you feel strongly that we should remove this test, I'm ok with that.
> 
> This test is redundant with the unit test
> PersonalDataManagerTest.AddProfilesAndCreditCards, and hence should be
removed.

Done.

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:915: }
On 2012/08/21 03:04:28, Ilya Sherman wrote:
> Ditto.

Done.

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:1014: }
On 2012/08/21 07:24:17, Ilya Sherman wrote:
> On 2012/08/21 06:20:06, John Abd-El-Malek wrote:
> > On 2012/08/21 03:04:28, Ilya Sherman wrote:
> > > If this test is not redundant with existing PersonalDataManagerTest.*
tests,
> > it
> > > should probably be moved into that test suite.
> > 
> > I don't know all the tests there. if you find one that this is redundant
with,
> > let me know and I can delete it.
> > 
> > >  It doesn't look like this needs
> > > to be a browser test.
> > 
> > Again, I don't know if there's value in testing this through an actual
browser
> > or not. Since I don't know which tests are redundant, or unnecessary as a
> > browser test, I had opted for a straight translation.
> 
> This test does not need to be a browser_test; it can be a regular unit test. 
If
> you don't want to move it over as part of this CL, please add a
TODO(isherman):
> to move this over to be a unit test under PersonalDataManagerTest.

Done.

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:1036: }
On 2012/08/21 07:24:17, Ilya Sherman wrote:
> On 2012/08/21 06:20:06, John Abd-El-Malek wrote:
> > On 2012/08/21 03:04:28, Ilya Sherman wrote:
> > > I think this test is redundant with tests in
> > > chrome/browser/autofill/personal_data_manager_unittest.cc
> > 
> > ditto
> 
> This test is meant to be a test of the WebUI at chrome://settings/autofill. 
> Admittedly, that is not actually what the PyAuto code was doing.  If you don't
> want to convert this over to being a WebUI test right now, please file a bug
for
> converting it.

Done.

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:1038: // Test invalid credit
card numbers typed in prefs should be saved as-is.
On 2012/08/21 07:24:17, Ilya Sherman wrote:
> On 2012/08/21 06:20:06, John Abd-El-Malek wrote:
> > On 2012/08/21 03:04:28, Ilya Sherman wrote:
> > > This test is meant to be testing the settings page at
> > > chrome://chrome/settings/autofill.  In fact, maybe all of the above tests
> that
> > I
> > > mentioned are probably already unit tested are actually supposed to be
> testing
> > > the webui settings.  Right now, the test is skipping all the WebUI
> JavaScript
> > > and handlers, which rather defeats the point.
> > 
> > I think there is webui js harness already that is meant to test settings
page,
> > so it seems tests can use that.
> > 
> > Again, I was aiming for a straight translation. If you think this is
redundant
> > with other existing tests, then I'm happy to remove it.
> 
> Ditto with the above: This test is meant to be a test of the WebUI at
> chrome://settings/autofill.  Admittedly, that is not actually what the PyAuto
> code was doing.  If you don't want to convert this over to being a WebUI test
> right now, please file a bug for converting it.

Done.

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:1312: ASSERT_NE(email,
readonly_field_value);
On 2012/08/21 07:24:17, Ilya Sherman wrote:
> On 2012/08/21 06:20:06, John Abd-El-Malek wrote:
> > On 2012/08/21 03:04:28, Ilya Sherman wrote:
> > > nit: ExpectFieldValue(), and compare to the field's original value?  (We
> > > shouldn't partially overwrite or clear a read-only field.)
> > 
> > this is ASSERT_NE
> 
> Please add a TODO(isherman): to fix this to be an ASSERT_EQ if you don't want
to
> change this right now.

ah I think I didn't understand you originally. now checking that it's empty.

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:1393: // Entire form should be
filled with one user gesture.
On 2012/08/21 07:24:17, Ilya Sherman wrote:
> On 2012/08/21 06:20:06, John Abd-El-Malek wrote:
> > On 2012/08/21 03:04:28, Ilya Sherman wrote:
> > > This description makes me think that we should be checking the values of
all
> > of
> > > the fields in the form, not just one of the fields.
> > 
> > please feel free to improve the tests in followup change. my goal was to
> > translate them so we can remove the automation hooks.
> 
> Please add a TODO(isherman): to verify the entire form's contents.

Done.

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:1480: (base::Time::Now() -
start_time).InSeconds() << " seconds.";
On 2012/08/21 07:24:17, Ilya Sherman wrote:
> On 2012/08/21 06:20:06, John Abd-El-Malek wrote:
> > On 2012/08/21 03:04:28, Ilya Sherman wrote:
> > > nit: Passing tests should be silent.  Is this log line important to the
> test,
> > or
> > > can it be removed?
> > 
> > I think one line is ok, at least in the beginning so that we know how long
> this
> > step takes on the slower bots (i.e. if it causes flakiness because it takes
> too
> > long).
> 
> Ok.  In that case, please add a TODO or file a bug to remove this once we've
> gathered the info from the bots.

Done.

https://chromiumcodereview.appspot.com/10855253/diff/1045/chrome/browser/auto...
chrome/browser/autofill/autofill_browsertest.cc:1525:
static_cast<int>(personal_data_manager()->profiles().size()));
On 2012/08/21 07:24:17, Ilya Sherman wrote:
> On 2012/08/21 06:20:06, John Abd-El-Malek wrote:
> > On 2012/08/21 03:04:28, Ilya Sherman wrote:
> > > This looks like it's only testing that we throw away or merge at least one
> > > profile, which is a weaker test than the above ones.  Is this test just
> > > redundant with those?  If so, let's get rid of it.
> > 
> > again, I don't know enough about autofill to know if I should throw this out
> or
> > not. if you think we can delete this, it's fine with me.
> 
> Please add a TODO(isherman): to investigate removing this test, as it is
likely
> redundant.

Done.