Simplify WorkItemListTest.

Simplify WorkItemListTest.

Clean up the test to improve clarity and coverage, and reduce
dependencies on specific WorkItem implementations. Move
ConditionalWorkItemList tests to their own file.

BUG=601936
Committed: https://crrev.com/aa0a10c2ac4e96cb4b66130993f70bb33bb8bcbb
Cr-Commit-Position: refs/heads/master@{#388055}

[fdoray, 2016-04-12 18:18:40]

fdoray@chromium.org changed reviewers:
+ grt@chromium.org

[fdoray, 2016-04-12 18:18:41]

Can you review this CL? Thanks.

[grt (UTC plus 2), 2016-04-12 19:47:06]

https://codereview.chromium.org/1869383003/diff/1/chrome/installer/util/work_...
File chrome/installer/util/work_item_list_unittest.cc (right):

https://codereview.chromium.org/1869383003/diff/1/chrome/installer/util/work_...
chrome/installer/util/work_item_list_unittest.cc:50: std::vector<WorkItem*>
out(in.size());
how about:
  return std::vector<WorkItem*>(in.rbegin(), in.rend());

https://codereview.chromium.org/1869383003/diff/1/chrome/installer/util/work_...
chrome/installer/util/work_item_list_unittest.cc:59:
std::unique_ptr<WorkItemList> work_item_list(WorkItem::CreateWorkItemList());
this seems like a perfect use of GoogleMock. i think the tests and the
expectations can be laid out very cleanly without loops and holding intermediate
state. what do you think?

{
  std::unique_ptr<WorkItemList> work_item_list(WorkItem::CreateWorkItemList());

  // Create the mock work items.
  std::unique_ptr<MockWorkItem> item1(new MockWorkItem);
  std::unique_ptr<MockWorkItem> item2(new MockWorkItem);
  std::unique_ptr<MockWorkItem> item3(new MockWorkItem);

  {
    // Expect all three items to be done in order then undone.
    InSequence s;

    EXPECT_CALL(*item1, Do()).WillOnce(Return(true));
    EXPECT_CALL(*item2, Do()).WillOnce(Return(true));
    EXPECT_CALL(*item3, Do()).WillOnce(Return(true));
    EXPECT_CALL(*item3, Rollback());
    EXPECT_CALL(*item2, Rollback());
    EXPECT_CALL(*item1, Rollback());
  }

  // Add the items to the list.
  list->AddWorkItem(item1.release());
  list->AddWorkItem(item2.release());
  list->AddWorkItem(item3.release());

  // Do it!
  EXPECT_TRUE(work_item_list->Do());
  work_item_list->Rollback();
}

[fdoray, 2016-04-13 15:07:09]

Can you take another look? Thanks.

https://codereview.chromium.org/1869383003/diff/1/chrome/installer/util/work_...
File chrome/installer/util/work_item_list_unittest.cc (right):

https://codereview.chromium.org/1869383003/diff/1/chrome/installer/util/work_...
chrome/installer/util/work_item_list_unittest.cc:50: std::vector<WorkItem*>
out(in.size());
On 2016/04/12 19:47:06, grt (very slow) wrote:
> how about:
>   return std::vector<WorkItem*>(in.rbegin(), in.rend());

n/a

https://codereview.chromium.org/1869383003/diff/1/chrome/installer/util/work_...
chrome/installer/util/work_item_list_unittest.cc:59:
std::unique_ptr<WorkItemList> work_item_list(WorkItem::CreateWorkItemList());
On 2016/04/12 19:47:06, grt (very slow) wrote:
> this seems like a perfect use of GoogleMock. i think the tests and the
> expectations can be laid out very cleanly without loops and holding
intermediate
> state. what do you think?
> 
> {
>   std::unique_ptr<WorkItemList>
work_item_list(WorkItem::CreateWorkItemList());
> 
>   // Create the mock work items.
>   std::unique_ptr<MockWorkItem> item1(new MockWorkItem);
>   std::unique_ptr<MockWorkItem> item2(new MockWorkItem);
>   std::unique_ptr<MockWorkItem> item3(new MockWorkItem);
> 
>   {
>     // Expect all three items to be done in order then undone.
>     InSequence s;
> 
>     EXPECT_CALL(*item1, Do()).WillOnce(Return(true));
>     EXPECT_CALL(*item2, Do()).WillOnce(Return(true));
>     EXPECT_CALL(*item3, Do()).WillOnce(Return(true));
>     EXPECT_CALL(*item3, Rollback());
>     EXPECT_CALL(*item2, Rollback());
>     EXPECT_CALL(*item1, Rollback());
>   }
> 
>   // Add the items to the list.
>   list->AddWorkItem(item1.release());
>   list->AddWorkItem(item2.release());
>   list->AddWorkItem(item3.release());
> 
>   // Do it!
>   EXPECT_TRUE(work_item_list->Do());
>   work_item_list->Rollback();
> }

Done.

[grt (UTC plus 2), 2016-04-13 15:37:08]

so much nicer than the original test!

https://codereview.chromium.org/1869383003/diff/20001/chrome/installer/util/w...
File chrome/installer/util/work_item_list_unittest.cc (right):

https://codereview.chromium.org/1869383003/diff/20001/chrome/installer/util/w...
chrome/installer/util/work_item_list_unittest.cc:102: new
ConditionRunIfFileExists(temp_dir.path())));
this is testing both CWIL and CRIFFE. could the latter be in its own test, and
this test simplified to using a mock Condition?

