[Password manager Python tests] Re-arrange tests

[Password manager Python tests] Re-arrange tests

This CL changes the structure of the tests. Instead of many repetitions, there are now just 3 types of tests: failed login, successful login, and autofill test.

The Environment and WebsiteTest classes are also being refactored for clarity.

BUG=369521
Committed: https://crrev.com/d0ed693995e85118a01908df04a61b0b37eaf195
Cr-Commit-Position: refs/heads/master@{#322574}

[vabr (Chromium), 2015-03-24 18:06:30]

Patchset #1 (id:1) has been deleted

[vabr (Chromium), 2015-03-26 19:46:56]

Patchset #1 (id:20001) has been deleted

[vabr (Chromium), 2015-03-26 19:47:02]

Patchset #1 (id:40001) has been deleted

[vabr (Chromium), 2015-03-26 20:11:19]

Patchset #1 (id:60001) has been deleted

[vabr (Chromium), 2015-03-26 20:15:53]

vabr@chromium.org changed reviewers:
+ msramek@chromium.org

[vabr (Chromium), 2015-03-26 20:15:54]

Hi Martin,

I'd be grateful for your review!

To explain -- this is a step in a longer refactoring effort. Instead of
rewriting from scratch, I try to do partial changes keeping the tests runnable
in the meantime. So while there are many obvious places to improve this code
further, I'd like to keep the CL focussed on what is written in the description.

In particular, the structure of the calls between the 4 Python modules is still
unnecessary complex and chaotic. And the naming of the methods is confusing,
contains often plurals (tests, websites, etc.) while it is always called just
for a single entity, etc. This will be rectified in some of the coming changes.

Thanks for your time!
Vaclav

[msramek, 2015-03-27 11:37:07]

LGTM, thanks for taking the time to refactor this.

I am not commenting on the structure now, as you said you're going to improve it
further in following CLs. So I just picked some nits.

https://codereview.chromium.org/1026833003/diff/110001/components/test/data/p...
File components/test/data/password_manager/automated_tests/environment.py
(right):

https://codereview.chromium.org/1026833003/diff/110001/components/test/data/p...
components/test/data/password_manager/automated_tests/environment.py:69:
self.passwords_tree = ElementTree.parse(passwords_path).getroot()
This can throw IOError or ParseError. Why don't we handle the former anymore?
Also, please add to the "Raises:" section of the docstring.

https://codereview.chromium.org/1026833003/diff/110001/components/test/data/p...
components/test/data/password_manager/automated_tests/environment.py:128:
"#clear-data-checkboxes > * > * >[type=\"checkbox\"]")
Nit: Why not just

#clear-data-checkboxes [type="checkbox"] ?

I think the semantics are clearer (all checkboxes in this section) as opposed to
unnecessary structural information (there are two <div>s inbetween).

https://codereview.chromium.org/1026833003/diff/110001/components/test/data/p...
components/test/data/password_manager/automated_tests/environment.py:133: "  if
(checkboxes[i].checked)"
Nit:

checkboxes[i].checked = false

https://codereview.chromium.org/1026833003/diff/110001/components/test/data/p...
components/test/data/password_manager/automated_tests/environment.py:153: "if
(!cbox.checked)"
Nit:

document.querySelector().checked = true

https://codereview.chromium.org/1026833003/diff/110001/components/test/data/p...
components/test/data/password_manager/automated_tests/environment.py:172: "if
(!document.querySelector('#password-manager-enabled').checked)"
Nit: As above.

https://codereview.chromium.org/1026833003/diff/110001/components/test/data/p...
components/test/data/password_manager/automated_tests/environment.py:223: while
timeout:
Why don't we just sleep for |timeout| and then count the strings? Do they
disappear over time?

https://codereview.chromium.org/1026833003/diff/110001/components/test/data/p...
File components/test/data/password_manager/automated_tests/tests.py (right):

https://codereview.chromium.org/1026833003/diff/110001/components/test/data/p...
components/test/data/password_manager/automated_tests/tests.py:530:
environment.RunTestsOnSites(test_type)
I was confused that RunTest() calls RunTest*s*, but then I remembered that you
want to fix plurals in a follow-up CLs. Please do :)

https://codereview.chromium.org/1026833003/diff/110001/components/test/data/p...
File components/test/data/password_manager/automated_tests/websitetest.py
(right):

