Support automatically resolving dependencies in javascript tests.

Support automatically resolving dependencies in javascript tests.





BUG=371692
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=275260

[Peter Lundblad, 2014-05-30 15:08:45]

@sky for chrome/test/base

NOTE: If anyone has a suggestion for how to avoid the duplication of test files
in chrome_tests.gypi, I am all open for implementing that.

[sky, 2014-05-30 16:25:12]

I'm not a good reviewer for chrome/test/base/js2gtest.js . Is there another
owner you can find?

[Peter Lundblad, 2014-05-30 16:34:13]

sky@chromium.org writes:
> I'm not a good reviewer for chrome/test/base/js2gtest.js . Is there another
> owner you can find?

Sure, I'll take you off this one.

Thanks,
//Peter

> https://codereview.chromium.org/304793002/

-- 

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmazzoni, 2014-05-30 17:03:41]

https://codereview.chromium.org/304793002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/chromeos/chromevox/common/aria_util_test.js
(right):

https://codereview.chromium.org/304793002/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/common/aria_util_test.js:19:
extraLibraries: [
Let's use something different like extraClosureModules or closureModuleDeps
rather than overloading extraLibraries.

https://codereview.chromium.org/304793002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/chromeos/chromevox/tools/find_js_files.py (right):

https://codereview.chromium.org/304793002/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/tools/find_js_files.py:18:
sys.path.insert(0, os.path.join(
nit: indent

https://codereview.chromium.org/304793002/diff/20001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/304793002/diff/20001/chrome/chrome_tests.gypi...
chrome/chrome_tests.gypi:9: 'chromevox_deps_js_file':
'<(SHARED_INTERMEDIATE_DIR)/chrome/browser/resources/chromeos/chromevox/deps.js',
I think it'd make sense to move these into a new gyp file and a new test target.
It's too much special-casing now and it just clutters this file.

https://codereview.chromium.org/304793002/diff/20001/tools/gypv8sh.py
File tools/gypv8sh.py (left):

https://codereview.chromium.org/304793002/diff/20001/tools/gypv8sh.py#oldcode9
tools/gypv8sh.py:9: Usage:
Removing this because the usage is below in the code?

[David Tseng, 2014-05-30 17:08:46]

Drive-by

https://codereview.chromium.org/304793002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/chromeos/chromevox/common/aria_util_test.js
(right):

https://codereview.chromium.org/304793002/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/common/aria_util_test.js:22:
'cvox.ChromeVox',]
Is order important here (if not, sort).

https://codereview.chromium.org/304793002/diff/20001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/304793002/diff/20001/chrome/chrome_tests.gypi...
chrome/chrome_tests.gypi:2054: # are stripped by the linker.  Therefore, sources
mentioned in tihis
this

https://codereview.chromium.org/304793002/diff/20001/chrome/test/base/js2gtes...
File chrome/test/base/js2gtest.js (right):

https://codereview.chromium.org/304793002/diff/20001/chrome/test/base/js2gtes...
chrome/test/base/js2gtest.js:197: // Always add closure libaries base.js if
provided by deps.
libraries

https://codereview.chromium.org/304793002/diff/20001/chrome/test/base/js2gtes...
chrome/test/base/js2gtest.js:259:
resolveLibraries(this[testFixture].prototype.extraLibraries));
Nice.

[Peter Lundblad, 2014-05-31 03:42:09]

dtseng@chromium.org writes:
> Drive-by
> 
> 
>
https://codereview.chromium.org/304793002/diff/20001/chrome/browser/resources...
> File
> chrome/browser/resources/chromeos/chromevox/common/aria_util_test.js
> (right):
> 
>
https://codereview.chromium.org/304793002/diff/20001/chrome/browser/resources...
> chrome/browser/resources/chromeos/chromevox/common/aria_util_test.js:22:
> 'cvox.ChromeVox',]
> Is order important here (if not, sort).

Sorted and doesn't seem to matter.