[fdoray, 2016-04-13 16:46:45]

Can you take another look? Thanks.

https://codereview.chromium.org/1869383003/diff/20001/chrome/installer/util/w...
File chrome/installer/util/work_item_list_unittest.cc (right):

https://codereview.chromium.org/1869383003/diff/20001/chrome/installer/util/w...
chrome/installer/util/work_item_list_unittest.cc:102: new
ConditionRunIfFileExists(temp_dir.path())));
On 2016/04/13 15:37:07, grt wrote:
> this is testing both CWIL and CRIFFE. could the latter be in its own test, and
> this test simplified to using a mock Condition?

Done.

[grt (UTC plus 2), 2016-04-13 18:08:51]

https://codereview.chromium.org/1869383003/diff/20001/chrome/installer/util/w...
File chrome/installer/util/work_item_list_unittest.cc (right):

https://codereview.chromium.org/1869383003/diff/20001/chrome/installer/util/w...
chrome/installer/util/work_item_list_unittest.cc:102: new
ConditionRunIfFileExists(temp_dir.path())));
On 2016/04/13 16:46:45, fdoray wrote:
> On 2016/04/13 15:37:07, grt wrote:
> > this is testing both CWIL and CRIFFE. could the latter be in its own test,
and
> > this test simplified to using a mock Condition?
> 
> Done.

Sorry, forgot to mention before that these belong in
conditional_work_item_list_unittest.cc, no?

https://codereview.chromium.org/1869383003/diff/60001/chrome/installer/util/w...
File chrome/installer/util/work_item_list_unittest.cc (right):

https://codereview.chromium.org/1869383003/diff/60001/chrome/installer/util/w...
chrome/installer/util/work_item_list_unittest.cc:30: class AlwaysTrueCondition :
public WorkItem::Condition {
why not use a Mock for this so that the trueness and falseness can be within the
test body via WillOnce(Return(true/false))?

[fdoray, 2016-04-13 18:57:16]

https://codereview.chromium.org/1869383003/diff/20001/chrome/installer/util/w...
File chrome/installer/util/work_item_list_unittest.cc (right):

https://codereview.chromium.org/1869383003/diff/20001/chrome/installer/util/w...
chrome/installer/util/work_item_list_unittest.cc:102: new
ConditionRunIfFileExists(temp_dir.path())));
On 2016/04/13 18:08:51, grt wrote:
> On 2016/04/13 16:46:45, fdoray wrote:
> > On 2016/04/13 15:37:07, grt wrote:
> > > this is testing both CWIL and CRIFFE. could the latter be in its own test,
> and
> > > this test simplified to using a mock Condition?
> > 
> > Done.
> 
> Sorry, forgot to mention before that these belong in
> conditional_work_item_list_unittest.cc, no?

Done.

https://codereview.chromium.org/1869383003/diff/60001/chrome/installer/util/w...
File chrome/installer/util/work_item_list_unittest.cc (right):