https://codereview.chromium.org/1026833003/diff/110001/components/test/data/p...
components/test/data/password_manager/automated_tests/websitetest.py:75: What is
clickable defines the driver's find_element_by_css_selector
Nit: is defined by

https://codereview.chromium.org/1026833003/diff/110001/components/test/data/p...
components/test/data/password_manager/automated_tests/websitetest.py:133: The
element if disaplayed, None otherwise.
Nit: *-a

https://codereview.chromium.org/1026833003/diff/110001/components/test/data/p...
components/test/data/password_manager/automated_tests/websitetest.py:169:
Exception in case the accummulated waiting limit is exceeded.
Nit: You usually write "<class>: <desc>", see e.g. line 245.

https://codereview.chromium.org/1026833003/diff/110001/components/test/data/p...
components/test/data/password_manager/automated_tests/websitetest.py:225: " one
on website : %s" % self.name)
Nit: No space in front of the colon here or space below. Also in
FillUsernameInto().

(I don't want to nitpick too much, but this would irk me while reading the logs)

[vabr (Chromium), 2015-03-27 12:14:42]

vabr@chromium.org changed reviewers:
+ engedy@chromium.org

[vabr (Chromium), 2015-03-27 12:14:44]

Balázs: Could you please give a committer rubber-stamp?
Martin: Thanks for your review! Comments addressed.

Cheers,
Vaclav

https://codereview.chromium.org/1026833003/diff/110001/components/test/data/p...
File components/test/data/password_manager/automated_tests/environment.py
(right):

https://codereview.chromium.org/1026833003/diff/110001/components/test/data/p...
components/test/data/password_manager/automated_tests/environment.py:69:
self.passwords_tree = ElementTree.parse(passwords_path).getroot()
On 2015/03/27 11:37:06, msramek wrote:
> This can throw IOError or ParseError. Why don't we handle the former anymore?
This is intentional. The tests cannot be run, and the original error raised by
ElementTree gives enough context to fix that.

> Also, please add to the "Raises:" section of the docstring.
Done.

https://codereview.chromium.org/1026833003/diff/110001/components/test/data/p...
components/test/data/password_manager/automated_tests/environment.py:128:
"#clear-data-checkboxes > * > * >[type=\"checkbox\"]")
On 2015/03/27 11:37:07, msramek wrote:
> Nit: Why not just
> 
> #clear-data-checkboxes [type="checkbox"] ?
> 
> I think the semantics are clearer (all checkboxes in this section) as opposed
to
> unnecessary structural information (there are two <div>s inbetween).

Agreed and done.
(Learning CSS selectors on the fly, thanks for teaching me!)

https://codereview.chromium.org/1026833003/diff/110001/components/test/data/p...
components/test/data/password_manager/automated_tests/environment.py:133: "  if
(checkboxes[i].checked)"
On 2015/03/27 11:37:06, msramek wrote:
> Nit:
> 
> checkboxes[i].checked = false

Done.

https://codereview.chromium.org/1026833003/diff/110001/components/test/data/p...
components/test/data/password_manager/automated_tests/environment.py:153: "if
(!cbox.checked)"
On 2015/03/27 11:37:07, msramek wrote:
> Nit:
> 
> document.querySelector().checked = true

Done.

https://codereview.chromium.org/1026833003/diff/110001/components/test/data/p...
components/test/data/password_manager/automated_tests/environment.py:172: "if
(!document.querySelector('#password-manager-enabled').checked)"
On 2015/03/27 11:37:06, msramek wrote:
> Nit: As above.

Done.

https://codereview.chromium.org/1026833003/diff/110001/components/test/data/p...
components/test/data/password_manager/automated_tests/environment.py:223: while
timeout:
On 2015/03/27 11:37:06, msramek wrote:
> Why don't we just sleep for |timeout| and then count the strings? Do they
> disappear over time?

No, but in the case when they appear, they mostly appear after the first second
or so, so waiting for the whole timeout is wasteful. This delay is particularly
inconvenient when doing interactive testing (i.e., with a person observing the
test, as opposed to the stuff run by cron).

https://codereview.chromium.org/1026833003/diff/110001/components/test/data/p...
File components/test/data/password_manager/automated_tests/tests.py (right):

https://codereview.chromium.org/1026833003/diff/110001/components/test/data/p...
components/test/data/password_manager/automated_tests/tests.py:530:
environment.RunTestsOnSites(test_type)
On 2015/03/27 11:37:07, msramek wrote:
> I was confused that RunTest() calls RunTest*s*, but then I remembered that you
> want to fix plurals in a follow-up CLs. Please do :)

Acknowledged.

The naming is now a horrible mess. Not just the plural vs. singular, but also
"test" means a single scenario, a whole set of them for a single website, or the
whole run for all websites. And wait until we actually get some unittests for
this code! :) I'll chose a non-conflicting nomenclature, and clean all of this
up in a consistent way.

https://codereview.chromium.org/1026833003/diff/110001/components/test/data/p...
File components/test/data/password_manager/automated_tests/websitetest.py
(right):

https://codereview.chromium.org/1026833003/diff/110001/components/test/data/p...
components/test/data/password_manager/automated_tests/websitetest.py:75: What is
clickable defines the driver's find_element_by_css_selector
On 2015/03/27 11:37:07, msramek wrote:
> Nit: is defined by

Tried to simplify even more by reordering the sentence and making the method the
subject.

https://codereview.chromium.org/1026833003/diff/110001/components/test/data/p...
components/test/data/password_manager/automated_tests/websitetest.py:133: The
element if disaplayed, None otherwise.
On 2015/03/27 11:37:07, msramek wrote:
> Nit: *-a

Done.

https://codereview.chromium.org/1026833003/diff/110001/components/test/data/p...
components/test/data/password_manager/automated_tests/websitetest.py:169:
Exception in case the accummulated waiting limit is exceeded.
On 2015/03/27 11:37:07, msramek wrote:
> Nit: You usually write "<class>: <desc>", see e.g. line 245.

Also, I usually write Raises. Such C++-ism! (And Chromium even does not use
exceptions.) I'm ashamed. :)
Done.

https://codereview.chromium.org/1026833003/diff/110001/components/test/data/p...
components/test/data/password_manager/automated_tests/websitetest.py:225: " one
on website : %s" % self.name)
On 2015/03/27 11:37:07, msramek wrote:
> Nit: No space in front of the colon here or space below. Also in
> FillUsernameInto().
> 
> (I don't want to nitpick too much, but this would irk me while reading the
logs)

Done. Fully agree, also fixed this elsewhere! :)

[vabr (Chromium), 2015-03-27 12:46:03]

vabr@chromium.org changed reviewers:
+ mkwst@chromium.org

[vabr (Chromium), 2015-03-27 12:46:03]

Mike: Since Balázs is at a training all afternoon, could you please do a
committer rubberstamp here? Martin has reviewed the CL.

Cheers,
Vaclav

[vabr (Chromium), 2015-03-27 12:46:13]

vabr@chromium.org changed reviewers:
- engedy@chromium.org

[Mike West, 2015-03-27 12:54:21]

The CQ bit was checked by mkwst@chromium.org

[Mike West, 2015-03-27 12:54:22]

The patchset sent to the CQ was uploaded after l-g-t-m from msramek@chromium.org
Link to the patchset: https://codereview.chromium.org/1026833003/#ps130001
(title: "Comments addressed")

[commit-bot: I haz the power, 2015-03-27 12:54:32]

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

[commit-bot: I haz the power, 2015-03-27 12:54:35]

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

[commit-bot: I haz the power, 2015-03-27 12:54:36]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Mike West, 2015-03-27 12:54:53]

On 2015/03/27 at 12:54:32, commit-bot wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1026833003/130001

Hrm. LGTM. The LGTM button didn't LGTM it for some reason.

[vabr (Chromium), 2015-03-27 12:59:08]

On 2015/03/27 12:54:53, Mike West wrote:
> On 2015/03/27 at 12:54:32, commit-bot wrote:
> > CQ is trying da patch. Follow status at
> >  https://chromium-cq-status.appspot.com/patch-status/1026833003/130001
> 
> Hrm. LGTM. The LGTM button didn't LGTM it for some reason.

Thanks, Mike!

Martin, given you approval in #7, I'll send to CQ now.
If you have any new requests, I'll be happy to address them in a follow-up CL.

Cheers,
Vaclav

[vabr (Chromium), 2015-03-27 12:59:12]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2015-03-27 12:59:23]

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

[commit-bot: I haz the power, 2015-03-27 14:21:35]

Committed patchset #4 (id:130001)

[commit-bot: I haz the power, 2015-03-27 14:22:30]

Patchset 4 (id:??) landed as
https://crrev.com/d0ed693995e85118a01908df04a61b0b37eaf195
Cr-Commit-Position: refs/heads/master@{#322574}