> https://codereview.chromium.org/304793002/diff/20001/chrome/chrome_tests.gypi
> File chrome/chrome_tests.gypi (right):
> 
>
https://codereview.chromium.org/304793002/diff/20001/chrome/chrome_tests.gypi...
> chrome/chrome_tests.gypi:2054: # are stripped by the linker.  Therefore,
> sources mentioned in tihis
> this

Done.

>
https://codereview.chromium.org/304793002/diff/20001/chrome/test/base/js2gtes...
> File chrome/test/base/js2gtest.js (right):
> 
>
https://codereview.chromium.org/304793002/diff/20001/chrome/test/base/js2gtes...
> chrome/test/base/js2gtest.js:197: // Always add closure libaries base.js
> if provided by deps.
> libraries

Actually, 'library's', fixed.

>
https://codereview.chromium.org/304793002/diff/20001/chrome/test/base/js2gtes...
> chrome/test/base/js2gtest.js:259:
> resolveLibraries(this[testFixture].prototype.extraLibraries));
> Nice.

Thanks;)

//Peter

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Peter Lundblad, 2014-05-31 03:47:20]

dmazzoni@chromium.org writes:
> 
>
https://codereview.chromium.org/304793002/diff/20001/chrome/browser/resources...
> File
> chrome/browser/resources/chromeos/chromevox/common/aria_util_test.js
> (right):
> 
>
https://codereview.chromium.org/304793002/diff/20001/chrome/browser/resources...
> chrome/browser/resources/chromeos/chromevox/common/aria_util_test.js:19:
> extraLibraries: [
> Let's use something different like extraClosureModules or
> closureModuleDeps rather than overloading extraLibraries.

Settled for closureModuleDeps.

>
https://codereview.chromium.org/304793002/diff/20001/chrome/browser/resources...
> File chrome/browser/resources/chromeos/chromevox/tools/find_js_files.py
> (right):
> 
>
https://codereview.chromium.org/304793002/diff/20001/chrome/browser/resources...
> chrome/browser/resources/chromeos/chromevox/tools/find_js_files.py:18:
> sys.path.insert(0, os.path.join(
> nit: indent

Did some reformatting here.

> https://codereview.chromium.org/304793002/diff/20001/chrome/chrome_tests.gypi
> File chrome/chrome_tests.gypi (right):
> 
>
https://codereview.chromium.org/304793002/diff/20001/chrome/chrome_tests.gypi...
> chrome/chrome_tests.gypi:9: 'chromevox_deps_js_file':
>
'<(SHARED_INTERMEDIATE_DIR)/chrome/browser/resources/chromeos/chromevox/deps.js',
> I think it'd make sense to move these into a new gyp file and a new test
> target. It's too much special-casing now and it just clutters this file.

You're right.  I created the new chromevox_tests target.  There's a bunch of
additional churn because of that (such as splitting out test framework code from
its tests;).  Please have a look at that as well.

> https://codereview.chromium.org/304793002/diff/20001/tools/gypv8sh.py
> File tools/gypv8sh.py (left):
> 
>
https://codereview.chromium.org/304793002/diff/20001/tools/gypv8sh.py#oldcode9
> tools/gypv8sh.py:9: Usage:
> Removing this because the usage is below in the code?

It had already bitrotted which is worse than not having it, so eliminated this
duplication.

Thanks,
//Peter

> https://codereview.chromium.org/304793002/

-- 

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmazzoni, 2014-05-31 06:16:48]

Moving web_ui_browser_test_browsertest.cc into its own file was definitely a
good cleanup idea (besides being necessary to make use of web_ui_browser_test
from another target).

https://codereview.chromium.org/304793002/diff/80001/chrome/test/base/js2gtes...
File chrome/test/base/js2gtest.js (right):

https://codereview.chromium.org/304793002/diff/80001/chrome/test/base/js2gtes...
chrome/test/base/js2gtest.js:126: * Maps namespace/object names to the path to
the file that provides them.
How about just "class names" instead of namespace/object names - that's what
they are 90% of the time.

https://codereview.chromium.org/304793002/diff/80001/chrome/test/base/js2gtes...
chrome/test/base/js2gtest.js:133: * For each path included in the |depsFile|, if
any, contains
Nit: I'd word this more similarly to the one above: Maps a path name to the list
of class names required by the file.

https://codereview.chromium.org/304793002/diff/80001/chrome/test/base/js2gtes...
chrome/test/base/js2gtest.js:145: * @param Array.<string> provides Objects
provided by this file.
Objects -> Classes?

[Peter Lundblad, 2014-06-03 06:00:59]

dmazzoni@chromium.org writes:
> Moving web_ui_browser_test_browsertest.cc into its own file was definitely a
> good cleanup idea (besides being necessary to make use of  
> web_ui_browser_test
> from another target).
> 
> 
> 
> 
>
https://codereview.chromium.org/304793002/diff/80001/chrome/test/base/js2gtes...
> File chrome/test/base/js2gtest.js (right):
> 
>
https://codereview.chromium.org/304793002/diff/80001/chrome/test/base/js2gtes...
> chrome/test/base/js2gtest.js:126: * Maps namespace/object names to the
> path to the file that provides them.
> How about just "class names" instead of namespace/object names - that's
> what they are 90% of the time.

I reworded to object names everywhere which is consistent with the closure
jsdoc.

>
https://codereview.chromium.org/304793002/diff/80001/chrome/test/base/js2gtes...
> chrome/test/base/js2gtest.js:133: * For each path included in the
> |depsFile|, if any, contains
> Nit: I'd word this more similarly to the one above: Maps a path name to
> the list of class names required by the file.

Good idea, done.

>
https://codereview.chromium.org/304793002/diff/80001/chrome/test/base/js2gtes...
> chrome/test/base/js2gtest.js:145: * @param Array.<string> provides
> Objects provided by this file.
> Objects -> Classes?

See above.

> https://codereview.chromium.org/304793002/

-- 

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmazzoni, 2014-06-03 16:16:28]

lgtm

Love it.

https://codereview.chromium.org/304793002/diff/100001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (left):

https://codereview.chromium.org/304793002/diff/100001/chrome/chrome_tests.gyp...
chrome/chrome_tests.gypi:1615: '<(accessibility_audit_js)',
I don't think you want to delete this - this allows the audit to run in all
WebUI tests.

https://codereview.chromium.org/304793002/diff/100001/chrome/js_unittest_rule...
File chrome/js_unittest_rules.gypi (left):

https://codereview.chromium.org/304793002/diff/100001/chrome/js_unittest_rule...
chrome/js_unittest_rules.gypi:63: '<(accessibility_audit_js)',
Same here, are you sure you want to delete this?

https://codereview.chromium.org/304793002/diff/100001/chrome/test/data/webui/...
File chrome/test/data/webui/test_api.js (right):

https://codereview.chromium.org/304793002/diff/100001/chrome/test/data/webui/...
chrome/test/data/webui/test_api.js:125: * Extra libraries to add before loading
this test file.
This comment should maybe mention that it gathers dependencies recursively /
transitively

https://codereview.chromium.org/304793002/diff/100001/chrome/third_party/chro...
File
chrome/third_party/chromevox/third_party/closure-library/closure/bin/build/depswriter.py
(right):

https://codereview.chromium.org/304793002/diff/100001/chrome/third_party/chro...
chrome/third_party/chromevox/third_party/closure-library/closure/bin/build/depswriter.py:37:
__author__ = 'nnaze@google.com (Nathan Naze)'
Do we leave authors in for third_party? I don't recall

[Peter Lundblad, 2014-06-03 21:23:21]

dmazzoni@chromium.org writes:
> lgtm
> 
> Love it.
> 
> 
> 
>
https://codereview.chromium.org/304793002/diff/100001/chrome/chrome_tests.gypi
> File chrome/chrome_tests.gypi (left):
> 
>
https://codereview.chromium.org/304793002/diff/100001/chrome/chrome_tests.gyp...
> chrome/chrome_tests.gypi:1615: '<(accessibility_audit_js)',
> I don't think you want to delete this - this allows the audit to run in
> all WebUI tests.

This is only used for the run during code generation.  I don't think we need
the audit there.  I also don't think it needs to be an input file to this rule
since changing that file doesn't affect the output.

>
https://codereview.chromium.org/304793002/diff/100001/chrome/js_unittest_rule...
> File chrome/js_unittest_rules.gypi (left):
> 
>
https://codereview.chromium.org/304793002/diff/100001/chrome/js_unittest_rule...
> chrome/js_unittest_rules.gypi:63: '<(accessibility_audit_js)',
> Same here, are you sure you want to delete this?

Yes.

>
https://codereview.chromium.org/304793002/diff/100001/chrome/test/data/webui/...
> File chrome/test/data/webui/test_api.js (right):
> 
>
https://codereview.chromium.org/304793002/diff/100001/chrome/test/data/webui/...
> chrome/test/data/webui/test_api.js:125: * Extra libraries to add before
> loading this test file.
> This comment should maybe mention that it gathers dependencies
> recursively / transitively

Changed wording.

>
https://codereview.chromium.org/304793002/diff/100001/chrome/third_party/chro...
> File
>
chrome/third_party/chromevox/third_party/closure-library/closure/bin/build/depswriter.py
> (right):
> 
>
https://codereview.chromium.org/304793002/diff/100001/chrome/third_party/chro...
>
chrome/third_party/chromevox/third_party/closure-library/closure/bin/build/depswriter.py:37:
> __author__ = 'nnaze@google.com (Nathan Naze)'
> Do we leave authors in for third_party? I don't recall

I've never seen us do style adjustments on third_party code, and I hope we
aren't;)  The @authors line comes from the open source version
of the library, FWIW.

Thanks,
//Peter

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Peter Lundblad, 2014-06-03 22:48:22]

