Add tests for Component Updater command line arguments

Add tests for Component Updater command line arguments

This adds tests to validate the effects of command line arguments on
ChromeComponentUpdaterConfigurator:
--component-updater disable-pings
--component-updater fast-update
--component-updater url-source = <url>
--component-updater test-request

BUG=469875
Committed: https://crrev.com/c72f158ec9163f075114451fc96ae5e336eca935
Cr-Commit-Position: refs/heads/master@{#322413}

[tomc, 2015-03-23 21:58:36]

tomc@amazon.com changed reviewers:
+ sorin@chromium.org

[tomc, 2015-03-23 21:58:37]

Hey Sorin,
PTAL. Note that I just requested access to the CCLA and expect to be added
within a day.

Thanks!

[Sorin Jianu, 2015-03-23 23:32:01]

Hi, thank you for sending the CL.
What is the motivation for this change?
I have a couple of style comments below, thank you!

https://codereview.chromium.org/1013003003/diff/1/chrome/browser/component_up...
File
chrome/browser/component_updater/test/chrome_component_updater_configurator_unittest.cc
(right):

https://codereview.chromium.org/1013003003/diff/1/chrome/browser/component_up...
chrome/browser/component_updater/test/chrome_component_updater_configurator_unittest.cc:14:

Extra empty line not needed/

https://codereview.chromium.org/1013003003/diff/1/chrome/browser/component_up...
chrome/browser/component_updater/test/chrome_component_updater_configurator_unittest.cc:16:
base::CommandLine * cmdline = base::CommandLine::ForCurrentProcess();
* goes next to the type.

Are you using the "git cl format" command? For the component update code we like
to run that command, so that we have code that is formatted in a
style-compliant, and uniform way. You could only run it for new code that you
have written.

https://codereview.chromium.org/1013003003/diff/1/chrome/browser/component_up...
chrome/browser/component_updater/test/chrome_component_updater_configurator_unittest.cc:17:
cmdline->AppendSwitchASCII(switches::kComponentUpdater, kSwitchDisablePings);
There are two ways to write these data driven unit tests: like this or by
hardcoding the expected values. If we do the latter, then there is not need to
export the command line arguments. In this case, the test not only tests the
behavior of TestDisablePings, but also that the command argument has not changed
in a way that breaks backward compatibility.

https://codereview.chromium.org/1013003003/diff/1/chrome/browser/component_up...
chrome/browser/component_updater/test/chrome_component_updater_configurator_unittest.cc:19:
update_client::Configurator* config =
Some of these variables can be declared const.

https://codereview.chromium.org/1013003003/diff/1/chrome/browser/component_up...
chrome/browser/component_updater/test/chrome_component_updater_configurator_unittest.cc:28:
// Set command line flag
Comment not needed.

https://codereview.chromium.org/1013003003/diff/1/chrome/browser/component_up...
chrome/browser/component_updater/test/chrome_component_updater_configurator_unittest.cc:52:
// should only be one element
Comment not needed.

https://codereview.chromium.org/1013003003/diff/1/chrome/browser/component_up...
chrome/browser/component_updater/test/chrome_component_updater_configurator_unittest.cc:53:
EXPECT_EQ(urls.size(), 1U);
The syntaxt for the EXPECT assertion is ASSERT_EQ(expected, actual);

When the expectation fails, it prints a human friendly message. The message will
be confusing if the parameters are swapped.

https://codereview.chromium.org/1013003003/diff/1/chrome/browser/component_up...
chrome/browser/component_updater/test/chrome_component_updater_configurator_unittest.cc:67:
} // namespace component_updater
Needs a space before //.

[tomc, 2015-03-24 22:13:10]

Hi Sorin, thanks for the feedback.
We've been upstreaming test coverage for the various command line switches in
Chromium, because there are so many of them :-)


