New issue
Advanced search Search tips

Issue 880688 link

Starred by 1 user

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

WebUI test - Add ability to pause before tests to be able to attach debugger

Project Member Reported by lucmult@chromium.org, Sep 5

Issue description

When working in unittest it isn't possible to inspect and debug the test code in the Developers Console.

For Files app integration tests we have a flag that waits for a command to be issued in the JS console before starting the tests.

It would beneficial if WebUI tests infrastructure provided similar feature.
 
I prototyped a similar approach for WebUI tests:

crrev.com/c/1205919

Should I polish this and send for review?
Cc: steve...@chromium.org michae...@chromium.org dpa...@chromium.org
Ooh, that's useful. I'm supportive of this.

Can this be used to set breakpoints inside the test code?

cc-ing more WebUI folks.
At times I've ended up modifying some local JS file to do something similar, so something like that would be great.
Have you had a chance to see https://docs.google.com/document/d/1Z18WTNv28z5FW3smNEm_GtsfVD2IL-CmmAikwjw3ryo/edit#heading=h.9tcrnu5vvfwi ?

Specifically adding the following flags allows debugging a WebUI test

--ui-test-action-max-timeout=1000000 --test-launcher-timeout=1000000 --enable-pixel-output-in-tests

If you couple this with a debugger; statement, you could basically achieve the same I think. Alternatively, when using Mocha (which most of the newer WebUI tests do), it is trivial to a |done| argument to a test function and cause it to never exit the test.
Demetrios, I'm not sure if the doc you point to which mentions https://cs.chromium.org/chromium/src/chrome/test/base/javascript_browser_test.h relates at all to https://cs.chromium.org/chromium/src/ui/webui/resources/js/webui_resource_test.js which is the file that Luciano is suggesting to change.

I think the webui_resource_test.js code is not actually used in any WebUI tests. 
 It is mostly used in file_maanger and a few others.  https://cs.chromium.org/chromium/src/content/public/test/browser_test_utils.cc?l=1587&rcl=99db8d62a62610f9aed8225c47dc051b5d61b1a1

Adding a debugger; statement only helps if you already have devtools attached.  This proposed change is to allow a flag which pauses execution and gives you time to open devtools in the window that pops up for the test, or attach one remotely by using the --remote-debugging-port flag.

For me personally, a change like this is useful, but it isn't such a big deal since I'm now familiar enough with the code now that I can make these local hacks myself.  But something like this if it was well documented would have been very helpful at the start of the year when I started working on this code.
Hi dpapad@,

I use the flags you mentioned, however as joelhockey@ mentioned it doesn't allow us to attach the remote inspector/debugger, the CL I linked, it basically keeps waiting until a command "go()" is issued from JS console.

So it actually has to be used with --remote-debugging-port, to be able to attach the inspector/debugger.

To debug, we can use either "debugger;" or break points set manually via Developer Tools, after attaching to the debugger/inspector.

Basically this is a similar strategy already in use in FileManager integration tests for a while:
https://cs.chromium.org/chromium/src/ui/file_manager/integration_tests/remote_call.js?l=34-62&rcl=0931dc803165037b9abae0d567ec0a17ef2cf12b
Owner: lucmult@chromium.org
Status: Started (was: Untriaged)
From crrev.com/c/1205919 discussion.

I managed to extend the initial approach to WebUI test_api.js tests too:

See attached screenshot.

I've run the following test/command line:

./out/Default/browser_tests --gtest_filter="*CrExtensionsItemsTest.Warnings*"  --enable-pixel-output-in-tests --use-gpu-in-tests --ui-test-action-timeout=1000000 --enable-file-manager-step-by-step-tests --webui-test-wait-debugger --remote-debugging-port=9222


Selection_051.png
618 KB View Download
Project Member

Comment 8 by bugdroid1@chromium.org, Sep 19

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/034fd9df0917df3a46732d281aa794ca23e766bb

commit 034fd9df0917df3a46732d281aa794ca23e766bb
Author: Luciano Pacheco <lucmult@chromium.org>
Date: Wed Sep 19 01:18:40 2018

Pause webui test to wait for debugger

