Refactor history menu tests for clearer setup

Refactor history menu tests for clearer setup

In https://crrev.com/c2aa85e42d6d85b15e46e82f2df516e2e896bd1b, I had to
fix a couple of test bugs including a wrong hard-coded array index.

This switches initialization to a hopefully-less-error-prone form.

BUG=633689
Committed: https://crrev.com/407f858ac218dc95d41e32d8752ec938b1d90ff3
Cr-Commit-Position: refs/heads/master@{#409331}

[Sidney San Martín, 2016-07-29 20:21:32]

sdy@chromium.org changed reviewers:
+ rsesek@chromium.org

[Sidney San Martín, 2016-07-29 20:21:33]

https://codereview.chromium.org/2192253002/diff/1/chrome/browser/ui/cocoa/his...
File chrome/browser/ui/cocoa/history_menu_bridge_unittest.mm (right):

https://codereview.chromium.org/2192253002/diff/1/chrome/browser/ui/cocoa/his...
chrome/browser/ui/cocoa/history_menu_bridge_unittest.mm:53: class TestEntries {
This class is longer/nastier than I wanted — if anyone sees ways to
shorten/simplify it, let me know.

[Sidney San Martín, 2016-07-29 20:22:33]

Description was changed from

==========
Refactor history menu tests to share code

In https://crrev.com/c2aa85e42d6d85b15e46e82f2df516e2e896bd1b, I had to
fix a couple of test bugs, including changing a hard-coded array index
(which set one `Tab`'s id twice instead of two different `Tab`s' ids).

So, replace the repetative copy/pasted test code with a declarative form
that should be less error-prone and easier to read.

BUG=
==========

to

==========
Refactor history menu tests to share code

In https://crrev.com/c2aa85e42d6d85b15e46e82f2df516e2e896bd1b, I had to
fix a couple of test bugs, including changing a hard-coded array index
(which set one `Tab`'s id twice instead of two different `Tab`s' ids).

So, replace the repetitive copy/pasted test code with a declarative form
that should be less error-prone and easier to read.

BUG=
==========

[Robert Sesek, 2016-07-29 21:37:26]

These are mostly style comments. But I'm not sure if this is the right direction
to take this code. This adds quite a bit of structure, for just two tests, and
actually increases the total amount of code in the file. Tests in Chromium do
generally use declarative/straight-line style, with some helper functions (like
AddItemToMenu/CreateItem).

https://codereview.chromium.org/2192253002/diff/1/chrome/browser/ui/cocoa/his...
File chrome/browser/ui/cocoa/history_menu_bridge_unittest.mm (right):

https://codereview.chromium.org/2192253002/diff/1/chrome/browser/ui/cocoa/his...
chrome/browser/ui/cocoa/history_menu_bridge_unittest.mm:10: #include <vector>
#include <initializer_list>

https://codereview.chromium.org/2192253002/diff/1/chrome/browser/ui/cocoa/his...
chrome/browser/ui/cocoa/history_menu_bridge_unittest.mm:57: virtual
std::unique_ptr<MockTRS::Entry> toTRS() const = 0;
naming: always use CamelCase for method names, unless it is a simple accessor,
in which case you use under_scores. This site and throughout.

https://google.github.io/styleguide/cppguide.html#Function_Names

https://codereview.chromium.org/2192253002/diff/1/chrome/browser/ui/cocoa/his...
chrome/browser/ui/cocoa/history_menu_bridge_unittest.mm:68: std::string url,
title;
Don't declare member variables using comma notation.

https://codereview.chromium.org/2192253002/diff/1/chrome/browser/ui/cocoa/his...
chrome/browser/ui/cocoa/history_menu_bridge_unittest.mm:70:
Tab(SessionID::id_type id_, std::string url_, std::string title_)
Take std::string arguments as const&, and the argument names shouldn't end in _
(trailing underscores are reserved for member variables).

https://codereview.chromium.org/2192253002/diff/1/chrome/browser/ui/cocoa/his...
chrome/browser/ui/cocoa/history_menu_bridge_unittest.mm:87: void test(const
MockBridge::HistoryItem& historyItem,
naming: unless this is an ObjC method, variables always use under_scores

https://codereview.chromium.org/2192253002/diff/1/chrome/browser/ui/cocoa/his...
chrome/browser/ui/cocoa/history_menu_bridge_unittest.mm:114:
EXPECT_EQ(tabs.size() + 2, (size_t)submenu.numberOfItems);
numberOfItems should be NSUInteger and so should be testable without needing a
cast.

https://codereview.chromium.org/2192253002/diff/1/chrome/browser/ui/cocoa/his...
chrome/browser/ui/cocoa/history_menu_bridge_unittest.mm:122: private:
Style does not mix public and private. private always comes at the end.

[Sidney San Martín, 2016-07-29 22:02:17]

On 2016/07/29 21:37:26, Robert Sesek wrote:
> These are mostly style comments. But I'm not sure if this is the right
direction
> to take this code. This adds quite a bit of structure, for just two tests, and
> actually increases the total amount of code in the file.

Yeah, you're right. This grew out of a frustrating debugging session that ended
up being a wrong array index in one of the tests, and I wanted to design that
out… but this fix is inelegant. It came out long and complex, and I was hoping I
missed a good compromise.

[Sidney San Martín, 2016-07-29 22:02:34]

https://codereview.chromium.org/2192253002/diff/1/chrome/browser/ui/cocoa/his...
File chrome/browser/ui/cocoa/history_menu_bridge_unittest.mm (right):

https://codereview.chromium.org/2192253002/diff/1/chrome/browser/ui/cocoa/his...
chrome/browser/ui/cocoa/history_menu_bridge_unittest.mm:70:
Tab(SessionID::id_type id_, std::string url_, std::string title_)
On 2016/07/29 21:37:25, Robert Sesek wrote:
> Take std::string arguments as const&, and the argument names shouldn't end in
_
> (trailing underscores are reserved for member variables).

WRT underscores, it's to avoid shadowing the members — what do you folks usually
do in a constructor like this?

https://codereview.chromium.org/2192253002/diff/1/chrome/browser/ui/cocoa/his...
chrome/browser/ui/cocoa/history_menu_bridge_unittest.mm:122: private:
On 2016/07/29 21:37:25, Robert Sesek wrote:
> Style does not mix public and private. private always comes at the end.

I like that. When I was writing this I thought entries_ would depend on those
structs, but it turns out it doesn't.

[Robert Sesek, 2016-08-02 14:39:25]

https://codereview.chromium.org/2192253002/diff/1/chrome/browser/ui/cocoa/his...
File chrome/browser/ui/cocoa/history_menu_bridge_unittest.mm (right):

https://codereview.chromium.org/2192253002/diff/1/chrome/browser/ui/cocoa/his...
chrome/browser/ui/cocoa/history_menu_bridge_unittest.mm:70:
Tab(SessionID::id_type id_, std::string url_, std::string title_)
On 2016/07/29 22:02:33, Sidney San Martín wrote:
> On 2016/07/29 21:37:25, Robert Sesek wrote:
> > Take std::string arguments as const&, and the argument names shouldn't end
in
> _
> > (trailing underscores are reserved for member variables).
> 
> WRT underscores, it's to avoid shadowing the members — what do you folks
usually
> do in a constructor like this?

I use a_var or an_var, indirect through the this pointer, or make the members
private and put a trailing underscore (i.e. convert a struct to a class).

[Sidney San Martín, 2016-08-02 14:58:08]

Description was changed from

==========
Refactor history menu tests to share code

In https://crrev.com/c2aa85e42d6d85b15e46e82f2df516e2e896bd1b, I had to
fix a couple of test bugs, including changing a hard-coded array index
(which set one `Tab`'s id twice instead of two different `Tab`s' ids).

So, replace the repetitive copy/pasted test code with a declarative form
that should be less error-prone and easier to read.

BUG=
==========

to

==========
Refactor history menu tests for clearer setup

In https://crrev.com/c2aa85e42d6d85b15e46e82f2df516e2e896bd1b, I had to
fix a couple of test bugs including a wrong hard-coded array index.

This switches initialization to a hopefully-less-error-prone form.

BUG=
==========

[Sidney San Martín, 2016-08-02 15:01:03]

I found myself back in this file and wanted to try a less convoluted change.
What do you think?

[Sidney San Martín, 2016-08-02 15:01:47]

On 2016/08/02 14:39:25, Robert Sesek wrote:
> I use a_var or an_var, indirect through the this pointer, or make the members
> private and put a trailing underscore (i.e. convert a struct to a class).

Cool, thanks!

[Robert Sesek, 2016-08-02 15:06:28]

Yes, this is much more Chromium-style for unittest code.

https://codereview.chromium.org/2192253002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/history_menu_bridge_unittest.mm (right):

https://codereview.chromium.org/2192253002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/history_menu_bridge_unittest.mm:8: 
#include <initializer_list>

https://codereview.chromium.org/2192253002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/history_menu_bridge_unittest.mm:111: auto tab = new
MockTRS::Tab;
Who owns this object? It doesn't look like anything deletes this (same with line
123).

[Sidney San Martín, 2016-08-02 15:47:03]

Patchset #3 (id:40001) has been deleted

[Sidney San Martín, 2016-08-02 16:27:14]

The fix for ownership isn't great, but it should go away in another CL later
today.

An alternative would just be a loop at the bottom of each test:

    for (auto* entry : entries) {
      delete entry;
    }

…but I like the scoped nature of the current solution. Let me know what you
think. I'm only using raw pointers at all because `unique_ptr`s can't be passed
through an initializer list, which is frustrating.

https://codereview.chromium.org/2192253002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/history_menu_bridge_unittest.mm (right):

https://codereview.chromium.org/2192253002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/history_menu_bridge_unittest.mm:8: 
On 2016/08/02 15:06:27, Robert Sesek wrote:
> #include <initializer_list>

Done. Is there a tool to help catch indirectly-included headers?

https://codereview.chromium.org/2192253002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/history_menu_bridge_unittest.mm:111: auto tab = new
MockTRS::Tab;
On 2016/08/02 15:06:27, Robert Sesek wrote:
> Who owns this object? It doesn't look like anything deletes this (same with
line
> 123).

Oof, good catch. `Entries` and `Window::tabs` both hold `unique_ptr`s and would
take ownership. …as of another CL that I didn't submit yet :).

[Robert Sesek, 2016-08-02 18:47:38]

https://codereview.chromium.org/2192253002/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/history_menu_bridge_unittest.mm (right):

https://codereview.chromium.org/2192253002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/history_menu_bridge_unittest.mm:100: MockTRS::Entries
CreateSessionEntries(
Rather than this being a "Create" function, this could be called with a
vector<unique_ptr<MockTRS::Entry>> and return the unwrapped version instead.
That removes the weird ownership dance.

Something like this in code:

std::vector<std::unique_ptr<MockTRS::Entry>> owned_entries{
  CreateSessionTab(…),
  CreateSessionWindow(…),
};
MockTRS::Entry entries = UnwrapEntries(owned_entries);

https://codereview.chromium.org/2192253002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/history_menu_bridge_unittest.mm:102:
std::initializer_list<MockTRS::Entry*> l) {
naming: something more descriptive than |l|

https://codereview.chromium.org/2192253002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/history_menu_bridge_unittest.mm:112: MockTRS::Tab*
CreateSessionTab(SessionID::id_type id,
This could return a unique_ptr<MockTRS::Tab>.

https://codereview.chromium.org/2192253002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/history_menu_bridge_unittest.mm:131:
window->tabs.emplace_back(std::move(*tab));
This line is very hard to scrutinize (I've read it for five minutes). I guess
you're moving the underlying Tab, but does that mean |tab| is then inconsistent
after this, but that's OK because you immediately delete it?

https://codereview.chromium.org/2192253002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/history_menu_bridge_unittest.mm:132: delete tab;
... so that you don't need to manually delete here.

https://codereview.chromium.org/2192253002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/history_menu_bridge_unittest.mm:274:
std::vector<std::unique_ptr<MockTRS::Entry>> holdEntries;
naming: hold_entries

[Sidney San Martín, 2016-08-02 19:42:34]

Patchset #4 (id:80001) has been deleted

[Sidney San Martín, 2016-08-02 19:47:35]

The catch with the memory management changes you suggested is that
std::unique_ptr can't be passed in an initializer list. See
http://stackoverflow.com/a/8468817/84745. Let me know if you can think of a
nicer alternative.

https://codereview.chromium.org/2192253002/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/history_menu_bridge_unittest.mm (right):

https://codereview.chromium.org/2192253002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/history_menu_bridge_unittest.mm:102:
std::initializer_list<MockTRS::Entry*> l) {
On 2016/08/02 18:47:38, Robert Sesek wrote:
> naming: something more descriptive than |l|

Sure. I picked `l` because I remember seeing it as a convention in example code
but, now that I look again, it may not be.

https://codereview.chromium.org/2192253002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/history_menu_bridge_unittest.mm:274:
std::vector<std::unique_ptr<MockTRS::Entry>> holdEntries;
On 2016/08/02 18:47:38, Robert Sesek wrote:
> naming: hold_entries

Done.

[Sidney San Martín, 2016-08-02 20:11:44]

On 2016/08/02 18:47:38, Robert Sesek wrote:
>
https://codereview.chromium.org/2192253002/diff/60001/chrome/browser/ui/cocoa...
> chrome/browser/ui/cocoa/history_menu_bridge_unittest.mm:131:
> window->tabs.emplace_back(std::move(*tab));
> This line is very hard to scrutinize (I've read it for five minutes). I guess
> you're moving the underlying Tab, but does that mean |tab| is then
inconsistent
> after this, but that's OK because you immediately delete it?

FWIW, this line gets nicer when the containers use unique_ptrs:
https://codereview.chromium.org/2200993004/diff/1/chrome/browser/ui/cocoa/his...

[Robert Sesek, 2016-08-02 20:44:05]

LGTM. With the follow-up CL that does get better.

[Robert Sesek, 2016-08-02 20:44:30]

Did you want to set BUG= in the description?

[Sidney San Martín, 2016-08-02 20:45:55]

On 2016/08/02 20:44:30, Robert Sesek wrote:
> Did you want to set BUG= in the description?

I don't think there is a bug for the test refactor. Should there be (or should I
piggyback on crbug.com/633689)?

[Robert Sesek, 2016-08-02 20:46:34]

On 2016/08/02 20:45:55, Sidney San Martín wrote:
> On 2016/08/02 20:44:30, Robert Sesek wrote:
> > Did you want to set BUG= in the description?
> 
> I don't think there is a bug for the test refactor. Should there be (or should
I
> piggyback on crbug.com/633689)?

Bugs don't need to be 1:1 CLs. You can definitely have more than one CL per bug.

[Sidney San Martín, 2016-08-02 20:47:14]

Description was changed from

==========
Refactor history menu tests for clearer setup

In https://crrev.com/c2aa85e42d6d85b15e46e82f2df516e2e896bd1b, I had to
fix a couple of test bugs including a wrong hard-coded array index.

This switches initialization to a hopefully-less-error-prone form.

BUG=
==========

to

==========
Refactor history menu tests for clearer setup

In https://crrev.com/c2aa85e42d6d85b15e46e82f2df516e2e896bd1b, I had to
fix a couple of test bugs including a wrong hard-coded array index.

This switches initialization to a hopefully-less-error-prone form.

BUG=633689
==========

[Sidney San Martín, 2016-08-02 20:47:29]

The CQ bit was checked by sdy@chromium.org

[commit-bot: I haz the power, 2016-08-02 20:47:55]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-02 21:34:50]

Description was changed from

==========
Refactor history menu tests for clearer setup

In https://crrev.com/c2aa85e42d6d85b15e46e82f2df516e2e896bd1b, I had to
fix a couple of test bugs including a wrong hard-coded array index.

This switches initialization to a hopefully-less-error-prone form.

BUG=633689
==========

to

==========
Refactor history menu tests for clearer setup

In https://crrev.com/c2aa85e42d6d85b15e46e82f2df516e2e896bd1b, I had to
fix a couple of test bugs including a wrong hard-coded array index.

This switches initialization to a hopefully-less-error-prone form.

BUG=633689
==========

[commit-bot: I haz the power, 2016-08-02 21:34:51]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2016-08-02 21:36:47]

Description was changed from

==========
Refactor history menu tests for clearer setup

In https://crrev.com/c2aa85e42d6d85b15e46e82f2df516e2e896bd1b, I had to
fix a couple of test bugs including a wrong hard-coded array index.

This switches initialization to a hopefully-less-error-prone form.

BUG=633689
==========

to

==========
Refactor history menu tests for clearer setup

In https://crrev.com/c2aa85e42d6d85b15e46e82f2df516e2e896bd1b, I had to
fix a couple of test bugs including a wrong hard-coded array index.

This switches initialization to a hopefully-less-error-prone form.

BUG=633689

Committed: https://crrev.com/407f858ac218dc95d41e32d8752ec938b1d90ff3
Cr-Commit-Position: refs/heads/master@{#409331}
==========

[commit-bot: I haz the power, 2016-08-02 21:36:49]

Patchset 4 (id:??) landed as
https://crrev.com/407f858ac218dc95d41e32d8752ec938b1d90ff3
Cr-Commit-Position: refs/heads/master@{#409331}