On 2015/03/23 23:32:01, Sorin Jianu wrote:
> Hi, thank you for sending the CL.
> What is the motivation for this change?
> I have a couple of style comments below, thank you!
> 
>
https://codereview.chromium.org/1013003003/diff/1/chrome/browser/component_up...
> File
>
chrome/browser/component_updater/test/chrome_component_updater_configurator_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/1013003003/diff/1/chrome/browser/component_up...
>
chrome/browser/component_updater/test/chrome_component_updater_configurator_unittest.cc:14:
> 
> Extra empty line not needed/
> 
>
https://codereview.chromium.org/1013003003/diff/1/chrome/browser/component_up...
>
chrome/browser/component_updater/test/chrome_component_updater_configurator_unittest.cc:16:
> base::CommandLine * cmdline = base::CommandLine::ForCurrentProcess();
> * goes next to the type.
> 
> Are you using the "git cl format" command? For the component update code we
like
> to run that command, so that we have code that is formatted in a
> style-compliant, and uniform way. You could only run it for new code that you
> have written.
> 
>
https://codereview.chromium.org/1013003003/diff/1/chrome/browser/component_up...
>
chrome/browser/component_updater/test/chrome_component_updater_configurator_unittest.cc:17:
> cmdline->AppendSwitchASCII(switches::kComponentUpdater, kSwitchDisablePings);
> There are two ways to write these data driven unit tests: like this or by
> hardcoding the expected values. If we do the latter, then there is not need to
> export the command line arguments. In this case, the test not only tests the
> behavior of TestDisablePings, but also that the command argument has not
changed
> in a way that breaks backward compatibility.
> 
>
https://codereview.chromium.org/1013003003/diff/1/chrome/browser/component_up...
>
chrome/browser/component_updater/test/chrome_component_updater_configurator_unittest.cc:19:
> update_client::Configurator* config =
> Some of these variables can be declared const.
> 
>
https://codereview.chromium.org/1013003003/diff/1/chrome/browser/component_up...
>
chrome/browser/component_updater/test/chrome_component_updater_configurator_unittest.cc:28:
> // Set command line flag
> Comment not needed.
> 
>
https://codereview.chromium.org/1013003003/diff/1/chrome/browser/component_up...
>
chrome/browser/component_updater/test/chrome_component_updater_configurator_unittest.cc:52:
> // should only be one element
> Comment not needed.
> 
>
https://codereview.chromium.org/1013003003/diff/1/chrome/browser/component_up...
>
chrome/browser/component_updater/test/chrome_component_updater_configurator_unittest.cc:53:
> EXPECT_EQ(urls.size(), 1U);
> The syntaxt for the EXPECT assertion is ASSERT_EQ(expected, actual);
> 
> When the expectation fails, it prints a human friendly message. The message
will
> be confusing if the parameters are swapped.
> 
>
https://codereview.chromium.org/1013003003/diff/1/chrome/browser/component_up...
>
chrome/browser/component_updater/test/chrome_component_updater_configurator_unittest.cc:67:
> } // namespace component_updater
> Needs a space before //.

[tomc, 2015-03-24 22:47:29]

Hey Sorin, I addressed your comments PTAL.

Thanks!

[Sorin Jianu, 2015-03-24 23:05:33]

Thank you!

Do you have trybot access? If yes, please run "git cl try" after uploading the
latest patch, to trigger a try bot run and see what kind of results we get.

If we see a green run, then I could approve this CL. Thank you for contributing
the code.

https://codereview.chromium.org/1013003003/diff/20001/chrome/browser/componen...
File
chrome/browser/component_updater/test/chrome_component_updater_configurator_unittest.cc
(right):

https://codereview.chromium.org/1013003003/diff/20001/chrome/browser/componen...
chrome/browser/component_updater/test/chrome_component_updater_configurator_unittest.cc:21:
std::vector<GURL> pingUrls = config->PingUrl();
Small deal, we could use a const declaration for the local variables that we
don't modify.

