Allow javascript unit tests using webui test_api framework.

Allow javascript unit tests using webui test_api framework.

I moved javascript2webui.js over to chrome/test/base/js2gtest.js and shared it
across webui and unit tests with an extra parameter for test type, which governs
the few differences, such as what to include/subclass from and the flavor of
gtest (TEST_F v. IN_PROC_BROWSER_TEST_F).

v8_unit_test implemented 2 main methods to make it work with the generated C++
- AddLibrary - loads the file in the test context
- RunJavascriptF - launches the runTest with the testFixture and testName,
coordinating with Error and ChromeSend to report results.

Helper functions:
- Error - watches for console.error, noting failure if seen.
- ChromeSend - pulls apart the result from the test_api's call to
chrome.send('testResult', [ok, msg])

R=arv@chromium.org
BUG=87820
TEST=unit_tests --gtest_filter=FrameworkUnitTest.*


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

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

[Sheridan Rawlins, 2011-10-28 21:50:18]

All righty - I think this is ready for review.

I moved javascript2webui.js over to chrome/test/base/js2gtest.js and shared it
across webui and unit tests with an extra parameter for test type, which governs
the few differences, such as what to include/subclass from and the flavor of
gtest (TEST_F v. IN_PROC_BROWSER_TEST_F).

v8_unit_test implemented 2 main methods to make it work with the generated C++
- AddLibrary - loads the file in the test context
- RunJavascriptF - launches the runTest with the testFixture and testName,
coordinating with Error and ChromeSend to report results.

Helper functions:
- Error - watches for console.error, noting failure if seen.
- ChromeSend - pulls apart the result from the test_api's call to
chrome.send('testResult', [ok, msg])

[Sheridan Rawlins, 2011-10-28 21:56:18]

Adjusted the title to not have WIP and check one more boolean for the test pass.

[arv (Not doing code reviews), 2011-10-28 23:16:55]

I would feel more comfortable if someone else could review the C++ part of this.

http://codereview.chromium.org/8418015/diff/1007/chrome/test/base/js2gtest.js
File chrome/test/base/js2gtest.js (right):

http://codereview.chromium.org/8418015/diff/1007/chrome/test/base/js2gtest.js...
chrome/test/base/js2gtest.js:47: var isAsyncParam = (testType === 'unit') ? '' :
useless parens are discouraged in js

[Sheridan Rawlins, 2011-10-28 23:50:07]

James, could you review the C++ changes?

http://codereview.chromium.org/8418015/diff/1007/chrome/test/base/js2gtest.js
File chrome/test/base/js2gtest.js (right):

http://codereview.chromium.org/8418015/diff/1007/chrome/test/base/js2gtest.js...
chrome/test/base/js2gtest.js:47: var isAsyncParam = (testType === 'unit') ? '' :
On 2011/10/28 23:16:56, arv wrote:
> useless parens are discouraged in js

Done.

[James Hawkins, 2011-10-29 22:15:48]

http://codereview.chromium.org/8418015/diff/16/chrome/test/base/v8_unit_test.cc
File chrome/test/base/v8_unit_test.cc (right):