https://codereview.chromium.org/1869383003/diff/60001/chrome/installer/util/w...
chrome/installer/util/work_item_list_unittest.cc:30: class AlwaysTrueCondition :
public WorkItem::Condition {
On 2016/04/13 18:08:51, grt wrote:
> why not use a Mock for this so that the trueness and falseness can be within
the
> test body via WillOnce(Return(true/false))?

Done.

[grt (UTC plus 2), 2016-04-14 14:42:23]

lgtm w/ one suggestion

https://codereview.chromium.org/1869383003/diff/100001/chrome/installer/util/...
File chrome/installer/util/work_item_list_unittest.cc (right):

https://codereview.chromium.org/1869383003/diff/100001/chrome/installer/util/...
chrome/installer/util/work_item_list_unittest.cc:16: class MockWorkItem : public
WorkItem {
how about pulling this out into something like work_item_mocks.h (along with the
condition mock) so that these can be building blocks re-used by other tests?
this will avoid the duplication in the two unittests.

[fdoray, 2016-04-18 13:04:46]

https://codereview.chromium.org/1869383003/diff/100001/chrome/installer/util/...
File chrome/installer/util/work_item_list_unittest.cc (right):

https://codereview.chromium.org/1869383003/diff/100001/chrome/installer/util/...
chrome/installer/util/work_item_list_unittest.cc:16: class MockWorkItem : public
WorkItem {
On 2016/04/14 14:42:22, grt wrote:
> how about pulling this out into something like work_item_mocks.h (along with
the
> condition mock) so that these can be building blocks re-used by other tests?
> this will avoid the duplication in the two unittests.

Done.

[fdoray, 2016-04-18 13:04:49]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2016-04-18 13:04:50]

The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org
Link to the patchset: https://codereview.chromium.org/1869383003/#ps120001
(title: "move mocks to their own file")

[commit-bot: I haz the power, 2016-04-18 13:04:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1869383003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1869383003/120001

[commit-bot: I haz the power, 2016-04-18 13:06:53]

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

[commit-bot: I haz the power, 2016-04-18 13:06:54]

Try jobs failed on following builders:
  ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[fdoray, 2016-04-18 15:11:25]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2016-04-18 15:11:25]

The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org
Link to the patchset: https://codereview.chromium.org/1869383003/#ps140001
(title: "rebase")

[commit-bot: I haz the power, 2016-04-18 15:11:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1869383003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1869383003/140001

[commit-bot: I haz the power, 2016-04-18 16:10:22]

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

[commit-bot: I haz the power, 2016-04-18 16:10:23]

Try jobs failed on following builders:
  win_clang on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[fdoray, 2016-04-18 16:41:54]

Description was changed from

==========
Simplify WorkItemListTest.

I had to write new tests for WorkItemList after I added the
best effort/no rollback flags. I felt that the existing tests could be
simplified.

BUG=601936
==========

to

==========
Simplify WorkItemListTest.

Clean up the test to improve clarity and coverage, and reduce
dependencies on specific WorkItem implementations. Also moved
ConditionalWorkItemList tests to their own file.

BUG=601936
==========

[fdoray, 2016-04-18 16:42:13]

Description was changed from

==========
Simplify WorkItemListTest.

Clean up the test to improve clarity and coverage, and reduce
dependencies on specific WorkItem implementations. Also moved
ConditionalWorkItemList tests to their own file.

BUG=601936
==========

to

==========
Simplify WorkItemListTest.

Clean up the test to improve clarity and coverage, and reduce
dependencies on specific WorkItem implementations. Move
ConditionalWorkItemList tests to their own file.

BUG=601936
==========

[fdoray, 2016-04-18 16:45:52]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2016-04-18 16:45:52]

The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org
Link to the patchset: https://codereview.chromium.org/1869383003/#ps180001
(title: "add missing file")

[commit-bot: I haz the power, 2016-04-18 16:46:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1869383003/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1869383003/180001

[commit-bot: I haz the power, 2016-04-18 17:38:23]

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

[commit-bot: I haz the power, 2016-04-18 17:38:24]

Try jobs failed on following builders:
  win_clang on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[fdoray, 2016-04-18 20:44:06]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2016-04-18 20:44:06]

The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org
Link to the patchset: https://codereview.chromium.org/1869383003/#ps200001
(title: "no inline destructors")

[commit-bot: I haz the power, 2016-04-18 20:44:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1869383003/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1869383003/200001

[commit-bot: I haz the power, 2016-04-18 22:33:29]

Description was changed from

==========
Simplify WorkItemListTest.

Clean up the test to improve clarity and coverage, and reduce
dependencies on specific WorkItem implementations. Move
ConditionalWorkItemList tests to their own file.

BUG=601936
==========

to

==========
Simplify WorkItemListTest.

Clean up the test to improve clarity and coverage, and reduce
dependencies on specific WorkItem implementations. Move
ConditionalWorkItemList tests to their own file.

BUG=601936
==========

[commit-bot: I haz the power, 2016-04-18 22:33:31]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-04-18 22:34:25]

Description was changed from

==========
Simplify WorkItemListTest.

Clean up the test to improve clarity and coverage, and reduce
dependencies on specific WorkItem implementations. Move
ConditionalWorkItemList tests to their own file.

BUG=601936
==========

to

==========
Simplify WorkItemListTest.

Clean up the test to improve clarity and coverage, and reduce
dependencies on specific WorkItem implementations. Move
ConditionalWorkItemList tests to their own file.

BUG=601936

Committed: https://crrev.com/aa0a10c2ac4e96cb4b66130993f70bb33bb8bcbb
Cr-Commit-Position: refs/heads/master@{#388055}
==========

[commit-bot: I haz the power, 2016-04-18 22:34:26]

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