Issue 2532793002: Add a TestBrowserDialog helper class for testing browser dialogs in a consistent way.

tapted

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

4 years ago (2016-11-28 00:45:43 UTC) #1

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2532793002/1

4 years ago (2016-11-28 00:45:51 UTC) #2

commit-bot: I haz the power

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

4 years ago (2016-11-28 01:32:26 UTC) #3

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/343008)

4 years ago (2016-11-28 01:32:27 UTC) #4

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2532793002/40001

4 years ago (2016-11-28 04:25:41 UTC) #6

commit-bot: I haz the power

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

4 years ago (2016-11-28 05:19:59 UTC) #7

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/343055)

4 years ago (2016-11-28 05:20:00 UTC) #8

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2532793002/60001

4 years ago (2016-11-28 06:23:39 UTC) #10

commit-bot: I haz the power

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

4 years ago (2016-11-28 07:29:56 UTC) #11

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/343076)

4 years ago (2016-11-28 07:29:57 UTC) #12

tapted

Description was changed from ========== Add a TEST_BROWSER_DIALOG Macro for testing browser dialogs in a ...

4 years ago (2016-11-28 07:37:51 UTC) #13

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2532793002/80001

4 years ago (2016-11-28 07:40:07 UTC) #15

commit-bot: I haz the power

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

4 years ago (2016-11-28 11:35:47 UTC) #17

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2532793002/120001

4 years ago (2016-11-29 08:13:31 UTC) #20

tapted

Description was changed from ========== Add a TEST_BROWSER_DIALOG Macro for testing browser dialogs in a ...

4 years ago (2016-11-29 08:19:05 UTC) #21

tapted

Description was changed from ========== Add a TEST_BROWSER_DIALOG Macro for testing browser dialogs in a ...

4 years ago (2016-11-29 08:19:31 UTC) #23

commit-bot: I haz the power

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

4 years ago (2016-11-29 09:21:04 UTC) #24

tapted

Hi Peter, please take a look. I think you're a good reviewer for this - ...

4 years ago (2016-11-29 11:58:10 UTC) #26

Peter Kasting

Let me see if I'm on the same page as you by restating what you ...

4 years ago (2016-11-30 00:08:24 UTC) #27

Let me see if I'm on the same page as you by restating what you said the purpose
here was.

The primary goal is to make it easy to manually audit these dialogs by providing
a simple way to list and invoke them all.  The fact that this uses the browser
test machinery is immaterial to this use case; in theory, we could choose to
implement this via switches to chrome.exe itself, or some other arbitrary
mechanism.  What's important is that there be _some_ method, since we need to
touch these (now, for Harmony, and probably eventually later), and we want to
make it easy for modifiers to check their changes.

The secondary goal is to facilitate other testing of these dialogs.  You
mentioned a few examples, though I'm not sure I understand dialogs enough to
grasp the examples you gave (for example, are there specific lifetime mistakes
this would let us check for?  Can you explain more what "ensuring
GetDialogButtons() is consistent with the behaviour of Accept/Cancel" means?). 
It's not totally clear to me whether any of these goals would be better served
with a unit test rather than a browser test.

Since I understand the primary goal better, I'll muse on it.  It seems like this
is a good thing.  I wonder if there is a way to generalize this to more than
just dialogs.  For the DIP->px work we'll be doing, I'd like to be able to test
any arbitrary view this way.  If we had a mechanism to "open a window and show a
particular named View in it at its preferred size", that'd be fantastic.  But I
don't know how feasible this is.  Presumably some Views are so closely tied to
their context that it's not very easy to display them standalone like this. 
Maybe there would be too much code overhead trying to provide ways to show
these.

I am not sure whether browser_tests is the right place for this, but I don't
know that that I can answer that better without understanding the secondary
goals more clearly.

Have you run this by anyone else?  robliao/kylixrd might be interested, as this
sort of relates to their desire to make Views easier to use/build/test in
general.

tapted

On 2016/11/30 00:08:24, Peter Kasting wrote: > Let me see if I'm on the same ...

4 years ago (2016-11-30 00:56:08 UTC) #28

On 2016/11/30 00:08:24, Peter Kasting wrote:
> Let me see if I'm on the same page as you by restating what you said the
purpose
> here was.
> 
> The primary goal is to make it easy to manually audit these dialogs by
providing
> a simple way to list and invoke them all.  The fact that this uses the browser
> test machinery is immaterial to this use case; in theory, we could choose to
> implement this via switches to chrome.exe itself, or some other arbitrary
> mechanism.  What's important is that there be _some_ method, since we need to
> touch these (now, for Harmony, and probably eventually later), and we want to
> make it easy for modifiers to check their changes.

Yes - that's right. However, I've discounted the option of using chrome.exe.
Although it would be nice, particularly for designers to invoke e.g. through a
scrappy chrome:// page, I don't think we can justify bundling the additional
dummy test data that may be required to invoke the dialog.

I chose browser_tests because quite a few browser dialogs already have a test
harness using InProcessBrowserTest -- I wanted the code/maintenance overheads of
incorporating a dialog into this framework to be as low as possible. The example
here for CardUnmaskPrompt is representative of this in some ways - there's quite
a lot of logic in autofill_test_utils.cc to generate test credit card object and
other test data. Adapting the test harness into this framework was
straightforward.

The follow-up in http://crrev.com/2534743002 requires an embedded test server,
and reads chrome/test/data/cookie1.html. But adapting the BrowserTest was easy.

Then I've got my eye on a bunch of extension dialogs -- e.g. installation,
uninstallation, changed permissions, etc. These are all really hard to invoke
manually, and appear in obscure places (e.g. a crash I fixed recently in
http://crrev.com/426302), and for specific APIs (e.g. showing bluetooth
devices). Many extension tests are using a test harness inherited from
ExtensionBrowserTest (which inherits from InProcessBrowserTest).

> 
> The secondary goal is to facilitate other testing of these dialogs.  You
> mentioned a few examples, though I'm not sure I understand dialogs enough to
> grasp the examples you gave (for example, are there specific lifetime mistakes
> this would let us check for?

Yes - one recent example is http://crrev.com/432320 "Fix memory leak for
extension uninstall dialog". This happened because the dialog didn't handle a
cascading Close() of the parent window. But we could easily subject all dialogs
opting in to this framework into a compulsory test for exactly that.

>  Can you explain more what "ensuring
> GetDialogButtons() is consistent with the behaviour of Accept/Cancel" means?).

DialogDelegate::GetDialogButtons() by default returns OK|CANCEL. I.e. all
dialogs get an OK and a cancel button. The override of GetDialogButtonLabel()
should be checked to ensure it returns labels for all the buttons. And then one
specific mistake on my mind is that DialogDelegate::Accept and
DialogDelegate::Cancel shouldn't actually dismiss the dialog, but some dialogs
do dismiss this way. This is a problem because triggering a close from there
invokes DialogClientView::CanClose() which may invoke Accept or Cancel a second
time (and, e.g., record UMA twice or do other weird stuff).  One culprit I came
across was permissions bubbles -- they have a lot of delegation - I think it
ends up in chrome/browser/permission_request_manager whose Accept() and Deny()
manipulate the bubble when really they should just be recording the action.

> It's not totally clear to me whether any of these goals would be better served
> with a unit test rather than a browser test.
> 
> Since I understand the primary goal better, I'll muse on it.  It seems like
this
> is a good thing.  I wonder if there is a way to generalize this to more than
> just dialogs.  For the DIP->px work we'll be doing, I'd like to be able to
test
> any arbitrary view this way.  If we had a mechanism to "open a window and show
a
> particular named View in it at its preferred size", that'd be fantastic.  But
I
> don't know how feasible this is.  Presumably some Views are so closely tied to
> their context that it's not very easy to display them standalone like this. 
> Maybe there would be too much code overhead trying to provide ways to show
> these.

views_examples_with_content_exe is handy for manual testing of a specific view.
But one side-issue is that views_examples (minus the content dependency) has a
history of fragility. E.g. http://crbug.com/640748 . There's a rather
heavyweight set up required to get an authentic multiprocess GPU/compositing
environment . E.g. convincing views_unittests to generate pixel output to screen
requires a bit of a dance, and it doesn't work on all platforms. Maintaining the
compositor bootstrapping in multiple targets requires a bit more investment
versus leaning on top of browser_tests, so that's why I went down that route.

> 
> I am not sure whether browser_tests is the right place for this, but I don't
> know that that I can answer that better without understanding the secondary
> goals more clearly.
> 
> Have you run this by anyone else?  robliao/kylixrd might be interested, as
this
> sort of relates to their desire to make Views easier to use/build/test in
> general.

Not really. We've had a frustrating time testing dialogs for Harmony, which
culminated in the attached http://crbug.com/654151 and a bunch of related bugs
via the crbug query in the bug description. Then I decided to explore some
things while everyone overseas was eating turkey :). I'll loop them in on the
bug.

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2532793002/140001

4 years ago (2016-12-06 05:59:39 UTC) #30

tapted

I've fleshed out some documentation and make a kind of design doc at https://goo.gl/ynxx1l (currently ...

4 years ago (2016-12-06 06:03:23 UTC) #31

commit-bot: I haz the power

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

4 years ago (2016-12-06 07:25:09 UTC) #32

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_swarming_rel/builds/80079)