Add a new flag "wait-for-debugger-webui" which makes WebUI tests stop
for waiting to a debugger. This flag only works if
"remote-debugging-port" flag is also set.

The WebUI test will wait until user issues "go()" command from the JS
console to proceed with tests.

This waiting scheme works by having a global variable |waitUser|, which
is either false or undefined by default, so it doesn't wait by default.
When the flags are provided, the BrowserTest C++ class sets the JS
global variable |waitUser| to true before starting the tests. In JS
when |waitUser| is true, the function retries by re-scheduling itself
for 1 second in the future. IMPORTANT: There is no timeout for this
retry, only the default timeout of the tests infrastructure applies
here.

--gtest_filter='FileManagerJsTest.MetadataCacheSet' and
--gtest_filter='*CrExtensionsItemsTest.Warnings*'

Bug: 880688
Test: Checked manually that the following tests wait for debugger
Change-Id: Ia198d2c5ed8c6d464f0ab8fb1c7201da9a7c5169
Reviewed-on: https://chromium-review.googlesource.com/1205919
Commit-Queue: Luciano Pacheco <lucmult@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Reviewed-by: Noel Gordon <noel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#592294}
[modify] https://crrev.com/034fd9df0917df3a46732d281aa794ca23e766bb/chrome/test/base/web_ui_browser_test.cc
[modify] https://crrev.com/034fd9df0917df3a46732d281aa794ca23e766bb/chrome/test/data/webui/test_api.js
[modify] https://crrev.com/034fd9df0917df3a46732d281aa794ca23e766bb/content/public/test/browser_test_utils.cc
[modify] https://crrev.com/034fd9df0917df3a46732d281aa794ca23e766bb/content/public/test/test_launcher.cc
[modify] https://crrev.com/034fd9df0917df3a46732d281aa794ca23e766bb/content/public/test/test_launcher.h
[modify] https://crrev.com/034fd9df0917df3a46732d281aa794ca23e766bb/ui/webui/resources/js/webui_resource_test.js

Project Member

Comment 9 by bugdroid1@chromium.org, Sep 20

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/faf80530e26b3f3d259703dd0cd4c516945338c9

commit faf80530e26b3f3d259703dd0cd4c516945338c9
Author: Luciano Pacheco <lucmult@chromium.org>
Date: Thu Sep 20 01:16:31 2018

Fix Markdown mark-up and add WebUI info

Rendering this table in Gitiles requires a table header.

I've checked rendering using "gitiles" link in:
https://chromium-review.googlesource.com/c/chromium/src/+/1223347/9/docs/test_descriptions.md
https://chromium.googlesource.com/chromium/src/+/refs/changes/47/1223347/5/docs/test_descriptions.md

Add more information for WebUI tests.

Bug: 880688
Change-Id: If19bada962b67899676ebd2e4d8fa19b9fcec381
Reviewed-on: https://chromium-review.googlesource.com/1223347
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Commit-Queue: Luciano Pacheco <lucmult@chromium.org>
Cr-Commit-Position: refs/heads/master@{#592647}
[modify] https://crrev.com/faf80530e26b3f3d259703dd0cd4c516945338c9/docs/test_descriptions.md

Project Member

Comment 10 by bugdroid1@chromium.org, Dec 17

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4d3266c172c17459e94864b304f447625acd4c45

commit 4d3266c172c17459e94864b304f447625acd4c45
Author: Christopher Lam <calamity@chromium.org>
Date: Mon Dec 17 01:49:44 2018

[WebUI] Remove port requirement for --wait-for-debugger-webui

WebUI tests don't need to set a remote port to run the debugger. This
flag should just always wait, regardless of port.

Bug: 880688

Change-Id: I433b152f1877619190173b058a974405f66d534a
Reviewed-on: https://chromium-review.googlesource.com/c/1371306
Reviewed-by: Noel Gordon <noel@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: calamity <calamity@chromium.org>
Cr-Commit-Position: refs/heads/master@{#617034}
[modify] https://crrev.com/4d3266c172c17459e94864b304f447625acd4c45/chrome/test/base/web_ui_browser_test.cc
[modify] https://crrev.com/4d3266c172c17459e94864b304f447625acd4c45/content/public/test/browser_test_utils.cc

Sign in to add a comment