[Password manager Python tests] Remove the option to disable tests

[Password manager Python tests] Remove the option to disable tests

The current use-cases of the tests only consist of either running all the tests, or running individually specified tests. The information about which tests are disabled and which are supposed to pass, is therefore not useful, and making the scripts more complicated than needed.

This CL removes the support for disabling the Python tests.

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

[vabr (Chromium), 2015-03-25 16:02:43]

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

[vabr (Chromium), 2015-03-25 16:02:43]

Hi Tanja,

Please have a look.

Note that this tries to focus solely on removing the disabled/enabled
distinction. There are many more things to improve in the scripts, but I will
deal with that with other CLs to keep the changes small.

Thanks!

Vaclav

[melandory, 2015-03-25 16:22:14]

lgtm with comments.

https://codereview.chromium.org/1012863006/diff/1/components/test/data/passwo...
File components/test/data/password_manager/automated_tests/tests.py (right):

https://codereview.chromium.org/1012863006/diff/1/components/test/data/passwo...
components/test/data/password_manager/automated_tests/tests.py:494: print "Skip
test: test {} is not in known tests".format(test)
Since recent switch to logger, probably this print should be changed too.

https://codereview.chromium.org/1012863006/diff/1/components/test/data/passwo...
components/test/data/password_manager/automated_tests/tests.py:501: 
Optional:
I would rewrite code in lines 490--500 in following way:

test_to_run = test_to_run or all_tests.keys()
for test_name in tests_to_run:
  if test_name not in all_tests.keys():
    print "Skip test: test {} is not in known tests".format(test_name)
  else:
    environment.AddWebsiteTest(all_tests[test_name])

It has some disadvantages, but in my opinion it's easier to read, so it's worth
it.

[vabr (Chromium), 2015-03-25 16:51:13]

Thanks, Tanja!

I addressed your comments and because you approved this, will send this to CQ.
If you have any follow-ups on how I addressed your comments, feel free to let me
know, I'll be happy to address those.

Cheers,
Vaclav

https://codereview.chromium.org/1012863006/diff/1/components/test/data/passwo...
File components/test/data/password_manager/automated_tests/tests.py (right):

https://codereview.chromium.org/1012863006/diff/1/components/test/data/passwo...
components/test/data/password_manager/automated_tests/tests.py:494: print "Skip
test: test {} is not in known tests".format(test)
On 2015/03/25 16:22:14, melandory wrote:
> Since recent switch to logger, probably this print should be changed too.

Good point.
Because the value of this log is rather questionable (the automatically run
tests will run all tests, and a user running a test on demand can probably
figure out what's happening, or add a log locally), I decided to remove it
completely.

Once the refactoring is done, I'd like to introduce a unified way of logging in
these scripts, and for that I'll need to figure out a way to centralise the log
level for that and the target logger (either the root one, or define a single
designated one).

https://codereview.chromium.org/1012863006/diff/1/components/test/data/passwo...
components/test/data/password_manager/automated_tests/tests.py:501: 
On 2015/03/25 16:22:14, melandory wrote:
> Optional:
> I would rewrite code in lines 490--500 in following way:
> 
> test_to_run = test_to_run or all_tests.keys()
> for test_name in tests_to_run:
>   if test_name not in all_tests.keys():
>     print "Skip test: test {} is not in known tests".format(test_name)
>   else:
>     environment.AddWebsiteTest(all_tests[test_name])
> 
> It has some disadvantages, but in my opinion it's easier to read, so it's
worth
> it. 
> 
> 
> 

I like that, thanks for suggesting!
Done.

[vabr (Chromium), 2015-03-25 16:51:26]

The CQ bit was checked by vabr@chromium.org

[vabr (Chromium), 2015-03-25 16:51:26]

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

[commit-bot: I haz the power, 2015-03-25 16:52:09]

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

[commit-bot: I haz the power, 2015-03-25 16:52:11]

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

[commit-bot: I haz the power, 2015-03-25 16:52:12]

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.

[vabr (Chromium), 2015-03-25 16:55:28]

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

[vabr (Chromium), 2015-03-25 16:55:29]

Hi Balázs,

Could you please rubber stamp this?
Tanja has been successfully nominated for committership, but the process will
likely take some more days, and apparently only committers can approve CLs.

Thanks!
Vaclav

[engedy, 2015-03-25 18:40:03]

On 2015/03/25 16:55:29, vabr (Chromium) wrote:
> Hi Balázs,
> 
> Could you please rubber stamp this?
> Tanja has been successfully nominated for committership, but the process will
> likely take some more days, and apparently only committers can approve CLs.
> 
> Thanks!
> Vaclav

LGTM.

[vabr (Chromium), 2015-03-26 08:51:49]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2015-03-26 08:52:11]

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

[commit-bot: I haz the power, 2015-03-26 10:03:05]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-03-26 10:03:41]

Patchset 2 (id:??) landed as
https://crrev.com/1d009819f28e4c7c8476fc4f313fab03f8df6a4c
Cr-Commit-Position: refs/heads/master@{#322347}