4 years ago (2016-12-06 07:25:10 UTC) #33

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2532793002/160001

4 years ago (2016-12-14 08:11:16 UTC) #35

commit-bot: I haz the power

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

4 years ago (2016-12-14 10:15:52 UTC) #36

tapted

Hi Peter, PTAL. I think this is ready for review. Feedback on the doc (https://goo.gl/ynxx1l) ...

4 years ago (2016-12-14 11:36:05 UTC) #38

Peter Kasting

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc File chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc (right): https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc#newcode98 chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc:98: // InProcessBrowserTest: Nit: You don't directly subclass InProcessBrowserTest (or ...

4 years ago (2016-12-15 00:28:56 UTC) #39

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc (right):

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc:98: //
InProcessBrowserTest:
Nit: You don't directly subclass InProcessBrowserTest (or TestDialogInterface,
below).  Say "// DialogBrowserTest:"

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc:108:
ShowUI(static_cast<CreditCardExpiry>(index));
I've been trying to think of ways to simplify the number of functions, types,
casts, etc. here.

What if ShowDialog() was simply given the std::string the user actually passed?

We could keep NameProvider() if we wanted to use it to provide usage messages to
the user and sanity-check their input, or we could eliminate it if those weren't
important.

In this case, ShowDialog() and ShowUI() could be combined into a function that
did something like:

    CreditCard card = (expiry == "expired") ? test::GetMaskedServerCard()
                                            : test::GetMaskedServerCardAmex();

...this would treat all strings other than "expired" as "valid".  If you had
removed input sanity-checking and needed to do it here, you could do something
like:

  void ShowDialog(const std::string& argument) override {
    const bool is_expired = argument == "expired";
    if (!is_expired && argument != "valid")
      return;

    ... use |is_expired|

In either case, callers that today do ShowUI(CreditCardExpiry::EXPIRED); could
do ShowDialog("expired");.  If you want to define a string constant for this
(and then use that constant in NameProvider()?) you could do that as well.

This allows us to combine these two functions, which is a pattern expect would
repeat in other cases we add support for.  It would eliminate the enum here. 
And it would eliminate the need for the caller to map the input string to an
index, meaning NameProvider() could return a set rather than a vector, which
would probably be more well-suited for things like sanity-checking the input is
within the set of valid strings.

These are not large differences, so it's not clear to me that this is wildly
better overall, but the current design feels boilerplate-y.

Another simpler change would be to change CreditCardExpiry from an enum class to
an enum, move the contents of ShowUI() into ShowDialog(), and call ShowDialog()
directly from the various callers.

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
File chrome/browser/ui/test/test_browser_dialog.cc (right):

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.cc:21: using views::Widget;
Nit: You already explicitly qualify the majority of places below, so just
eliminate this and qualify the rest.

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.cc:26: const char
kInteractiveSwitch[] = "interactive";
Nit: Prefer constexpr to const for compile-time constants.  (Several places)

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.cc:44: CLOSE,        // Call
Widget::Close() and empty the run loop.
Nit: I feel like either you should implement these in this CL, or not add them
until you do implement them.

If you do remove them for now, WidgetCloser()'s code should be simplified as
well.

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.cc:98: const std::string separator =
".";
Nit: constexpr

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.cc:99: std::vector<std::string> names
= name_provider();
Nit: Can inline below

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.cc:120: if
(!command_line.HasSwitch(switches::kExtendMdToSecondaryUi))
I'm worried this is going to break in the future.

It seems like really you don't care about --secondary-ui-md, you care about
toolkit=views.  Does --secondary-ui-md result in some of command-line flag being
auto-added which means that directly, which you can test for?  If not, what are
the consequences of just not having this ifdef and letting things break if
people try to run them the wrong way?

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.cc:128: dialog_name, ".",
base::KEEP_WHITESPACE, base::SPLIT_WANT_ALL);
I think instead of using SplitString, you should find() the first '.' in the
string, and return the substring either before or after it.  This will allow the
dialog name strings to contain '.'s if people want.

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.cc:130: const std::string& case_name
= name_parts[1];
Nit: There are two blocks in this file that want to SplitString() the dialog
name, verify it has two parts, and then return one.  I suggest a helper
function.

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.cc:149: << "More than one child
dialog shown.";
Nit: It doesn't seem like the error messages add much.  I'd just remove these
and change the ASSERT below to ASSERT_EQ(1, added.size());.

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.cc:156: action =
DialogAction::INTERACTIVE;
Nit: Not shorter, but makes it clearer this is just a data type conversion, not
a control flow change:

  const DialogAction action = command_line.HasSwitch(kInteractiveSwitch)
                            ? DialogAction::INTERACTIVE
                            : DialogAction::CLOSE_NOW;

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.cc:174: std::cout << "\t" << name <<
"\n";
Did you mean to VLOG() this?  Or did you mean to cout the VLOG() above?

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.cc:178: return;  // Nothing
specified.
Nit: Put this in the above conditional.

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.cc:195:
command.AppendSwitch(switches::kExtendMdToSecondaryUi);
Same worry as earlier, but even more so.  It seems like it's the user's
responsibility to supply this if desirable.

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.cc:199: const std::string disable =
"10000000";
This is an ugly way of disabling these timeouts.  It also doesn't match the
kAlmostInfiniteTimeoutMs value in test_timeouts.cc (too few zeroes).

I would prefer to modify that code to allow 0 or -1 to mean "no timeout at all",
remove the concept of "almost infinite timeout" from there and here, and then
pass that signal value.

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
File chrome/browser/ui/test/test_browser_dialog.h (right):

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.h:19: // consistent way. E.g. This
allows screenshots to be generated easily, with the
Nit: Just remove "E.g."

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.h:24: // To use TEST_BROWSER_DIALOG,
a test harness inherits from DialogBrowserTest
Nit: inherits -> should inherit

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.h:32: //   // TestDialogInterface:
Nit: TestDialogInterface -> DialogBrowserTest

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.h:48: //
Nit: Not clear to me why some places have two blank lines instead of one.

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.h:73: #define
TEST_BROWSER_DIALOG(Harness)                                          \
Nit: Blank line above this so the comment is visually separated from the content

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.h:75:
TestDialogInterface::Register(#Harness, &Harness::NameProvider);        \
This violates the Google style guide, but I don't know another way of
accomplishing what you're trying to do.

Basically, you're modifying global state from a function that's called during
static initialization, which is banned, and which is the only real way for
TestDialogInterface to enumerate all the names without actually instantiating
each relevant harness, which isn't desirable either.

It would be nice to eliminate this, fix the style guide violation, and allow
harnesses to use virtual overrides instead of this weird static override thing. 
But I don't see an obvious way to do it.

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.h:82: using NameProviderFunction =
std::vector<std::string> (*)();
Nit: Unless I am mistaken, if you declare this type as a function rather than a
function pointer:

  using NameProviderFunction = std::vector<std::string> ();

Then in test files using this, you can declare NameProvider() as:

  NameProviderFunction NameProvider;

This makes it more clear that this function's signature is defined by this type,
and will slightly help with maintenance if for some reason this type changed
later.

You still have to define the function using the full type, I believe, which is a
bit inconvenient.

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.h:114: return
this->browser()->window()->GetNativeWindow();
Nit: Can't see why this-> is needed here.  Does it not compile without it?

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2532793002/180001

4 years ago (2016-12-15 05:02:09 UTC) #41

commit-bot: I haz the power

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

4 years ago (2016-12-15 06:07:46 UTC) #42

tapted

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc File chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc (right): https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc#newcode98 chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc:98: // InProcessBrowserTest: On 2016/12/15 00:28:54, Peter Kasting wrote: > ...

4 years ago (2016-12-15 06:08:23 UTC) #44

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc (right):

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc:98: //
InProcessBrowserTest:
On 2016/12/15 00:28:54, Peter Kasting wrote:
> Nit: You don't directly subclass InProcessBrowserTest (or TestDialogInterface,
> below).  Say "// DialogBrowserTest:"

