Refactor install.cc into the work item parts and the non-work item parts.

Refactor out of install.cc the set of functions that operate on a work item list for better testability.

Write preliminary test framework for testing these functions.

BUG=61609
TEST=setup_unittests.exe



Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=71473

[robertshield, 2011-01-11 22:01:27]

Please find herein an attempt at making a large part of the installer
functionality unit testable. 

With this we should be able to write expectations that describe the behaviour of
the installer for any given set of initial states and target states.

I have written a so-simple-it's-useless example test case that demonstrates the
concept. When this change is in, we should be able to parallelize writing more
tests.

Also, some implementation notes came up while I was writing this around the need
for PackageProperties to exist in its current form. Let's discuss this tomorrow.

[grt (UTC plus 2), 2011-01-12 16:18:02]

Sweet!

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/setup/inst...
File chrome/installer/setup/install_worker.h (right):

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.h:11: 
#include <vector>

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.h:13: #include "base/version.h"
Looks like Version could be forward decl'd.

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.h:14: #include
"chrome/installer/util/product.h"
Forward decl Product, too?

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.h:18: 
class CommandLine;

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/setup/inst...
File chrome/installer/setup/install_worker_unittest.cc (right):

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/setup/inst...
chrome/installer/setup/install_worker_unittest.cc:32: class MockWorkItemList :
public WorkItemList {
Cool!

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/setup/inst...
chrome/installer/setup/install_worker_unittest.cc:133: new
MockInstallationState);
nit: Call me old-fashioned, but I prefer new Foo() over just new Foo.

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/setup/inst...
chrome/installer/setup/install_worker_unittest.cc:163: 
nit: does this blank line add something?

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/setup/inst...
chrome/installer/setup/install_worker_unittest.cc:183: // perhaps Package itself
should not exist at all.
this will be resolved if/when i finish my change-in-progress.

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/setup/inst...
chrome/installer/setup/install_worker_unittest.cc:206: 
nit: remove extra newline.

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/util/insta...
File chrome/installer/util/installation_state.h (right):

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/util/insta...
chrome/installer/util/installation_state.h:84: 
 private:

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/util/insta...
File chrome/installer/util/installer_state.h (right):

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/util/insta...
chrome/installer/util/installer_state.h:51: 
 private:

[erikwright (departed), 2011-01-12 16:31:49]

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/setup/inst...
File chrome/installer/setup/install_worker.cc (right):

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.cc:67: 
I assume these are identical to the originals?

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/setup/inst...
File chrome/installer/setup/install_worker.h (right):

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.h:13: #include "base/version.h"
replace version.h and product.h with forward decls.

include <vector>

consider replacing scoped_ptr.h with forward decl.

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/util/work_...
File chrome/installer/util/work_item_list.h (right):

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/util/work_...
chrome/installer/util/work_item_list.h:38: virtual void AddWorkItem(WorkItem*
work_item);
I would prefer this to be split into a pure-virtual class and an implementation.
Otherwise, someone can add a new method, forget to add a mock for it, and the
tests will actually call the implementation method (wreaking havoc on the
builder).

[amit, 2011-01-12 17:41:22]

Nice incremental change and I like the way we can write tests now.
LGTM as long as everyone else is happy with it :)

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/setup/inst...
File chrome/installer/setup/install_worker.cc (right):

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.cc:51: BOOL is64 = FALSE;
nit: is_64bit :)

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/setup/inst...
File chrome/installer/setup/install_worker_unittest.cc (right):

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/setup/inst...
chrome/installer/setup/install_worker_unittest.cc:192:
EXPECT_CALL(work_item_list, AddCopyTreeWorkItem(_,_,_,_,_))
It will be nice if we can create a work item from data and then create a list
from static array.

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/util/work_...
File chrome/installer/util/work_item_list.h (right):

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/util/work_...
chrome/installer/util/work_item_list.h:38: virtual void AddWorkItem(WorkItem*
work_item);
On 2011/01/12 16:31:49, erikwright wrote:
> I would prefer this to be split into a pure-virtual class and an
implementation.
> Otherwise, someone can add a new method, forget to add a mock for it, and the
> tests will actually call the implementation method (wreaking havoc on the
> builder).
+1