@jcivelli: Could you have a look at chrome/test/base?

@jhawkins: Would you mind having a look at chrome/browser/ui/webui?

Thanks,
//Peter

[James Hawkins, 2014-06-03 22:53:34]

webui lgtm

[Peter Lundblad, 2014-06-04 21:13:23]

@jcivelli: Friendly ping. Would you have a chance to have a look, please?

[jcivelli, 2014-06-04 23:35:20]

On 2014/06/04 21:13:23, Peter Lundblad wrote:
> @jcivelli: Friendly ping. Would you have a chance to have a look, please?

Can you clarify whether you renamed web_ui_browsertest.h or changed it?
I am not knowledgeable in JS. Can you find someone else for
chrome/test/base/js2gtest.js?

[Peter Lundblad, 2014-06-04 23:57:49]

@phajdan: Could you have a look at test/base/..., please?

[Peter Lundblad, 2014-06-05 00:06:34]

jcivelli@google.com writes:
> On 2014/06/04 21:13:23, Peter Lundblad wrote:
> > @jcivelli: Friendly ping. Would you have a chance to have a look, please?
> 
> Can you clarify whether you renamed web_ui_browsertest.h or changed it?

It was renamed to be consistent with in_process_browser_test.*.


> I am not knowledgeable in JS. Can you find someone else for
> chrome/test/base/js2gtest.js?

Alright, tried another owner as well.

Thanks,
//Peter

> https://codereview.chromium.org/304793002/

-- 

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Paweł Hajdan Jr., 2014-06-05 10:24:03]

LGTM

[Peter Lundblad, 2014-06-05 14:57:02]

The CQ bit was checked by plundblad@chromium.org

[commit-bot: I haz the power, 2014-06-05 15:00:38]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/plundblad@chromium.org/304793002/140001

[Peter Lundblad, 2014-06-05 17:04:26]

The CQ bit was unchecked by plundblad@chromium.org

[Peter Lundblad, 2014-06-05 18:08:03]

The CQ bit was checked by plundblad@chromium.org

[commit-bot: I haz the power, 2014-06-05 18:09:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/plundblad@chromium.org/304793002/160001

[commit-bot: I haz the power, 2014-06-05 22:17:02]

Change committed as 275260