Done.

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc:108:
ShowUI(static_cast<CreditCardExpiry>(index));
On 2016/12/15 00:28:54, Peter Kasting wrote:
> I've been trying to think of ways to simplify the number of functions, types,
> casts, etc. here.
> 
> What if ShowDialog() was simply given the std::string the user actually
passed?
> 
> We could keep NameProvider() if we wanted to use it to provide usage messages
to
> the user and sanity-check their input, or we could eliminate it if those
weren't
> important.
> In this case, ShowDialog() and ShowUI() could be combined into a function that
> did something like:
> 
>     CreditCard card = (expiry == "expired") ? test::GetMaskedServerCard()
>                                             : test::GetMaskedServerCardAmex();
> 
> ...this would treat all strings other than "expired" as "valid".  If you had
> removed input sanity-checking and needed to do it here, you could do something
> like:
> 
>   void ShowDialog(const std::string& argument) override {
>     const bool is_expired = argument == "expired";
>     if (!is_expired && argument != "valid")
>       return;
> 
>     ... use |is_expired|
> 
> In either case, callers that today do ShowUI(CreditCardExpiry::EXPIRED); could
> do ShowDialog("expired");.  If you want to define a string constant for this
> (and then use that constant in NameProvider()?) you could do that as well.
> 
> This allows us to combine these two functions, which is a pattern expect would
> repeat in other cases we add support for.  It would eliminate the enum here. 
> And it would eliminate the need for the caller to map the input string to an
> index, meaning NameProvider() could return a set rather than a vector, which
> would probably be more well-suited for things like sanity-checking the input
is
> within the set of valid strings.
> 
> These are not large differences, so it's not clear to me that this is wildly
> better overall, but the current design feels boilerplate-y.

I agree that the mapping in ShowDialog feels a bit clumsy :/. I do like the idea
of passing the string instead, but I think the net result will be more
boilerplate or redundant code in the test harness.

NameProvider() is important because it communicates/registers a list of
"available" dialog types in a central place. That is, one of the problems we
want to solve is that it's hard to be "systematic" when making a UI change --
Chrome has a lot of obscure dialogs that can do creative things (merging
permission requests comes to mind, or the "You are viewing a secure Google
Chrome page." version of the PageInfo bubble). Just registering the test
harness/class name doesn't communicate all the styles that should be checked to
someone unfamiliar with the code.

This doesn't rule out it being a std::set. However, given that we have a list of
strings "somewhere" (i.e. not just in a set of conditions) I wanted to reduce
the duplication of those strings (either as literals, or char[] constants). This
would likely mean ShowDialog(std::string) would do a lookup - e.g. using
NameProvider() itself to get an index, or in a supplementary mapping (**which
didn't need its own static initialization).

So, things might be neater if NameProvider returned a std::map with a known
mapped_type. However, since it's accessed during static initialization, we'd
need to be careful with what gets mapped. I think most client code would use an
`int`. So, providing `int` to ShowDialog just skips this this mapping. However,
it does then require that the mapped ints are contiguous, which isn't always
desirable.

See, e.g., the NameProvider for permissions prompts - _most_ but not all styles
are backed by a value from the content::PermissionType enum
https://codereview.chromium.org/2569703004/diff/40001/chrome/browser/permissi...
. This has both upsides and downsides: the code to make and map NameProvider
gets a little clumsy, but it _does_ force NameProvider to consider all the
different styles that may result from the various content::PermissionType
values.

> Another simpler change would be to change CreditCardExpiry from an enum class
to
> an enum, move the contents of ShowUI() into ShowDialog(), and call
ShowDialog()
> directly from the various callers.

Done.

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
File chrome/browser/ui/test/test_browser_dialog.cc (right):

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.cc:21: using views::Widget;
On 2016/12/15 00:28:55, Peter Kasting wrote:
> Nit: You already explicitly qualify the majority of places below, so just
> eliminate this and qualify the rest.

Done.

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.cc:26: const char
kInteractiveSwitch[] = "interactive";
On 2016/12/15 00:28:55, Peter Kasting wrote:
> Nit: Prefer constexpr to const for compile-time constants.  (Several places)

Done.

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.cc:44: CLOSE,        // Call
Widget::Close() and empty the run loop.
On 2016/12/15 00:28:54, Peter Kasting wrote:
> Nit: I feel like either you should implement these in this CL, or not add them
> until you do implement them.
> 
> If you do remove them for now, WidgetCloser()'s code should be simplified as
> well.

Done (added a TODO in the enum class comment)

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.cc:98: const std::string separator =
".";
On 2016/12/15 00:28:54, Peter Kasting wrote:
> Nit: constexpr

Sadly std::basic_string invokes malloc for this under most ABIs. This was
largely so I could concatenate two char[]s, but I neatened it up a bit further
(i.e. `prefix = harness + std::string(".")`)

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.cc:99: std::vector<std::string> names
= name_provider();
On 2016/12/15 00:28:55, Peter Kasting wrote:
> Nit: Can inline below

Done.

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.cc:120: if
(!command_line.HasSwitch(switches::kExtendMdToSecondaryUi))
On 2016/12/15 00:28:55, Peter Kasting wrote:
> I'm worried this is going to break in the future.
> 
> It seems like really you don't care about --secondary-ui-md, you care about
> toolkit=views.  Does --secondary-ui-md result in some of command-line flag
being
> auto-added which means that directly, which you can test for?  If not, what
are
> the consequences of just not having this ifdef and letting things break if
> people try to run them the wrong way?

Since MD dialogs on Mac are only implemented using toolkit-views,
--secondary-ui-md on Mac means that a dialog will be toolkit-views. (There are
no longer any other `toolkit-views` runtime flags for Mac).

Without this ifdef, a Cocoa dialog will appear, which won't be picked up below,
and the test will fail: "Dialog not shown, or not a toolkit-views Dialog". (A
`WidgetCloser` that works for native Cocoa dialogs is feasible, but may have a
limited lifespan).

I see one concern - if `--secondary-ui-md` effectively becomes default on Mac
but the switch isn't removed at the same time, then this would bail out
needlessly and lose out on some test coverage.

I've changed this to instead just force IsSecondaryUiMaterial() to report `true`
on Mac (i.e. change the current process). This needed a tweak to
MaterialDesignControllerTestAPI. There's an added bonus: the trybots on Mac will
start getting test coverage of the `InvokeDefault` case immediately, like other
platforms.

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.cc:128: dialog_name, ".",
base::KEEP_WHITESPACE, base::SPLIT_WANT_ALL);
On 2016/12/15 00:28:54, Peter Kasting wrote:
> I think instead of using SplitString, you should find() the first '.' in the
> string, and return the substring either before or after it.  This will allow
the
> dialog name strings to contain '.'s if people want.

Done.

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.cc:130: const std::string& case_name
= name_parts[1];
On 2016/12/15 00:28:54, Peter Kasting wrote:
> Nit: There are two blocks in this file that want to SplitString() the dialog
> name, verify it has two parts, and then return one.  I suggest a helper
> function.

Done.

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.cc:149: << "More than one child
dialog shown.";
On 2016/12/15 00:28:55, Peter Kasting wrote:
> Nit: It doesn't seem like the error messages add much.  I'd just remove these
> and change the ASSERT below to ASSERT_EQ(1, added.size());.

Done.

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.cc:156: action =
DialogAction::INTERACTIVE;
On 2016/12/15 00:28:55, Peter Kasting wrote:
> Nit: Not shorter, but makes it clearer this is just a data type conversion,
not
> a control flow change:
> 
>   const DialogAction action = command_line.HasSwitch(kInteractiveSwitch)
>                             ? DialogAction::INTERACTIVE
>                             : DialogAction::CLOSE_NOW;

Done.

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.cc:174: std::cout << "\t" << name <<
"\n";
On 2016/12/15 00:28:54, Peter Kasting wrote:
> Did you mean to VLOG() this?  Or did you mean to cout the VLOG() above?

Ah, that's a better idea :).

I wanted only the first line of output "decorated" with line numbers and things,
but I can make an ostringstream and VLOG after. (Done.)

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.cc:178: return;  // Nothing
specified.
On 2016/12/15 00:28:55, Peter Kasting wrote:
> Nit: Put this in the above conditional.

Done.

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.cc:195:
command.AppendSwitch(switches::kExtendMdToSecondaryUi);
On 2016/12/15 00:28:55, Peter Kasting wrote:
> Same worry as earlier, but even more so.  It seems like it's the user's
> responsibility to supply this if desirable.

Now removed with the change at the earlier comment.

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.cc:199: const std::string disable =
"10000000";
On 2016/12/15 00:28:55, Peter Kasting wrote:
> This is an ugly way of disabling these timeouts.  It also doesn't match the
> kAlmostInfiniteTimeoutMs value in test_timeouts.cc (too few zeroes).
> 
> I would prefer to modify that code to allow 0 or -1 to mean "no timeout at
all",
> remove the concept of "almost infinite timeout" from there and here, and then
> pass that signal value.

kAlmostInfiniteTimeoutMs is tricky to remove since it's needed to return a valid
base::TimeDelta(). There's a pasture of yaks to wade through :/

* base::TimeDelta should have `constextpr` constructors
* TestTimeouts's static members can then be base::TimeDelta
* base::debug::BeingDebugged() should result in action timeouts being
base::TimeDelta::Max()
* InitializeTimeout() in test_timeouts.cc needs to beware not to overflow when
multiplying base::TimeDelta values (e.g. for MSAN builds)
* test_timeout.cc should be using int64_t constants

I've gone for a compromise that just adds 

// static
const char TestTimeouts::kNoTimeoutSwitchValue[] = "-1";

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
File chrome/browser/ui/test/test_browser_dialog.h (right):

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.h:19: // consistent way. E.g. This
allows screenshots to be generated easily, with the
On 2016/12/15 00:28:55, Peter Kasting wrote:
> Nit: Just remove "E.g."

Done.

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.h:24: // To use TEST_BROWSER_DIALOG,
a test harness inherits from DialogBrowserTest
On 2016/12/15 00:28:55, Peter Kasting wrote:
> Nit: inherits -> should inherit

Done.

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.h:32: //   // TestDialogInterface:
On 2016/12/15 00:28:56, Peter Kasting wrote:
> Nit: TestDialogInterface -> DialogBrowserTest

Done.

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.h:48: //
On 2016/12/15 00:28:55, Peter Kasting wrote:
> Nit: Not clear to me why some places have two blank lines instead of one.

Got rid of the double-blank lines. (I think I was using them if a ~<code> block
was notionally at the end of a paragraph rather than inline, but I'm not fussed
and I like consistency :)).

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.h:73: #define
TEST_BROWSER_DIALOG(Harness)                                          \
On 2016/12/15 00:28:55, Peter Kasting wrote:
> Nit: Blank line above this so the comment is visually separated from the
content

Done.

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.h:75:
TestDialogInterface::Register(#Harness, &Harness::NameProvider);        \
On 2016/12/15 00:28:56, Peter Kasting wrote:
> This violates the Google style guide, but I don't know another way of
> accomplishing what you're trying to do.
> 
> Basically, you're modifying global state from a function that's called during
> static initialization, which is banned, and which is the only real way for
> TestDialogInterface to enumerate all the names without actually instantiating
> each relevant harness, which isn't desirable either.
> 
> It would be nice to eliminate this, fix the style guide violation, and allow
> harnesses to use virtual overrides instead of this weird static override
thing. 
> But I don't see an obvious way to do it.

Yeah, I was aware of this, but it wasn't a casual decision :/. gtest's
TEST[_FOO] macros actually do the same thing. (I hadn't checked earlier, but
assumed this was the mechanism involved. Sure enough, gtest-internal.h does:

// Helper macro for defining tests.
#define GTEST_TEST_(test_case_name, test_name, parent_class, parent_id)\
...
::testing::TestInfo* const GTEST_TEST_CLASS_NAME_(test_case_name, test_name)\
  ::test_info_ =\
    ::testing::internal::MakeAndRegisterTestInfo(\..)

So, I think this could be a specific use-case where it is informally accepted.
E.g., the approach here wouldn't be adding static initializers to any
translation unit that doesn't already have them for their own TEST_F macros.

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.h:82: using NameProviderFunction =
std::vector<std::string> (*)();
On 2016/12/15 00:28:55, Peter Kasting wrote:
> Nit: Unless I am mistaken, if you declare this type as a function rather than
a
> function pointer:
> 
>   using NameProviderFunction = std::vector<std::string> ();
> 
> Then in test files using this, you can declare NameProvider() as:
> 
>   NameProviderFunction NameProvider;
> 
> This makes it more clear that this function's signature is defined by this
type,
> and will slightly help with maintenance if for some reason this type changed
> later.
> 
> You still have to define the function using the full type, I believe, which is
a
> bit inconvenient.

Done.

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.h:114: return
this->browser()->window()->GetNativeWindow();
On 2016/12/15 00:28:56, Peter Kasting wrote:
> Nit: Can't see why this-> is needed here.  Does it not compile without it?

It's needed because browser() is a member of a template argument. Without it,
the compiler ejects "error: use of undeclared identifier 'browser'". An
alternative is

    return Base::browser()->window()->GetNativeWindow();

(Of course, my penchant is for "this->", but I don't feel strongly about it).

https://codereview.chromium.org/2532793002/diff/180001/chrome/browser/ui/test...
File chrome/browser/ui/test/test_browser_dialog.cc (right):

https://codereview.chromium.org/2532793002/diff/180001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.cc:204: options.wait = true;
I realised it was pretty pointless removing the timeouts on the subprocess if
they were still in effect on the parent. Leaving LaunchOptions::wait as false
means the subprocess outputs goes to an interactive shell, but the browser won't
die unexpectedly while you're trying to take a screenshot.

Peter Kasting

LGTM https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test/test_browser_dialog.cc File chrome/browser/ui/test/test_browser_dialog.cc (right): https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test/test_browser_dialog.cc#newcode120 chrome/browser/ui/test/test_browser_dialog.cc:120: if (!command_line.HasSwitch(switches::kExtendMdToSecondaryUi)) On 2016/12/15 06:08:22, tapted wrote: > ...

4 years ago (2016-12-15 08:12:29 UTC) #45

LGTM

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
File chrome/browser/ui/test/test_browser_dialog.cc (right):

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.cc:120: if
(!command_line.HasSwitch(switches::kExtendMdToSecondaryUi))
On 2016/12/15 06:08:22, tapted wrote:
> On 2016/12/15 00:28:55, Peter Kasting wrote:
> > I'm worried this is going to break in the future.
> > 
> > It seems like really you don't care about --secondary-ui-md, you care about
> > toolkit=views.  Does --secondary-ui-md result in some of command-line flag
> being
> > auto-added which means that directly, which you can test for?  If not, what
> are
> > the consequences of just not having this ifdef and letting things break if
> > people try to run them the wrong way?
> 
> Since MD dialogs on Mac are only implemented using toolkit-views,
> --secondary-ui-md on Mac means that a dialog will be toolkit-views. (There are
> no longer any other `toolkit-views` runtime flags for Mac).
> 
> Without this ifdef, a Cocoa dialog will appear, which won't be picked up
below,
> and the test will fail: "Dialog not shown, or not a toolkit-views Dialog". (A
> `WidgetCloser` that works for native Cocoa dialogs is feasible, but may have a
> limited lifespan).

And to be clear, this is a problem because:
* The test will run on the bots automatically, which don't pass this flag, so
just breaking in this case is Not OK
* You're trying to test Harmony UI outside the whole-shebang "Mac TOOLKIT_VIEWS"
build, so you can't #ifdef the test to bail when that's not set

> I've changed this to instead just force IsSecondaryUiMaterial() to report
`true`
> on Mac (i.e. change the current process). This needed a tweak to
> MaterialDesignControllerTestAPI. There's an added bonus: the trybots on Mac
will
> start getting test coverage of the `InvokeDefault` case immediately, like
other
> platforms.

I think that's probably a slightly better tradeoff.  All the solutions here have
downsides.

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.cc:199: const std::string disable =
"10000000";
On 2016/12/15 06:08:22, tapted wrote:
>  There's a pasture of yaks to wade through :/
> 
> * base::TimeDelta should have `constextpr` constructors
> * TestTimeouts's static members can then be base::TimeDelta
> * base::debug::BeingDebugged() should result in action timeouts being
> base::TimeDelta::Max()
> * InitializeTimeout() in test_timeouts.cc needs to beware not to overflow when
> multiplying base::TimeDelta values (e.g. for MSAN builds)
> * test_timeout.cc should be using int64_t constants

Feel free to file those yaks as bugs...

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
File chrome/browser/ui/test/test_browser_dialog.h (right):

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.h:82: using NameProviderFunction =
std::vector<std::string> (*)();
On 2016/12/15 06:08:23, tapted wrote:
> On 2016/12/15 00:28:55, Peter Kasting wrote:
> > Nit: Unless I am mistaken, if you declare this type as a function rather
than
> a
> > function pointer:
> > 
> >   using NameProviderFunction = std::vector<std::string> ();
> > 
> > Then in test files using this, you can declare NameProvider() as:
> > 
> >   NameProviderFunction NameProvider;
> > 
> > This makes it more clear that this function's signature is defined by this
> type,
> > and will slightly help with maintenance if for some reason this type changed
> > later.
> > 
> > You still have to define the function using the full type, I believe, which
is
> a
> > bit inconvenient.
> 
> Done.

I realize now this suggestion didn't actually help much since for the overriding
class in this CL, you define the "override" inline in the class declaration, and
I don't think this syntax can be used there:

  NameProviderFunction NameProvider {
    ...
  }

And probably future people will do something similar.

Oh well :/.  I guess this is available in case anyone is like me and likes doing
things out-of-line even in their test code.

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.h:114: return
this->browser()->window()->GetNativeWindow();
On 2016/12/15 06:08:22, tapted wrote:
> On 2016/12/15 00:28:56, Peter Kasting wrote:
> > Nit: Can't see why this-> is needed here.  Does it not compile without it?
> 
> It's needed because browser() is a member of a template argument. Without it,
> the compiler ejects "error: use of undeclared identifier 'browser'".

Ah, that makes sense.  I figured it was due to templates but I couldn't
immediately think why it was important.

I agree that this-> is better than Base:: in this case, since I at first misread
Base:: as meaning "this function is static".

https://codereview.chromium.org/2532793002/diff/180001/base/test/test_timeouts.h
File base/test/test_timeouts.h (right):

https://codereview.chromium.org/2532793002/diff/180001/base/test/test_timeout...
base/test/test_timeouts.h:17: static const char kNoTimeoutSwitchValue[];
Nit: Can this be declared constexpr?  Maybe by moving the initializer value
here?  (Probably still needs a definition point in the .cc file in that case to
link correctly)

https://codereview.chromium.org/2532793002/diff/180001/chrome/browser/ui/test...
File chrome/browser/ui/test/test_browser_dialog.cc (right):

https://codereview.chromium.org/2532793002/diff/180001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.cc:86: std::string* test_case_name) {
Nit: I don't know whether it's better or worse than this approach, but another
way would be to declare like:

std::string SplitArgument(const std::string& argument, bool harness);

And return one name or the other.  I admit that the use of bool here is not
wonderful, but since this is basically a file-scope helper, so what.

This would be about the same code within the function, but would be slightly
nicer to call.

Up to you.

https://codereview.chromium.org/2532793002/diff/180001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.cc:176: case_list << "\t" << name <<
"\n";
Nit: Unless I overlooked something, you have no need for ostringstream, just do:

    std::string case_list;
    for (const std::string& name : GetCases())
       case_list += '\t' + name + '\n';
    VLOG(0) << ... << case_list;

https://codereview.chromium.org/2532793002/diff/180001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.cc:179: return;  // Nothing provided
(just list options).
Nit: I found this comment more confusing than helpful.

If you think this is really unclear, I would replace this comment with a comment
above the whole conditional saying "if no dialog was specified, print a usage
message and exit" or similar.

https://codereview.chromium.org/2532793002/diff/180001/chrome/browser/ui/test...
File chrome/browser/ui/test/test_browser_dialog.h (right):

https://codereview.chromium.org/2532793002/diff/180001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.h:94: static int Register(const char*
harness, NameProviderFunction name_provider);
Nit: While technically I think they mean the same thing to the compiler,
shouldn't this take a "NameProviderFunction*" for clarity?

https://codereview.chromium.org/2532793002/diff/180001/ui/base/test/material_...
File ui/base/test/material_design_controller_test_api.h (right):

https://codereview.chromium.org/2532793002/diff/180001/ui/base/test/material_...
ui/base/test/material_design_controller_test_api.h:24: void
SetSecondaryUiMaterial(bool value);
Nit: ...ForTesting()?

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2532793002/200001

4 years ago (2016-12-16 00:57:56 UTC) #47

commit-bot: I haz the power

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

4 years ago (2016-12-16 01:34:31 UTC) #48

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_compile_dbg_ng/builds/316142) win_chromium_x64_rel_ng on ...

4 years ago (2016-12-16 01:34:32 UTC) #49

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2532793002/220001

4 years ago (2016-12-16 02:58:38 UTC) #51

tapted

Took a few attempts to get the right incantation for MSVC: I thought "constexpr const" ...

4 years ago (2016-12-16 04:21:54 UTC) #52

Took a few attempts to get the right incantation for MSVC: I thought "constexpr
const" only made sense for pointer types (and not fixed-sized arrays, but it
seems char[] arrays are treated differently around constexpr.

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
File chrome/browser/ui/test/test_browser_dialog.cc (right):

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.cc:120: if
(!command_line.HasSwitch(switches::kExtendMdToSecondaryUi))
On 2016/12/15 08:12:28, Peter Kasting wrote:
> On 2016/12/15 06:08:22, tapted wrote:
> > On 2016/12/15 00:28:55, Peter Kasting wrote:
> > > I'm worried this is going to break in the future.
> > > 
> > > It seems like really you don't care about --secondary-ui-md, you care
about
> > > toolkit=views.  Does --secondary-ui-md result in some of command-line flag
> > being
> > > auto-added which means that directly, which you can test for?  If not,
what
> > are
> > > the consequences of just not having this ifdef and letting things break if
> > > people try to run them the wrong way?
> > 
> > Since MD dialogs on Mac are only implemented using toolkit-views,
> > --secondary-ui-md on Mac means that a dialog will be toolkit-views. (There
are
> > no longer any other `toolkit-views` runtime flags for Mac).
> > 
> > Without this ifdef, a Cocoa dialog will appear, which won't be picked up
> below,
> > and the test will fail: "Dialog not shown, or not a toolkit-views Dialog".
(A
> > `WidgetCloser` that works for native Cocoa dialogs is feasible, but may have
a
> > limited lifespan).
> 
> And to be clear, this is a problem because:
> * The test will run on the bots automatically, which don't pass this flag, so
> just breaking in this case is Not OK

Yes (I considered defining a DISABLED_ test to spawn in the subprocess with
--also_run_disabled_tests, but that seemed like an easy/lazy way out. We're
already lacking some coverage of the UI code of various dialogs, so this will
help us get some more).

> * You're trying to test Harmony UI outside the whole-shebang "Mac
TOOLKIT_VIEWS"
> build, so you can't #ifdef the test to bail when that's not set

Yeah - we'll be launching Harmony/MD dialogs on Mac without any extra #defines.
TOOLKIT_VIEWS is already set on Mac in Release, and it allows dialogs to be
created using toolkit-views (e.g. all the Harmony dialogs).

There's a separate mac_views_browser .gn flag set that switches the entire
browser over to views as well. It works pretty well, but needs a lot more work.
It has a bot (http://go/macviewsbuilder), but it's not part of the CQ or
waterfall (and at this stage is mainly for catching compile regressions).

https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.cc:199: const std::string disable =
"10000000";
On 2016/12/15 08:12:28, Peter Kasting wrote:
> On 2016/12/15 06:08:22, tapted wrote:
> >  There's a pasture of yaks to wade through :/
> > 
> > * base::TimeDelta should have `constextpr` constructors
> > * TestTimeouts's static members can then be base::TimeDelta
> > * base::debug::BeingDebugged() should result in action timeouts being
> > base::TimeDelta::Max()
> > * InitializeTimeout() in test_timeouts.cc needs to beware not to overflow
when
> > multiplying base::TimeDelta values (e.g. for MSAN builds)
> > * test_timeout.cc should be using int64_t constants
> 
> Feel free to file those yaks as bugs...

Filed http://crbug.com/674764 (which you've seen :).

https://codereview.chromium.org/2532793002/diff/180001/base/test/test_timeouts.h
File base/test/test_timeouts.h (right):

https://codereview.chromium.org/2532793002/diff/180001/base/test/test_timeout...
base/test/test_timeouts.h:17: static const char kNoTimeoutSwitchValue[];
On 2016/12/15 08:12:29, Peter Kasting wrote:
> Nit: Can this be declared constexpr?  Maybe by moving the initializer value
> here?  (Probably still needs a definition point in the .cc file in that case
to
> link correctly)

Done. Although (curiously?) Clang was happy with just `constexpr`, but MSVC
required `constexpr const`.

With just `constexpr` MSVC emits:

test_timeouts.h(17):error C2131: expression did not evaluate to a constant
test_timeouts.h(17): note: failure was caused by unevaluable pointer value

This looks kinda like
https://connect.microsoft.com/VisualStudio/feedback/details/1290476

"half-solved in Visual Studio 2015 Update 1"

Except according to http://crrev.com/417157 we are already using VS 2015 Update
3 as of Sep 7, 2016. But the example code there is using `constexpr const`.

Seems MSVC accepts the following:

  static constexpr char kNoTimeoutSwitchValue[] = { '-', '1', '\0' };

(gross). But not this:

  static constexpr char kNoTimeoutSwitchValue[] = "-1";

However, it *does*  accept "constexpr const":

  static constexpr const char kNoTimeoutSwitchValue[] = "-1";

Also, with the first version, it seems to be "impossible" to provide a
*definition* with MSVC. 
c:\src\devel\git\chromium\src\base\test\test_timeouts.cc(71): error C2373:
'kNoTimeoutSwitchValue': redefinition; different type modifiers
c:\src\devel\git\chromium\src\base\test\test_timeouts.h(17): note: see
declaration of 'kNoTimeoutSwitchValue'

or

test_timeouts.cc(71): error C2734: 'TestTimeouts::kNoTimeoutSwitchValue':
'const' object must be initialized if not 'extern'

or

test_timeouts.cc(71): error C2374: 'kNoTimeoutSwitchValue': redefinition;
multiple initialization

And actually MSVC will link without a definition at all. However, Clang will
not.

However, "constexpr const" seems to make all the compilers happy.

https://codereview.chromium.org/2532793002/diff/180001/chrome/browser/ui/test...
File chrome/browser/ui/test/test_browser_dialog.cc (right):

https://codereview.chromium.org/2532793002/diff/180001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.cc:86: std::string* test_case_name) {
On 2016/12/15 08:12:29, Peter Kasting wrote:
> Nit: I don't know whether it's better or worse than this approach, but another
> way would be to declare like:
> 
> std::string SplitArgument(const std::string& argument, bool harness);
> 
> And return one name or the other.  I admit that the use of bool here is not
> wonderful, but since this is basically a file-scope helper, so what.
> 
> This would be about the same code within the function, but would be slightly
> nicer to call.
> 
> Up to you.

Yeah I considered this too - flip-flopped a bit. I guess it felt kinda like
"coincidence" that the two callers only wanted one or the other part. A
bool/enum argument kinda hides the fact that it's a split+discard. But also
encapsulation is good too, so my feelings are mixed. Kept it the same (because
inertia).

https://codereview.chromium.org/2532793002/diff/180001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.cc:176: case_list << "\t" << name <<
"\n";
On 2016/12/15 08:12:29, Peter Kasting wrote:
> Nit: Unless I overlooked something, you have no need for ostringstream, just
do:
> 
>     std::string case_list;
>     for (const std::string& name : GetCases())
>        case_list += '\t' + name + '\n';
>     VLOG(0) << ... << case_list;

Done.

https://codereview.chromium.org/2532793002/diff/180001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.cc:179: return;  // Nothing provided
(just list options).
On 2016/12/15 08:12:29, Peter Kasting wrote:
> Nit: I found this comment more confusing than helpful.
> 
> If you think this is really unclear, I would replace this comment with a
comment
> above the whole conditional saying "if no dialog was specified, print a usage
> message and exit" or similar.

Done (just added "and exits" to the function-level comment)

https://codereview.chromium.org/2532793002/diff/180001/chrome/browser/ui/test...
File chrome/browser/ui/test/test_browser_dialog.h (right):

https://codereview.chromium.org/2532793002/diff/180001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.h:94: static int Register(const char*
harness, NameProviderFunction name_provider);
On 2016/12/15 08:12:29, Peter Kasting wrote:
> Nit: While technically I think they mean the same thing to the compiler,
> shouldn't this take a "NameProviderFunction*" for clarity?

Yup - that makes sense. Done.

https://codereview.chromium.org/2532793002/diff/180001/ui/base/test/material_...
File ui/base/test/material_design_controller_test_api.h (right):

https://codereview.chromium.org/2532793002/diff/180001/ui/base/test/material_...
ui/base/test/material_design_controller_test_api.h:24: void
SetSecondaryUiMaterial(bool value);
On 2016/12/15 08:12:29, Peter Kasting wrote:
> Nit: ...ForTesting()?

I tried this out, but it felt a bit too redundant to say `ui::test::FooTestApi
test_api; test_api.SetFooForTesting(..)`. I think `ForTesting` comes up more
often when a release-code header needs to declare a testing method (so that,
e.g., presubmits can complain if release-code tries to call it).

But when it's a member of a test API in a test-only header (and test namespace),
that requirement doesn't present itself. (even sounds a bit like we're testing
the TestAPI :).

commit-bot: I haz the power

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

4 years ago (2016-12-16 06:03:11 UTC) #55

Peter Kasting

https://codereview.chromium.org/2532793002/diff/180001/base/test/test_timeouts.h File base/test/test_timeouts.h (right): https://codereview.chromium.org/2532793002/diff/180001/base/test/test_timeouts.h#newcode17 base/test/test_timeouts.h:17: static const char kNoTimeoutSwitchValue[]; On 2016/12/16 04:21:53, tapted wrote: ...

4 years ago (2016-12-16 06:30:56 UTC) #57

Paweł Hajdan Jr.

https://codereview.chromium.org/2532793002/diff/220001/chrome/browser/ui/test/test_browser_dialog.cc File chrome/browser/ui/test/test_browser_dialog.cc (right): https://codereview.chromium.org/2532793002/diff/220001/chrome/browser/ui/test/test_browser_dialog.cc#newcode207 chrome/browser/ui/test/test_browser_dialog.cc:207: base::LaunchProcess(command, options); Whoa, this looks quite nontrivial. Do you ...

4 years ago (2016-12-16 20:14:29 UTC) #58

Peter Kasting

https://codereview.chromium.org/2532793002/diff/220001/chrome/browser/ui/test/test_browser_dialog.cc File chrome/browser/ui/test/test_browser_dialog.cc (right): https://codereview.chromium.org/2532793002/diff/220001/chrome/browser/ui/test/test_browser_dialog.cc#newcode207 chrome/browser/ui/test/test_browser_dialog.cc:207: base::LaunchProcess(command, options); On 2016/12/16 20:14:29, Paweł Hajdan Jr. wrote: ...

4 years ago (2016-12-17 03:53:29 UTC) #59

tapted

Description was changed from ========== Add a TEST_BROWSER_DIALOG Macro for testing browser dialogs in a ...

4 years ago (2016-12-18 23:57:17 UTC) #60

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2532793002/240001

4 years ago (2016-12-18 23:57:37 UTC) #62

commit-bot: I haz the power

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

4 years ago (2016-12-19 01:34:57 UTC) #63

tapted

https://codereview.chromium.org/2532793002/diff/220001/chrome/browser/ui/test/test_browser_dialog.cc File chrome/browser/ui/test/test_browser_dialog.cc (right): https://codereview.chromium.org/2532793002/diff/220001/chrome/browser/ui/test/test_browser_dialog.cc#newcode207 chrome/browser/ui/test/test_browser_dialog.cc:207: base::LaunchProcess(command, options); On 2016/12/17 03:53:28, Peter Kasting wrote: > ...

4 years ago (2016-12-19 01:56:11 UTC) #65

Paweł Hajdan Jr.

phajdan.jr@chromium.org changed reviewers: + jam@chromium.org

4 years ago (2016-12-19 18:54:59 UTC) #66

Paweł Hajdan Jr.

+jam Thanks for providing more context. Have all concerns from the design doc been resolved? ...

4 years ago (2016-12-19 18:54:59 UTC) #67

tapted

On 2016/12/19 18:54:59, Paweł Hajdan Jr. wrote: > +jam > > Thanks for providing more ...

4 years ago (2016-12-19 22:21:26 UTC) #68

On 2016/12/19 18:54:59, Paweł Hajdan Jr. wrote:
> +jam
> 
> Thanks for providing more context.
> 
> Have all concerns from the design doc been resolved? I noticed some about
> debuggability, and indeed yet another level of process nesting doesn't help.

The extra process only comes up when invoking dialogs via
BrowserDialogTest.Invoke, and not on trybots. To debug, a developer just needs
to invoke the test via FooDialogTest.InvokeDefault instead. As mentioned at
http://crrev.com/2532793002#msg38 I think removing the subprocess is a good
follow-up (but intrusive, etc.).

Debugging isn't really a goal - this isn't a replacement for regular tests. The
goal is to register a comprehensive set of Chrome's dialogs in one place and
provide a means of easily showing them (principally for UI review /
screenshots).

> It may make sense to have a broader discussion about this, maybe on
> chromium-dev. If a CL breaks one of these tests, it may introduce a burden for
> all Chrome developers when debugging is hard.

There's a comment here about this:
https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...

Quoting,

On 2016/12/16 04:21:53, tapted wrote:
> On 2016/12/15 08:12:28, Peter Kasting wrote:
> > And to be clear, this is a problem because:
> > * The test will run on the bots automatically, which don't pass this flag,
so
> > just breaking in this case is Not OK
> 
> Yes (I considered defining a DISABLED_ test to spawn in the subprocess with
> --also_run_disabled_tests, but that seemed like an easy/lazy way out. We're
> already lacking some coverage of the UI code of various dialogs, so this will
> help us get some more).

Another option could be to add an extra argument to the TEST_BROWSER_DIALOG
macro. E.g. TEST_BROWSER_DIALOG(FooDialogTest, InvokeDefault). With a comment
saying "The second argument should be InvokeDefault or DISABLED_InvokeDefault" -
this could help sheriffs deal with problems in a more familiar way versus just
commenting out the test. Will that improve things?

Otherwise, debugging a failure from an automated test run should be the same as
any other test - the subprocess only gets involved when passing extra arguments
on the command line.

jam

I glanced through the cl and looked at the design doc. My main question is ...

4 years ago (2016-12-20 00:17:57 UTC) #69

Peter Kasting

https://codereview.chromium.org/2532793002/diff/240001/base/test/test_timeouts.h File base/test/test_timeouts.h (right): https://codereview.chromium.org/2532793002/diff/240001/base/test/test_timeouts.h#newcode17 base/test/test_timeouts.h:17: static constexpr const char kNoTimeoutSwitchValue[] = "-1"; On 2016/12/20 ...

4 years ago (2016-12-20 00:24:23 UTC) #70

tapted

On 2016/12/20 00:17:57, jam wrote: > I glanced through the cl and looked at the ...

4 years ago (2016-12-20 00:25:17 UTC) #71

jam

On 2016/12/20 00:25:17, tapted wrote: > On 2016/12/20 00:17:57, jam wrote: > > I glanced ...

4 years ago (2016-12-20 01:12:33 UTC) #72

On 2016/12/20 00:25:17, tapted wrote:
> On 2016/12/20 00:17:57, jam wrote:
> > I glanced through the cl and looked at the design doc. My main question is
do
> we
> > need this macro? i.e. could we accomplish the same thing by having a test
> helper
> > function that takes a callback to show the dialog? That might be less magic
> > compared to macros.
> 
> There are a couple of reasons for the macro.
> 
> One is similar to why we use a macro for the regular TEST TEST_F, etc.
methods.
> There is some extra "magic" in the way the macro is able to "register" the
name
> of the dialog in a central location (this registration is so that we can be
> systematic about checking the full set of Chrome dialogs for changes). See
> review comments at
>
https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
> for more about this.

Seems like there could be other ways of doing this, i.e. using a standard string
in dialog test names.

> 
> Another reason is that the macro is invoking another macro (i.e.
> IN_PROC_BROWSER_TEST_F) so that we get a "hook" in the test suite for reaching
> back into this particular test harness from a central location (i.e. when
> BrowserDialogTest.Invoke invokes FooDialogTest.InvokeDefault via
gtest_filter).

Couldn't that be done with a test base class? i.e. many features have their own
helper test harness class that derives from InProcessBrowserTest, such as
ExtensionBrowserTest.

> 
>
https://codereview.chromium.org/2532793002/diff/240001/base/test/test_timeouts.h
> File base/test/test_timeouts.h (right):
> 
>
https://codereview.chromium.org/2532793002/diff/240001/base/test/test_timeout...
> base/test/test_timeouts.h:17: static constexpr const char
> kNoTimeoutSwitchValue[] = "-1";
> On 2016/12/20 00:17:57, jam wrote:
> > Why do we need this, given that there are already switches to set the
timeout
> > high?
> 
> This is from the review comment at
>
https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
> 
> See also http://crbug.com/674764 filed from that comment

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2532793002/260001

4 years ago (2016-12-20 03:41:24 UTC) #74

tapted

Description was changed from ========== Add a TEST_BROWSER_DIALOG Macro for testing browser dialogs in a ...

4 years ago (2016-12-20 03:48:36 UTC) #75

Description was changed from

==========
Add a TEST_BROWSER_DIALOG Macro for testing browser dialogs in a consistent way.

Allows a test harness to opt in to a framework for testing browser
dialogs. Merely override a method, `void ShowDialog(int index)`, and add

TEST_BROWSER_DIALOG(FooDialogTest);

to foo_dialog_browsertest.cc.

This will
- add a FooDialogTest.InvokeDefault test case (invokes ShowDialog(0))
- register the test harness with the dialog testing framework.

Running
  browser_tests --gtest_filter=BrowserDialogTest.Invoke

will list all dialogs registered with the testing framework. And with
the additional arguments,
  --dialog=FooDialogTest.Invoke --interactive

BrowserDialogTest will invoke the dialog "interactively". This allows
for manual testing and screenshots for UI review.

A test harness can register multiple "styles" of its dialog. See in-file
documentation for more info.

BUG=654151
==========

to

==========
Add a TestBrowserDialog helper class for testing browser dialogs in a consistent
way.

Allows a test harness to opt in to a framework for testing browser
dialogs. Merely override a method, `void ShowDialog(const std::string&
name)`, and add any number of

IN_PROC_BROWSER_TEST_F(FooDialogTest, InvokeDialog_name) {
  RunDialog();
}

to foo_dialog_browsertest.cc.

This will
- Invoke ShowDialog("name") in the test case and perform some basic
checks.
- register the test name with the dialog testing framework.

Running
  browser_tests --gtest_filter=BrowserDialogTest.Invoke

will list all dialogs registered with the testing framework. And with
the additional arguments,
  --dialog=FooDialogTest.InvokeDialog_name --interactive

BrowserDialogTest will invoke the dialog "interactively". This allows
for manual testing and screenshots for UI review.

A test harness can register multiple "styles" of its dialog by providing
multiple tests invoking RunDialog() using different suffixes after
"InvokeDialog_". See in-file documentation for more info.

BUG=654151
==========

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2532793002/280001

4 years ago (2016-12-20 03:53:32 UTC) #77

tapted

PTAL - ditched the macro. And the whole NameProvider() thing. The framework now hunts for ...

4 years ago (2016-12-20 04:06:35 UTC) #79

PTAL - ditched the macro. And the whole NameProvider() thing. The framework now
hunts for tests defined like

IN_PROC_BROWSER_TEST_F(CardUnmaskPromptViewBrowserTest, InvokeDialog_expired)

(the body of the test is always just `RunDialog()`).


On 2016/12/20 01:12:33, jam wrote:
> On 2016/12/20 00:25:17, tapted wrote:
> > On 2016/12/20 00:17:57, jam wrote:
> > > I glanced through the cl and looked at the design doc. My main question is
> do
> > we
> > > need this macro? i.e. could we accomplish the same thing by having a test
> > helper
> > > function that takes a callback to show the dialog? That might be less
magic
> > > compared to macros.
> > 
> > There are a couple of reasons for the macro.
> > 
> > One is similar to why we use a macro for the regular TEST TEST_F, etc.
> methods.
> > There is some extra "magic" in the way the macro is able to "register" the
> name
> > of the dialog in a central location (this registration is so that we can be
> > systematic about checking the full set of Chrome dialogs for changes). See
> > review comments at
> >
>
https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
> > for more about this.
> 
> Seems like there could be other ways of doing this, i.e. using a standard
string
> in dialog test names.

Done. This changes a lot, but I don't think it's more limiting that the previous
approach.

> > Another reason is that the macro is invoking another macro (i.e.
> > IN_PROC_BROWSER_TEST_F) so that we get a "hook" in the test suite for
reaching
> > back into this particular test harness from a central location (i.e. when
> > BrowserDialogTest.Invoke invokes FooDialogTest.InvokeDefault via
> gtest_filter).
> 
> Couldn't that be done with a test base class? i.e. many features have their
own
> helper test harness class that derives from InProcessBrowserTest, such as
> ExtensionBrowserTest.

I think I've resolved this just by requiring client code to provide their own
IN_PROC_BROWSER_TEST_F as part of the registration process (i.e. no longer need
a macro to invoke IN_PROC_BROWSER_TEST_F since client code must invoke it
directly).

commit-bot: I haz the power

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

4 years ago (2016-12-20 06:26:16 UTC) #80

Paweł Hajdan Jr.

phajdan.jr@chromium.org changed reviewers: + dpranke@chromium.org

4 years ago (2016-12-20 16:27:14 UTC) #82

Paweł Hajdan Jr.

+dpranke for further infra considerations I think the CL is moving in the right direction. ...

4 years ago (2016-12-20 16:27:15 UTC) #83

jam

Thanks, this looks more straightforward now with the removal of the macro. lgtm My one ...

4 years ago (2016-12-20 17:10:57 UTC) #84

Thanks, this looks more straightforward now with the removal of the macro. lgtm

My one question is: why is the "name" parameter needed to be passed to
ShowDialog. Is that if FooDialogTest is used as a parent class for multiple
dialog tests? and there would be FooDialogTest.InvokeDialogDialog1 and
FooDialogTest.InvokeDialogDialog2?


On 2016/12/20 04:06:35, tapted wrote:
> PTAL - ditched the macro. And the whole NameProvider() thing. The framework
now
> hunts for tests defined like
> 
> IN_PROC_BROWSER_TEST_F(CardUnmaskPromptViewBrowserTest, InvokeDialog_expired)
> 
> (the body of the test is always just `RunDialog()`).
> 
> 
> On 2016/12/20 01:12:33, jam wrote:
> > On 2016/12/20 00:25:17, tapted wrote:
> > > On 2016/12/20 00:17:57, jam wrote:
> > > > I glanced through the cl and looked at the design doc. My main question
is
> > do
> > > we
> > > > need this macro? i.e. could we accomplish the same thing by having a
test
> > > helper
> > > > function that takes a callback to show the dialog? That might be less
> magic
> > > > compared to macros.
> > > 
> > > There are a couple of reasons for the macro.
> > > 
> > > One is similar to why we use a macro for the regular TEST TEST_F, etc.
> > methods.
> > > There is some extra "magic" in the way the macro is able to "register" the
> > name
> > > of the dialog in a central location (this registration is so that we can
be
> > > systematic about checking the full set of Chrome dialogs for changes). See
> > > review comments at
> > >
> >
>
https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...
> > > for more about this.
> > 
> > Seems like there could be other ways of doing this, i.e. using a standard
> string
> > in dialog test names.
> 
> Done. This changes a lot, but I don't think it's more limiting that the
previous
> approach.
> 
> > > Another reason is that the macro is invoking another macro (i.e.
> > > IN_PROC_BROWSER_TEST_F) so that we get a "hook" in the test suite for
> reaching
> > > back into this particular test harness from a central location (i.e. when
> > > BrowserDialogTest.Invoke invokes FooDialogTest.InvokeDefault via
> > gtest_filter).
> > 
> > Couldn't that be done with a test base class? i.e. many features have their
> own
> > helper test harness class that derives from InProcessBrowserTest, such as
> > ExtensionBrowserTest.
> 
> I think I've resolved this just by requiring client code to provide their own
> IN_PROC_BROWSER_TEST_F as part of the registration process (i.e. no longer
need
> a macro to invoke IN_PROC_BROWSER_TEST_F since client code must invoke it
> directly).

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2532793002/300001

4 years ago (2016-12-20 23:16:30 UTC) #86

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2532793002/320001

4 years ago (2016-12-20 23:28:31 UTC) #88

tapted

On 2016/12/20 17:10:57, jam wrote: > My one question is: why is the "name" parameter ...

4 years ago (2016-12-20 23:34:47 UTC) #90

On 2016/12/20 17:10:57, jam wrote:
> My one question is: why is the "name" parameter needed to be passed to
> ShowDialog. Is that if FooDialogTest is used as a parent class for multiple
> dialog tests? and there would be FooDialogTest.InvokeDialogDialog1 and
> FooDialogTest.InvokeDialogDialog2?

There's an example in card_unmask_prompt_view_browsertest.cc in this CL for how
|name| is used (and others in the `Concrete Example` CLs in the design doc).
In this example, different UI is shown for expired and valid credit cards, and
the UX is quite different too. Typically, not much changes in the SetUp/TearDown
so the |name| argument allows `RunDialog()` to feed through a parameter to
ShowDialog() in a consistent and concise way.

(another from `Concrete Examples` is permissions prompts: the passing different
values for the content::PermisisonType enum to PermissionPromptImpl allows the
different icons/layouts to be checked easily).

https://codereview.chromium.org/2532793002/diff/280001/chrome/browser/ui/test...
File chrome/browser/ui/test/browser_dialog_browsertest.cc (right):

https://codereview.chromium.org/2532793002/diff/280001/chrome/browser/ui/test...
chrome/browser/ui/test/browser_dialog_browsertest.cc:73:
base::LaunchProcess(command, options);
On 2016/12/20 16:27:15, Paweł Hajdan Jr. wrote:
> Are we launching all subprocesses under one test?
> 
> That one test (BrowserDialogTest.Invoke) will still be subject to test
launcher
> timeouts. As more test cases are added, I don't think it'll be sustainable.
> 
> Have you considered parametrizing the test? See
>
https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid...
> for a possible way to do that.

I think this falls under the 'Future Work' section of the design doc (link:
https://goo.gl/ynxx1l for newcomers).

In this CL (and without the --interactive argument), BrowserDialogTest.Invoke
merely lists the available dialogs and exits. To pursue the "automated test
suite leveraging and iterating over ShowDialog()" future work item - you are
quite right that we will need something more intelligent here. Some kind of
sharding is likely.

A possible approach would be for TestBrowserDialog::RunDialog() to iterate over
the `DialogAction` enum for a single dialog, rather than have
BrowserDialogTest.Invoke iterate over all the dialog styles. See patchset 8
(link:
https://codereview.chromium.org/2532793002/diff/160001/chrome/browser/ui/test...)
for what I had in mind for the other enum values for `DialogAction`. I think
this iteration will be able to run within the default timeouts.

The subprocess can then remain just for the `--interactive, --dialog=Foo` mode
(e.g. taking screenshots). Bots should never launch the subprocess.

https://codereview.chromium.org/2532793002/diff/280001/chrome/browser/ui/test...
File chrome/browser/ui/test/test_browser_dialog.cc (right):

https://codereview.chromium.org/2532793002/diff/280001/chrome/browser/ui/test...
chrome/browser/ui/test/test_browser_dialog.cc:62: std::string NameFromTestCase()
{
On 2016/12/20 16:27:15, Paweł Hajdan Jr. wrote:
> Is this the same as TestNameWithoutDisabledPrefix from base/test/gtest_util.h?

Thanks for the pointer - there's some interesting stuff there. This is
effectively `gsed -r 's/^(DISABLED_)?[^_]*_//'`, so a bit different. Added an
example comment to the CL:

// E.g. for InvokeDialog_name (or DISABLED_InvokeDialog_name) returns "name".

But we can use TestNameWithoutDisabledPrefix for a bit of cleanup [Done.].

commit-bot: I haz the power

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

4 years ago (2016-12-21 00:57:46 UTC) #91

tapted

The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org, jam@chromium.org Link to ...

4 years ago (2016-12-21 22:16:10 UTC) #95

commit-bot: I haz the power

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

4 years ago (2016-12-21 22:16:41 UTC) #96

commit-bot: I haz the power

CQ is committing da patch. Bot data: {"patchset_id": 320001, "attempt_start_ts": 1482358569986580, "parent_rev": "4e6b02602cbd809c4a2c5003b9394e6c078ff812", "commit_rev": "d81bae3e37c5afc253d1f67f26f1a657a6089fff"}

4 years ago (2016-12-21 22:23:39 UTC) #97

commit-bot: I haz the power

Description was changed from ========== Add a TestBrowserDialog helper class for testing browser dialogs in ...

4 years ago (2016-12-21 22:24:09 UTC) #98

Message was sent while issue was closed.

Description was changed from

==========
Add a TestBrowserDialog helper class for testing browser dialogs in a consistent
way.

Allows a test harness to opt in to a framework for testing browser
dialogs. Merely override a method, `void ShowDialog(const std::string&
name)`, and add any number of

IN_PROC_BROWSER_TEST_F(FooDialogTest, InvokeDialog_name) {
  RunDialog();
}

to foo_dialog_browsertest.cc.

This will
- Invoke ShowDialog("name") in the test case and perform some basic
checks.
- register the test name with the dialog testing framework.

Running
  browser_tests --gtest_filter=BrowserDialogTest.Invoke

will list all dialogs registered with the testing framework. And with
the additional arguments,
  --dialog=FooDialogTest.InvokeDialog_name --interactive

BrowserDialogTest will invoke the dialog "interactively". This allows
for manual testing and screenshots for UI review.

A test harness can register multiple "styles" of its dialog by providing
multiple tests invoking RunDialog() using different suffixes after
"InvokeDialog_". See in-file documentation for more info.

BUG=654151
==========

to

==========
Add a TestBrowserDialog helper class for testing browser dialogs in a consistent
way.

Allows a test harness to opt in to a framework for testing browser
dialogs. Merely override a method, `void ShowDialog(const std::string&
name)`, and add any number of

IN_PROC_BROWSER_TEST_F(FooDialogTest, InvokeDialog_name) {
  RunDialog();
}

to foo_dialog_browsertest.cc.

This will
- Invoke ShowDialog("name") in the test case and perform some basic
checks.
- register the test name with the dialog testing framework.

Running
  browser_tests --gtest_filter=BrowserDialogTest.Invoke

will list all dialogs registered with the testing framework. And with
the additional arguments,
  --dialog=FooDialogTest.InvokeDialog_name --interactive

BrowserDialogTest will invoke the dialog "interactively". This allows
for manual testing and screenshots for UI review.

A test harness can register multiple "styles" of its dialog by providing
multiple tests invoking RunDialog() using different suffixes after
"InvokeDialog_". See in-file documentation for more info.

BUG=654151

Review-Url: https://codereview.chromium.org/2532793002
==========

commit-bot: I haz the power

Description was changed from ========== Add a TestBrowserDialog helper class for testing browser dialogs in ...

4 years ago (2016-12-21 22:26:01 UTC) #100

Message was sent while issue was closed.

Description was changed from

==========
Add a TestBrowserDialog helper class for testing browser dialogs in a consistent
way.

Allows a test harness to opt in to a framework for testing browser
dialogs. Merely override a method, `void ShowDialog(const std::string&
name)`, and add any number of

IN_PROC_BROWSER_TEST_F(FooDialogTest, InvokeDialog_name) {
  RunDialog();
}

to foo_dialog_browsertest.cc.

This will
- Invoke ShowDialog("name") in the test case and perform some basic
checks.
- register the test name with the dialog testing framework.

Running
  browser_tests --gtest_filter=BrowserDialogTest.Invoke

will list all dialogs registered with the testing framework. And with
the additional arguments,
  --dialog=FooDialogTest.InvokeDialog_name --interactive

BrowserDialogTest will invoke the dialog "interactively". This allows
for manual testing and screenshots for UI review.

A test harness can register multiple "styles" of its dialog by providing
multiple tests invoking RunDialog() using different suffixes after
"InvokeDialog_". See in-file documentation for more info.

BUG=654151

Review-Url: https://codereview.chromium.org/2532793002
==========

to

==========
Add a TestBrowserDialog helper class for testing browser dialogs in a consistent
way.

Allows a test harness to opt in to a framework for testing browser
dialogs. Merely override a method, `void ShowDialog(const std::string&
name)`, and add any number of

IN_PROC_BROWSER_TEST_F(FooDialogTest, InvokeDialog_name) {
  RunDialog();
}

to foo_dialog_browsertest.cc.

This will
- Invoke ShowDialog("name") in the test case and perform some basic
checks.
- register the test name with the dialog testing framework.

Running
  browser_tests --gtest_filter=BrowserDialogTest.Invoke

will list all dialogs registered with the testing framework. And with
the additional arguments,
  --dialog=FooDialogTest.InvokeDialog_name --interactive

BrowserDialogTest will invoke the dialog "interactively". This allows
for manual testing and screenshots for UI review.

A test harness can register multiple "styles" of its dialog by providing
multiple tests invoking RunDialog() using different suffixes after
"InvokeDialog_". See in-file documentation for more info.

BUG=654151

Committed: https://crrev.com/658e7bd30bc3164953da7796a4198b58b5514655
Cr-Commit-Position: refs/heads/master@{#440240}
==========

commit-bot: I haz the power

4 years ago (2016-12-21 22:26:03 UTC) #101

Message was sent while issue was closed.

Patchset 14 (id:??) landed as
https://crrev.com/658e7bd30bc3164953da7796a4198b58b5514655
Cr-Commit-Position: refs/heads/master@{#440240}

Issue 2532793002: Add a TestBrowserDialog helper class for testing browser dialogs in a consistent way. (Closed)

Description

Patch Set 1 #

Patch Set 2 : Now with options #

Patch Set 3 : More documentation #

Patch Set 4 : Skip InvokeDefault on Mac #

Patch Set 5 : Rebase on native parent thingo #

Patch Set 6 : Better doc. Mac should pass now #

Patch Set 7 : Remove no longer used switches #

Patch Set 8 : Add kEnablePixelOutputInTests #

Patch Set 9 : Respond to comments #

Patch Set 10 : Respond to comments #

Patch Set 11 : constexpr const it is #

Patch Set 12 : Split out browsertest into browsertest.cc #

Patch Set 13 : Overhaul #

Patch Set 14 : Comment example. Use TestNameWithoutDisabledPrefix #

Messages