https://codereview.chromium.org/1013003003/diff/20001/chrome/browser/componen...
chrome/browser/component_updater/test/chrome_component_updater_configurator_unittest.cc:50:
ASSERT_EQ(overrideUrl, urls.at(0).possibly_invalid_spec());
Can we test that the actual URL is the same as the expected URL previously set?

[tomc, 2015-03-25 18:35:16]

Hey Sorin,

> Do you have trybot access? If yes, please run "git cl try" after uploading the
> latest patch, to trigger a try bot run and see what kind of results we get.
I do not have trybot access, it would be great if you could trigger a run for
me.

>
https://codereview.chromium.org/1013003003/diff/20001/chrome/browser/componen...
>
chrome/browser/component_updater/test/chrome_component_updater_configurator_unittest.cc:50:
> ASSERT_EQ(overrideUrl, urls.at(0).possibly_invalid_spec());
> Can we test that the actual URL is the same as the expected URL previously
set?
Could you clarify what you mean? I believe that this assert is testing just
that. Would you rather have me use urls.at(0).spec()?

[Sorin Jianu, 2015-03-25 19:25:13]

Would it be possible for you to request try bot access? The bots are important
if you are thinking of contributing more code.

I will start some tries.

https://codereview.chromium.org/1013003003/diff/20001/chrome/browser/componen...
File
chrome/browser/component_updater/test/chrome_component_updater_configurator_unittest.cc
(right):

https://codereview.chromium.org/1013003003/diff/20001/chrome/browser/componen...
chrome/browser/component_updater/test/chrome_component_updater_configurator_unittest.cc:50:
ASSERT_EQ(overrideUrl, urls.at(0).possibly_invalid_spec());
On 2015/03/24 23:05:32, Sorin Jianu wrote:
> Can we test that the actual URL is the same as the expected URL previously
set?

I apologize, I misread the code. Is there a reason we call possibly_invalid_spec
instead of spec? Is http://0.0.0.0/ invalid?

[Sorin Jianu, 2015-03-25 20:32:21]

We have some results for the tries. Thank you!

https://codereview.chromium.org/1013003003/diff/40001/chrome/browser/componen...
File
chrome/browser/component_updater/test/chrome_component_updater_configurator_unittest.cc
(right):

https://codereview.chromium.org/1013003003/diff/40001/chrome/browser/componen...
chrome/browser/component_updater/test/chrome_component_updater_configurator_unittest.cc:18:
const update_client::Configurator* config =
I missed the memory leak here and below.

I am working on code right now where I made the Configurator a ref_counted type,
specifically to avoid similar issues.

[tomc, 2015-03-25 21:28:34]

Hey Sorin,
Thanks for kicking off the try bot run for me. I fixed the memory leak in the
new tests.

As for requesting try bot access I think that it may be a good idea to wait
until I have worked up a bigger list of contributions as suggested here:
http://www.chromium.org/getting-involved/become-a-committer#TOC-Try-job-access

[Sorin Jianu, 2015-03-25 21:50:09]

Thank you. One idea how to use scoped_ptr for memory management.

https://codereview.chromium.org/1013003003/diff/60001/chrome/browser/componen...
File
chrome/browser/component_updater/test/chrome_component_updater_configurator_unittest.cc
(right):

https://codereview.chromium.org/1013003003/diff/60001/chrome/browser/componen...
chrome/browser/component_updater/test/chrome_component_updater_configurator_unittest.cc:23:
delete config;
we can use a scoped_ptr for this:

#include "base/memory/scoped_ptr.h"

scoped_ptr<update_client::Configurator> config(MakeChrome...());

That's should be it.

[tomc, 2015-03-25 23:21:51]

> we can use a scoped_ptr for this:

Good idea. I added a new patch set where I use scoped_ptr instead of raw
pointers. PTAL :-)

[Sorin Jianu, 2015-03-25 23:41:42]

lgtm

Thank you!

[tomc, 2015-03-26 16:49:19]

The CQ bit was checked by tomc@amazon.com

[commit-bot: I haz the power, 2015-03-26 16:50:06]

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

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

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-03-26 17:42:09]

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