http://codereview.chromium.org/8418015/diff/16/chrome/test/base/v8_unit_test....
chrome/test/base/v8_unit_test.cc:21: namespace {
Inconsistent: No blank line between namespace and function while blank line
between end of function and end of namespace.  Add blank line here.

http://codereview.chromium.org/8418015/diff/16/chrome/test/base/v8_unit_test....
chrome/test/base/v8_unit_test.cc:22: std::string Args2String(const
v8::Arguments& args) {
Document the function and params.

http://codereview.chromium.org/8418015/diff/16/chrome/test/base/v8_unit_test....
chrome/test/base/v8_unit_test.cc:27: if (first) {
Optional but recommended: no braces on single-line blocks.

http://codereview.chromium.org/8418015/diff/16/chrome/test/base/v8_unit_test....
chrome/test/base/v8_unit_test.cc:55: std::vector<FilePath>::iterator
user_libraries_iterator;
Declare the iterator in the context of the for loop.

http://codereview.chromium.org/8418015/diff/16/chrome/test/base/v8_unit_test.h
File chrome/test/base/v8_unit_test.h (right):

http://codereview.chromium.org/8418015/diff/16/chrome/test/base/v8_unit_test....
chrome/test/base/v8_unit_test.h:31: void AddLibrary(const FilePath&
library_path);
Document these methods and parameters.

http://codereview.chromium.org/8418015/diff/16/chrome/test/base/v8_unit_test....
chrome/test/base/v8_unit_test.h:68: // Executes all added javascript libraries.
Returns true if passing so far.
What does "if passing so far" mean?

http://codereview.chromium.org/8418015/diff/16/chrome/test/base/v8_unit_test....
chrome/test/base/v8_unit_test.h:71: // Initialize paths and libraries.
Initializes

http://codereview.chromium.org/8418015/diff/16/chrome/test/base/v8_unit_test....
chrome/test/base/v8_unit_test.h:80: // Location of test data (currently
test/data/webui).
Don't add implementation details to the interface (even if in private section). 
It's not unlikely to change.  Here and below.

http://codereview.chromium.org/8418015/diff/16/chrome/test/base/v8_unit_test....
chrome/test/base/v8_unit_test.h:90: static bool had_errors_;
Are statics particularly necessary?

[Sheridan Rawlins, 2011-10-30 17:04:20]

http://codereview.chromium.org/8418015/diff/16/chrome/test/base/v8_unit_test.cc
File chrome/test/base/v8_unit_test.cc (right):

http://codereview.chromium.org/8418015/diff/16/chrome/test/base/v8_unit_test....
chrome/test/base/v8_unit_test.cc:21: namespace {
On 2011/10/29 22:15:48, James Hawkins wrote:
> Inconsistent: No blank line between namespace and function while blank line
> between end of function and end of namespace.  Add blank line here.

Done.

http://codereview.chromium.org/8418015/diff/16/chrome/test/base/v8_unit_test....
chrome/test/base/v8_unit_test.cc:22: std::string Args2String(const
v8::Arguments& args) {
On 2011/10/29 22:15:48, James Hawkins wrote:
> Document the function and params.

Done.

http://codereview.chromium.org/8418015/diff/16/chrome/test/base/v8_unit_test....
chrome/test/base/v8_unit_test.cc:27: if (first) {
On 2011/10/29 22:15:48, James Hawkins wrote:
> Optional but recommended: no braces on single-line blocks.

Done.

http://codereview.chromium.org/8418015/diff/16/chrome/test/base/v8_unit_test....
chrome/test/base/v8_unit_test.cc:55: std::vector<FilePath>::iterator
user_libraries_iterator;
On 2011/10/29 22:15:48, James Hawkins wrote:
> Declare the iterator in the context of the for loop.

Done.

http://codereview.chromium.org/8418015/diff/16/chrome/test/base/v8_unit_test....
chrome/test/base/v8_unit_test.cc:242: v8::Handle<v8::Value>
V8UnitTest::ChromeSend(const v8::Arguments& args) {
Need a handle_scope.  Done.

http://codereview.chromium.org/8418015/diff/16/chrome/test/base/v8_unit_test.h
File chrome/test/base/v8_unit_test.h (right):

http://codereview.chromium.org/8418015/diff/16/chrome/test/base/v8_unit_test....
chrome/test/base/v8_unit_test.h:31: void AddLibrary(const FilePath&
library_path);
On 2011/10/29 22:15:48, James Hawkins wrote:
> Document these methods and parameters.

Done.

http://codereview.chromium.org/8418015/diff/16/chrome/test/base/v8_unit_test....
chrome/test/base/v8_unit_test.h:68: // Executes all added javascript libraries.
Returns true if passing so far.
On 2011/10/29 22:15:48, James Hawkins wrote:
> What does "if passing so far" mean?

Done.

http://codereview.chromium.org/8418015/diff/16/chrome/test/base/v8_unit_test....
chrome/test/base/v8_unit_test.h:71: // Initialize paths and libraries.
On 2011/10/29 22:15:48, James Hawkins wrote:
> Initializes

Done.

http://codereview.chromium.org/8418015/diff/16/chrome/test/base/v8_unit_test....
chrome/test/base/v8_unit_test.h:80: // Location of test data (currently
test/data/webui).
On 2011/10/29 22:15:48, James Hawkins wrote:
> Don't add implementation details to the interface (even if in private
section). 
> It's not unlikely to change.  Here and below.

Done.

http://codereview.chromium.org/8418015/diff/16/chrome/test/base/v8_unit_test....
chrome/test/base/v8_unit_test.h:90: static bool had_errors_;
On 2011/10/29 22:15:48, James Hawkins wrote:
> Are statics particularly necessary?

Done.

[Sheridan Rawlins, 2011-11-01 21:48:45]

Back to green after rebase (forcing trybot revision to 108019 to pick up the fix
for crbug.com/99970).

[Paweł Hajdan Jr., 2011-11-02 08:27:40]

http://codereview.chromium.org/8418015/diff/12001/chrome/test/base/v8_unit_te...
File chrome/test/base/v8_unit_test.cc (right):

http://codereview.chromium.org/8418015/diff/12001/chrome/test/base/v8_unit_te...
chrome/test/base/v8_unit_test.cc:16: #define FILE_PATH_TO_ASCII(FP)
WideToASCII((FP).value())
I'm not sure if that's an optimal solution. Mark, could you take a look?

[Sheridan Rawlins, 2011-11-02 18:35:51]

http://codereview.chromium.org/8418015/diff/12001/chrome/test/base/v8_unit_te...
File chrome/test/base/v8_unit_test.cc (right):

http://codereview.chromium.org/8418015/diff/12001/chrome/test/base/v8_unit_te...
chrome/test/base/v8_unit_test.cc:16: #define FILE_PATH_TO_ASCII(FP)
WideToASCII((FP).value())
Mark recommended using MaybeAsASCII() in gchat and agreed to be moved to CC
list.

Done.

On 2011/11/02 08:27:40, Paweł Hajdan Jr. wrote:
> I'm not sure if that's an optimal solution. Mark, could you take a look?

[James Hawkins, 2011-11-02 22:53:37]

LGTM with nits.

http://codereview.chromium.org/8418015/diff/18001/chrome/test/base/v8_unit_te...
File chrome/test/base/v8_unit_test.h (right):

http://codereview.chromium.org/8418015/diff/18001/chrome/test/base/v8_unit_te...
chrome/test/base/v8_unit_test.h:31: // Add a custom helper JS library for your
test.
nit: Save lines, don't wrap per-sentence.  Fill up 80 cols.

http://codereview.chromium.org/8418015/diff/18001/chrome/test/base/v8_unit_te...
chrome/test/base/v8_unit_test.h:34: void AddLibrary(const FilePath&
library_path);
Document params here and elsewhere.

[Sheridan Rawlins, 2011-11-02 23:16:19]

http://codereview.chromium.org/8418015/diff/18001/chrome/test/base/v8_unit_te...
File chrome/test/base/v8_unit_test.h (right):

http://codereview.chromium.org/8418015/diff/18001/chrome/test/base/v8_unit_te...
chrome/test/base/v8_unit_test.h:31: // Add a custom helper JS library for your
test.
On 2011/11/02 22:53:37, James Hawkins wrote:
> nit: Save lines, don't wrap per-sentence.  Fill up 80 cols.

Done.

http://codereview.chromium.org/8418015/diff/18001/chrome/test/base/v8_unit_te...
chrome/test/base/v8_unit_test.h:34: void AddLibrary(const FilePath&
library_path);
On 2011/11/02 22:53:37, James Hawkins wrote:
> Document params here and elsewhere.

Done.

[commit-bot: I haz the power, 2011-11-02 23:17:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scr@chromium.org/8418015/10002

[commit-bot: I haz the power, 2011-11-03 01:25:32]

Change committed as 108391

[Ryan Sleevi, 2011-11-03 03:42:07]

Mac fixes LGTM.

May want to run with a -c on the try-runs for -clobber, or land when the tree is
relatively quiet, just in case there are any further timing issues.

[Sheridan Rawlins, 2011-11-03 15:55:01]

FYI I did a try with -c -t unit_tests and it passed.

On 2011/11/03 03:42:07, Ryan Sleevi wrote:
> Mac fixes LGTM.
> 
> May want to run with a -c on the try-runs for -clobber, or land when the tree
is
> relatively quiet, just in case there are any further timing issues.

[arv (Not doing code reviews), 2011-11-03 17:30:03]

This looks great in general. I found one bug and a learned a lot about the V8
APIs :-)

http://codereview.chromium.org/8418015/diff/17011/chrome/test/base/v8_unit_te...
File chrome/test/base/v8_unit_test.cc (right):

http://codereview.chromium.org/8418015/diff/17011/chrome/test/base/v8_unit_te...
chrome/test/base/v8_unit_test.cc:249: EXPECT_EQ(2, args.Length());
This is not correct. It is allowed to pass one argument to chrome.send.

[Sheridan Rawlins, 2011-11-03 19:18:14]

I'm not sure I'd say it's a bug, but thanks for pointing it out - I think this
expect sequence will be more helpful to test-authors.

http://codereview.chromium.org/8418015/diff/17011/chrome/test/base/v8_unit_te...
File chrome/test/base/v8_unit_test.cc (right):

http://codereview.chromium.org/8418015/diff/17011/chrome/test/base/v8_unit_te...
chrome/test/base/v8_unit_test.cc:249: EXPECT_EQ(2, args.Length());
In this case, anything other than a "testResult" message (which passes 2 args)
is incorrect and should fail.  However it would be more useful to see why it
failed (i.e. what message was sent that failed).  So, I check for at least 1
arg, then check for "testResult" then check for 2 args.

Done.

On 2011/11/03 17:30:04, arv wrote:
> This is not correct. It is allowed to pass one argument to chrome.send.

[Sheridan Rawlins, 2011-11-05 01:04:44]

Just a little more context/clarification of this patch's purpose and why I
didn't consider it a bug to be checking for 2 parameters and the "testResult"
message.

This patch is making it possible to run JavaScript gtests from the unit_tests
harness through the js2unit gyp rule.  Such tests should not be using
chrome.send like those using the browser_tests harness through the js2webui gyp
rule.

The test framework reports success on the following conditions:
1) All JavaScript files loaded/parsed properly
2) The test was able to execute successfully (runTest returns true)
3) The test framework reports success via the "testResult" message (in
testDone()).
4) There were no calls to console.error()

So...  Since unit_tests doesn't have any notion of webui or interprocess
communication, any calls to chrome.send other than the framework's "testResult"
message are an error.

I did, however, honor the insight, which your comment gave that checking
expectations progressively would give authors more clue to the error of their
ways, as it were if they break the assumptions and call chrome.send from their
tests intended to be unit tests without the appropriate mocking.

I hope this helps cement that this patch is good-to-go, and I'll address the
comments in my followup CL, which will be migrating
print_preview_utils_unittests from html / closure -> js / this framework.

Thanks,

-SCR

[arv1, 2011-11-05 01:11:48]

LGTM

Thanks for the clarification. It wasn't clear to me that the only calls to
chrome.send are supposed to come from the test harness.

[commit-bot: I haz the power, 2011-11-05 02:58:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scr@chromium.org/8418015/26001

[commit-bot: I haz the power, 2011-11-05 04:24:22]

Change committed as 108773