Issue 1022703004: [Password manager Python tests] Remove some dead code

Issue 1022703004: [Password manager Python tests] Remove some dead code (Closed)

Created:
5 years, 9 months ago by vabr (Chromium)

Modified:
5 years, 9 months ago

Reviewers:
engedy

CC:
chromium-reviews

Base URL:
https://chromium.googlesource.com/chromium/src.git@369521_remove_disabled_tests

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

[Password manager Python tests] Remove some dead code This CL removes: * Unused support for logging in environment.py. The two current diagnostics messages will be replaced later if needed, after the code gets rearranged. The command-line support for log levels was not used (and not working last time I tried). * Some unused code was removed, including WebsiteTest's SendEnterTo action. * It simplified the way to obtain all tests names -- no longer needed to instantiate Environment. * It removes adding ../../../../third_party/webdriver/pylib/ to sys.path -- this has not worked in most cases and was not needed (webdriver path has been added, from various sources, outside of the scripts in the framework we used). It also removes some part of the unused ability to run all tests, which has recently been simulated by running all tests one by one outside of Python. That was to make the tests run in parallel. This is just a transitional state, and is described in the TODOs. BUG=369521 Committed: https://crrev.com/ab29ef03762e48937e6fa28d692b59cdfcf6bc0b Cr-Commit-Position: refs/heads/master@{#322447}

Patch Set 1 : #

Total comments: 23

Patch Set 2 : Just rebased #

Patch Set 3 : Comments addressed #

Created: 5 years, 9 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+120 lines, -233 lines)			Patch
M	components/test/data/password_manager/automated_tests/README	View		1 chunk	+0 lines, -2 lines	0 comments	Download
M	components/test/data/password_manager/automated_tests/environment.py	View	1 2	8 chunks	+45 lines, -77 lines	0 comments	Download
M	components/test/data/password_manager/automated_tests/run_tests.py	View		3 chunks	+1 line, -4 lines	0 comments	Download
M	components/test/data/password_manager/automated_tests/tests.py	View	1 2	5 chunks	+74 lines, -137 lines	0 comments	Download
M	components/test/data/password_manager/automated_tests/websitetest.py	View		2 chunks	+0 lines, -13 lines	0 comments	Download

Messages