[tommi (sloooow) - chröme, 2011-01-12 19:02:49]

very nice!  I agree with Erik and Amit on having a separate factory interface.

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/setup/inst...
File chrome/installer/setup/install_worker.cc (right):

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.cc:50: typedef BOOL (WINAPI
*WOW_FUNC)(HANDLE, PBOOL);
BOOL* instead of PBOOL.

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.cc:53: HANDLE handle =
GetCurrentProcess();
'handle' is less descriptive than just calling GetCurrentProcess() where you
need it.

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.cc:55: WOW_FUNC p =
reinterpret_cast<WOW_FUNC>(GetProcAddress(module,
nit: is_wow_64 instead of p

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.cc:57: if ((p != NULL) && (!(p)(handle,
&is64) || (is64 != FALSE))) {
You don't really need the if and two return statements.  could just be:

return is_wow_64 != NULL && (!is_wow_64(GetCurrentProcess(), &is64) || is64);

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.cc:67: 
On 2011/01/12 16:31:49, erikwright wrote:
> I assume these are identical to the originals?

+1 (to save on review time)

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.cc:211:
install_list->AddSetRegValueWorkItem(reg_root, uninstall_reg,
observation - we should factor out all these factory methods to a special
interface that can then be replaced in a unit test.

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.cc:243: 
one empty line

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.cc:281: 
one empty line

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.cc:409: 
one empty line (and throughout)

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.cc:431: 
here too :)

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/setup/inst...
File chrome/installer/setup/install_worker_unittest.cc (right):

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/setup/inst...
chrome/installer/setup/install_worker_unittest.cc:36:
MOCK_METHOD5(AddCopyTreeWorkItem, WorkItem*(const std::wstring&,
Ah, I see :) I misunderstood the approach the other day.  This is good.

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/util/insta...
File chrome/installer/util/installation_state.h (right):

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/util/insta...
chrome/installer/util/installation_state.h:58: // Caller does NOT assume
ownership of returned pointer.
since the returned pointer is const, I presume you can't delete it anyway so the
comment isn't necessary

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/util/insta...
chrome/installer/util/installation_state.h:62: // Caller does NOT assume
ownership of returned pointer.
same here

[grt (UTC plus 2), 2011-01-12 19:23:57]

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/util/insta...
File chrome/installer/util/installation_state.h (right):

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/util/insta...
chrome/installer/util/installation_state.h:58: // Caller does NOT assume
ownership of returned pointer.
On 2011/01/12 19:02:49, tommi wrote:
> since the returned pointer is const, I presume you can't delete it anyway so
the
> comment isn't necessary

C++-03 5.3.5.2: Note: a pointer to a const type can be the operand of a
delete-expression; it is not necessary to cast away the constness (5.2.11) of
the pointer expression before it is used as the operand of the
delete-expression.

[tommi (sloooow) - chröme, 2011-01-12 20:13:51]

thanks.  I had a hunch I would be wrong about this :)

On Wed, Jan 12, 2011 at 2:23 PM, <grt@chromium.org> wrote:

>
>
>
http://codereview.chromium.org/6153003/diff/16001/chrome/installer/util/insta...
> File chrome/installer/util/installation_state.h (right):
>
>
>
http://codereview.chromium.org/6153003/diff/16001/chrome/installer/util/insta...
> chrome/installer/util/installation_state.h:58: // Caller does NOT assume
> ownership of returned pointer.
> On 2011/01/12 19:02:49, tommi wrote:
>
>> since the returned pointer is const, I presume you can't delete it
>>
> anyway so the
>
>> comment isn't necessary
>>
>
> C++-03 5.3.5.2: Note: a pointer to a const type can be the operand of a
> delete-expression; it is not necessary to cast away the constness
> (5.2.11) of the pointer expression before it is used as the operand of
> the delete-expression.
>
>
> http://codereview.chromium.org/6153003/
>

[robertshield, 2011-01-13 17:06:32]

Thanks! PTAL

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/setup/inst...
File chrome/installer/setup/install_worker.cc (right):

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.cc:50: typedef BOOL (WINAPI
*WOW_FUNC)(HANDLE, PBOOL);
On 2011/01/12 19:02:49, tommi wrote:
> BOOL* instead of PBOOL.

Done.

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.cc:51: BOOL is64 = FALSE;
On 2011/01/12 17:41:22, amit wrote:
> nit: is_64bit :)

Done.

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.cc:53: HANDLE handle =
GetCurrentProcess();
On 2011/01/12 19:02:49, tommi wrote:
> 'handle' is less descriptive than just calling GetCurrentProcess() where you
> need it.

Done.  (In case it wasn't clear, this code is copy/pasted from its original
location).

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.cc:55: WOW_FUNC p =
reinterpret_cast<WOW_FUNC>(GetProcAddress(module,
On 2011/01/12 19:02:49, tommi wrote:
> nit: is_wow_64 instead of p

Done.

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.cc:57: if ((p != NULL) && (!(p)(handle,
&is64) || (is64 != FALSE))) {
On 2011/01/12 19:02:49, tommi wrote:
> You don't really need the if and two return statements.  could just be:
> 
> return is_wow_64 != NULL && (!is_wow_64(GetCurrentProcess(), &is64) || is64);

Done.

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.cc:67: 
On 2011/01/12 16:31:49, erikwright wrote:
> I assume these are identical to the originals?

Yes, copy/pasted.

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.cc:67: 
On 2011/01/12 19:02:49, tommi wrote:
> On 2011/01/12 16:31:49, erikwright wrote:
> > I assume these are identical to the originals?
> 
> +1 (to save on review time)

Done.

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.cc:211:
install_list->AddSetRegValueWorkItem(reg_root, uninstall_reg,
On 2011/01/12 19:02:49, tommi wrote:
> observation - we should factor out all these factory methods to a special
> interface that can then be replaced in a unit test.

I'm mocking the WorkItemList at the moment which accomplishes what you describe
here.

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.cc:243: 
On 2011/01/12 19:02:49, tommi wrote:
> one empty line

Done.

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.cc:281: 
On 2011/01/12 19:02:49, tommi wrote:
> one empty line

Done.

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.cc:409: 
On 2011/01/12 19:02:49, tommi wrote:
> one empty line (and throughout)

Done.

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.cc:431: 
On 2011/01/12 19:02:49, tommi wrote:
> here too :)

Done.

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/setup/inst...
File chrome/installer/setup/install_worker.h (right):

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.h:11: 
On 2011/01/12 16:18:02, grt wrote:
> #include <vector>

Done.

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.h:13: #include "base/version.h"
On 2011/01/12 16:18:02, grt wrote:
> Looks like Version could be forward decl'd.

Done.

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.h:13: #include "base/version.h"
On 2011/01/12 16:31:49, erikwright wrote:
> replace version.h and product.h with forward decls.
> 
> include <vector>
> 
> consider replacing scoped_ptr.h with forward decl.

Done, except for the scoped_ptr.h thing. I think that's ok to leave.

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.h:14: #include
"chrome/installer/util/product.h"
On 2011/01/12 16:18:02, grt wrote:
> Forward decl Product, too?

Done.

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.h:18: 
On 2011/01/12 16:18:02, grt wrote:
> class CommandLine;

Done.

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/setup/inst...
File chrome/installer/setup/install_worker_unittest.cc (right):

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/setup/inst...
chrome/installer/setup/install_worker_unittest.cc:32: class MockWorkItemList :
public WorkItemList {
On 2011/01/12 16:18:02, grt wrote:
> Cool!

Thanks!

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/setup/inst...
chrome/installer/setup/install_worker_unittest.cc:36:
MOCK_METHOD5(AddCopyTreeWorkItem, WorkItem*(const std::wstring&,
On 2011/01/12 19:02:49, tommi wrote:
> Ah, I see :) I misunderstood the approach the other day.  This is good.

Thanks!

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/setup/inst...
chrome/installer/setup/install_worker_unittest.cc:133: new
MockInstallationState);
On 2011/01/12 16:18:02, grt wrote:
> nit: Call me old-fashioned, but I prefer new Foo() over just new Foo.

You, sir, are old fashioned.

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/setup/inst...
chrome/installer/setup/install_worker_unittest.cc:163: 
On 2011/01/12 16:18:02, grt wrote:
> nit: does this blank line add something?

feng shui.

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/setup/inst...
chrome/installer/setup/install_worker_unittest.cc:183: // perhaps Package itself
should not exist at all.
On 2011/01/12 16:18:02, grt wrote:
> this will be resolved if/when i finish my change-in-progress.

Then in addition to being old-fashioned, you are also awesome.

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/setup/inst...
chrome/installer/setup/install_worker_unittest.cc:192:
EXPECT_CALL(work_item_list, AddCopyTreeWorkItem(_,_,_,_,_))
On 2011/01/12 17:41:22, amit wrote:
> It will be nice if we can create a work item from data and then create a list
> from static array. 

That sounds cool, I would like to tackle that in a separate CL along with the
WorkItem and WorkItemList refactoring.

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/setup/inst...
chrome/installer/setup/install_worker_unittest.cc:206: 
On 2011/01/12 16:18:02, grt wrote:
> nit: remove extra newline.

Done.

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/util/insta...
File chrome/installer/util/installation_state.h (right):

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/util/insta...
chrome/installer/util/installation_state.h:58: // Caller does NOT assume
ownership of returned pointer.
On 2011/01/12 19:23:57, grt wrote:
> On 2011/01/12 19:02:49, tommi wrote:
> > since the returned pointer is const, I presume you can't delete it anyway so
> the
> > comment isn't necessary
> 
> C++-03 5.3.5.2: Note: a pointer to a const type can be the operand of a
> delete-expression; it is not necessary to cast away the constness (5.2.11) of
> the pointer expression before it is used as the operand of the
> delete-expression.

Done.

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/util/insta...
chrome/installer/util/installation_state.h:58: // Caller does NOT assume
ownership of returned pointer.
On 2011/01/12 19:02:49, tommi wrote:
> since the returned pointer is const, I presume you can't delete it anyway so
the
> comment isn't necessary

What Greg said ;-)

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/util/insta...
chrome/installer/util/installation_state.h:62: // Caller does NOT assume
ownership of returned pointer.
On 2011/01/12 19:02:49, tommi wrote:
> same here

Done.

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/util/insta...
chrome/installer/util/installation_state.h:84: 
On 2011/01/12 16:18:02, grt wrote:
>  private:

oops, done, thanks :-)

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/util/insta...
File chrome/installer/util/installer_state.h (right):

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/util/insta...
chrome/installer/util/installer_state.h:51: 
On 2011/01/12 16:18:02, grt wrote:
>  private:

Done.

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/util/work_...
File chrome/installer/util/work_item_list.h (right):

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/util/work_...
chrome/installer/util/work_item_list.h:38: virtual void AddWorkItem(WorkItem*
work_item);
On 2011/01/12 17:41:22, amit wrote:
> On 2011/01/12 16:31:49, erikwright wrote:
> > I would prefer this to be split into a pure-virtual class and an
> implementation.
> > Otherwise, someone can add a new method, forget to add a mock for it, and
the
> > tests will actually call the implementation method (wreaking havoc on the
> > builder).
> +1

Done.

http://codereview.chromium.org/6153003/diff/16001/chrome/installer/util/work_...
chrome/installer/util/work_item_list.h:38: virtual void AddWorkItem(WorkItem*
work_item);
On 2011/01/12 16:31:49, erikwright wrote:
> I would prefer this to be split into a pure-virtual class and an
implementation.
> Otherwise, someone can add a new method, forget to add a mock for it, and the
> tests will actually call the implementation method (wreaking havoc on the
> builder).

I completely agree, but this leads in to the WorkItem/WorkItemList refactoring
that we'd also liketo perform to remove the redundant double factory
implementation we have now. I would like to defer this for now and leave a TODO
to do as you suggest, if you don't strongly object.

[grt (UTC plus 2), 2011-01-13 18:18:05]

lgtm!  you rock, dude.

[tommi (sloooow) - chröme, 2011-01-13 18:29:20]

lgtm!

[erikwright (departed), 2011-01-13 19:42:05]

On 2011/01/13 18:29:20, tommi wrote:
> lgtm!

LGTM.