Total messages: 11 (4 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

vabr (Chromium)

Thank you, Balázs, for being willing to have a look! Vaclav

5 years, 9 months ago (2015-03-26 13:26:37 UTC) #4

engedy

LGTM % some minor comments. We should introduce using new-style classes and properties in a ...

5 years, 9 months ago (2015-03-26 16:18:56 UTC) #5

vabr (Chromium)

Thanks, Balázs! I agree with the new-style classes and properties, that's a definite TODO. I'd ...

5 years, 9 months ago (2015-03-26 18:01:50 UTC) #6

Thanks, Balázs!

I agree with the new-style classes and properties, that's a definite TODO. I'd
also like a presubmit pylint checks (like we have in the internal part), and as
much testing as possible. That's all in the queue.

For now, my focus is on making the tests run and report reasonably. The
reporting is mostly OK, but the test scenarios are getting simplified in my
coming CLs, and the code itself less complex in a hunt for subtle bugs, which
might have been causing some strange failures. Fixing that should make this a
stable measure of how Chrome is doing (as opposed to how the test framework is
doing).

I addressed all your comments. Mostly straightforwardly, only in tests.py, a
renaming got a bit more creative, you might want to have a look.

Given you LGTM, I plan to land this later tonight, but if you have comments
afterwards, I'm happy to address them in a follow-up.

Thanks!
Vaclav

https://codereview.chromium.org/1022703004/diff/40001/components/test/data/pa...
File components/test/data/password_manager/automated_tests/README (left):

https://codereview.chromium.org/1022703004/diff/40001/components/test/data/pa...
components/test/data/password_manager/automated_tests/README:112: * SendEnterTo:
find an element using CSS Selector and send enter to it.
On 2015/03/26 16:18:55, engedy wrote:
> Could you please make note the removal of this in the CL description?

Done.

https://codereview.chromium.org/1022703004/diff/40001/components/test/data/pa...
File components/test/data/password_manager/automated_tests/environment.py
(left):

https://codereview.chromium.org/1022703004/diff/40001/components/test/data/pa...
components/test/data/password_manager/automated_tests/environment.py:91: # If
|chrome_path| is not defined, this means that we are in the dashboard
On 2015/03/26 16:18:55, engedy wrote:
> Could you please mention in the CL description that this functionality is
> removed too?

This was not so much removed, as simplified. In the past, to get the list of
websites, Environment had to be instantiated. I updated the CL description with
that. Note that getting the test names is not specific to the use in the
dashboard (which itself became deprecated in the meantime), so I don't mention
the dashboard in the CL description.
Done.

https://codereview.chromium.org/1022703004/diff/40001/components/test/data/pa...
File components/test/data/password_manager/automated_tests/environment.py
(right):

https://codereview.chromium.org/1022703004/diff/40001/components/test/data/pa...
components/test/data/password_manager/automated_tests/environment.py:58:
shutil.rmtree(profile_path, ignore_errors=True)
On 2015/03/26 16:18:55, engedy wrote:
> nit: Given that this masks both kinds of errors (i.e., directory did not exist
> in the first place, existed but failed to remove), I would instead check
> existence and then call this with ignore_errors=False.

Done.

https://codereview.chromium.org/1022703004/diff/40001/components/test/data/pa...
components/test/data/password_manager/automated_tests/environment.py:108:
websitetest.driver = self.driver
On 2015/03/26 16:18:55, engedy wrote:
> nit: Please add a todo to make this a property.

Done.

https://codereview.chromium.org/1022703004/diff/40001/components/test/data/pa...
File components/test/data/password_manager/automated_tests/tests.py (left):

https://codereview.chromium.org/1022703004/diff/40001/components/test/data/pa...
components/test/data/password_manager/automated_tests/tests.py:9: import logging
On 2015/03/26 16:18:55, engedy wrote:
> I think this can be removed.

Done.

https://codereview.chromium.org/1022703004/diff/40001/components/test/data/pa...
File components/test/data/password_manager/automated_tests/tests.py (right):

https://codereview.chromium.org/1022703004/diff/40001/components/test/data/pa...
components/test/data/password_manager/automated_tests/tests.py:479: def
Tests(environment, test_to_run):
On 2015/03/26 16:18:55, engedy wrote:
> nit: Do we even need this function? (Fine to leave it for now if removing it
> interferes with refactoring.)

Good call, removed.
Done.

https://codereview.chromium.org/1022703004/diff/40001/components/test/data/pa...
components/test/data/password_manager/automated_tests/tests.py:489: if
test_to_run in all_tests.keys():
On 2015/03/26 16:18:55, engedy wrote:
> nit: We could just use the 'in' operator.

You meant -- use it on the dictionary directly?
That sounds good, and is even asymptotically faster, assuming the dict is sorted
by the keys. Thanks for pointing that out (I forgot that in acts on the keys).
If you meant something else, please let me know.
Done.

https://codereview.chromium.org/1022703004/diff/40001/components/test/data/pa...
components/test/data/password_manager/automated_tests/tests.py:514: def
RunTests(chrome_path, chromedriver_path, profile_path,
On 2015/03/26 16:18:55, engedy wrote:
> nit: Rename this to RunTest if it does not interfere with refactoring.

Done.

https://codereview.chromium.org/1022703004/diff/40001/components/test/data/pa...
components/test/data/password_manager/automated_tests/tests.py:518: """Runs the
the tests
On 2015/03/26 16:18:55, engedy wrote:
> typo/nit: Runs the specified test.

I actually rephrased a bit more, because there was confusion between test in the
sense of a website for test, and test as a particular test scenario applied to
that website. There are multiple tests scenarios applied to one website. I now
keep referring to the scenarios as tests, and to the website as a website in the
function.

https://codereview.chromium.org/1022703004/diff/40001/components/test/data/pa...
File components/test/data/password_manager/automated_tests/websitetest.py
(left):

https://codereview.chromium.org/1022703004/diff/40001/components/test/data/pa...
components/test/data/password_manager/automated_tests/websitetest.py:11:
sys.path.insert(0, '../../../../third_party/webdriver/pylib/')
On 2015/03/26 16:18:56, engedy wrote:
> Could you please allude in the CL description to why this can be removed?

Done.

engedy

Still LGTM, thanks! https://codereview.chromium.org/1022703004/diff/40001/components/test/data/password_manager/automated_tests/environment.py File components/test/data/password_manager/automated_tests/environment.py (left): https://codereview.chromium.org/1022703004/diff/40001/components/test/data/password_manager/automated_tests/environment.py#oldcode91 components/test/data/password_manager/automated_tests/environment.py:91: # If |chrome_path| is not defined, ...

5 years, 9 months ago (2015-03-26 18:04:54 UTC) #7

Still LGTM, thanks!

https://codereview.chromium.org/1022703004/diff/40001/components/test/data/pa...
File components/test/data/password_manager/automated_tests/environment.py
(left):

https://codereview.chromium.org/1022703004/diff/40001/components/test/data/pa...
components/test/data/password_manager/automated_tests/environment.py:91: # If
|chrome_path| is not defined, this means that we are in the dashboard
On 2015/03/26 18:01:50, vabr (Chromium) wrote:
> On 2015/03/26 16:18:55, engedy wrote:
> > Could you please mention in the CL description that this functionality is
> > removed too?
> 
> This was not so much removed, as simplified. In the past, to get the list of
> websites, Environment had to be instantiated. I updated the CL description
with
> that. Note that getting the test names is not specific to the use in the
> dashboard (which itself became deprecated in the meantime), so I don't mention
> the dashboard in the CL description.
> Done.

Acknowledged.

https://codereview.chromium.org/1022703004/diff/40001/components/test/data/pa...
File components/test/data/password_manager/automated_tests/tests.py (right):

https://codereview.chromium.org/1022703004/diff/40001/components/test/data/pa...
components/test/data/password_manager/automated_tests/tests.py:489: if
test_to_run in all_tests.keys():
On 2015/03/26 18:01:50, vabr (Chromium) wrote:
> On 2015/03/26 16:18:55, engedy wrote:
> > nit: We could just use the 'in' operator.
> 
> You meant -- use it on the dictionary directly?
> That sounds good, and is even asymptotically faster, assuming the dict is
sorted
> by the keys. Thanks for pointing that out (I forgot that in acts on the keys).
> If you meant something else, please let me know.
> Done.

Yes, that's what I meant.

https://codereview.chromium.org/1022703004/diff/40001/components/test/data/pa...
components/test/data/password_manager/automated_tests/tests.py:518: """Runs the
the tests
On 2015/03/26 18:01:50, vabr (Chromium) wrote:
> On 2015/03/26 16:18:55, engedy wrote:
> > typo/nit: Runs the specified test.
> 
> I actually rephrased a bit more, because there was confusion between test in
the
> sense of a website for test, and test as a particular test scenario applied to
> that website. There are multiple tests scenarios applied to one website. I now
> keep referring to the scenarios as tests, and to the website as a website in
the
> function.

Acknowledged.

commit-bot: I haz the power

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

5 years, 9 months ago (2015-03-26 18:16:00 UTC) #9

commit-bot: I haz the power

5 years, 9 months ago (2015-03-26 19:29:43 UTC) #11

Message was sent while issue was closed.

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

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages