New issue
Advanced search Search tips

Issue 867700 link

Starred by 3 users
Status: Fixed
Owner:
tapted@chromium.org
Closed: Aug 27
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Task

Blocking:
issue 860355



Sign in to add a comment

Gallery unit tests should be run through the closure compiler.

Project Member Reported by tapted@chromium.org, Jul 25 
Chrome Version       : 69.0.3493.3

This will be a long road...

I tried adding just one and there were a ton of errors.

It would also be nice if we could encode the dependencies somehow.. Currently we use the .gn file pretty much like a list of "#includes" for each js file individually.

That's neat, but it's a bit verbose.

If that _is_ necessary, it would also be nice if we could use the .gn dependency list to _generate_ a list of #includes for the unit_test.html files that just have a bunch of <script src=> tags.

Oooor, it would be nice to use html imports so that we don't have to flatten the dependency tree for each unit_test.html

Or it would be nice to use ECMA6 imports.

Or it would be nice to use typescript.

Currently maintaining javascript based on 5-year-old tech is a real burden.

Comment 2 by tapted@chromium.org, Jul 31

Blocking: 860355

Comment 3 by tapted@chromium.org, Aug 1

Components: -Platform>Apps>FileManager
Summary: Gallery unit tests should be run through the closure compiler. (was: FileManager and Gallery unit tests should be run through the closure compiler.)
Project Member

Comment 4 by bugdroid1@chromium.org, Aug 2 

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

commit 63af8d5f9bbb4a065a76073e44855c54a6444f2f
Author: Trent Apted <tapted@chromium.org>
Date: Thu Aug 02 03:03:43 2018

Gallery: closure-compile image_editor/image_encoder_unittest.js

The shared, unittest_util.js optionally overwrites some vars specified
by externs that may or may not exist in the unit test. Suppress the
warnings around these.

Bug:  867700 ,  860355 
Change-Id: I461ed7921e7d454869d8e64de8c0eeeb626e4af7
Reviewed-on: https://chromium-review.googlesource.com/1154774
Reviewed-by: Noel Gordon <noel@chromium.org>
Commit-Queue: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#580043}
[modify] https://crrev.com/63af8d5f9bbb4a065a76073e44855c54a6444f2f/ui/file_manager/file_manager/common/js/BUILD.gn
[modify] https://crrev.com/63af8d5f9bbb4a065a76073e44855c54a6444f2f/ui/file_manager/file_manager/common/js/unittest_util.js
[modify] https://crrev.com/63af8d5f9bbb4a065a76073e44855c54a6444f2f/ui/file_manager/gallery/js/image_editor/BUILD.gn
[modify] https://crrev.com/63af8d5f9bbb4a065a76073e44855c54a6444f2f/ui/file_manager/gallery/js/image_editor/image_encoder_unittest.js
[modify] https://crrev.com/63af8d5f9bbb4a065a76073e44855c54a6444f2f/ui/file_manager/gallery/js/image_editor/test_util.js
Project Member

Comment 5 by bugdroid1@chromium.org, Aug 3 

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

commit a93d9c56878e80848daed075d8f882072e9e731d
Author: Trent Apted <tapted@chromium.org>
Date: Fri Aug 03 02:27:34 2018

Gallery: closure-compile image_editor/exif_encoder_unittest.js

Bug:  867700 
Change-Id: Icab0365cfaeb5a8a5c6daaffcc21db3437cc8e14
Reviewed-on: https://chromium-review.googlesource.com/1158105
Reviewed-by: Noel Gordon <noel@chromium.org>
Commit-Queue: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#580416}
[modify] https://crrev.com/a93d9c56878e80848daed075d8f882072e9e731d/ui/file_manager/gallery/js/image_editor/BUILD.gn
[modify] https://crrev.com/a93d9c56878e80848daed075d8f882072e9e731d/ui/file_manager/gallery/js/image_editor/exif_encoder_unittest.js
Project Member

Comment 6 by bugdroid1@chromium.org, Aug 9 

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

commit efca1917701b0c85a0d7c3316eadaf179dbf1cc7
Author: Trent Apted <tapted@chromium.org>
Date: Thu Aug 09 05:08:31 2018

FileManager: Generate unit test html files from closure dependencies.

JS files don't name their `#includes`, but for closure compilation we
must effectively list each .js file's #includes in build dependencies.
We can use this to generate the list of .js files, in the correct
order, that we use for js unittests. Manually creating the
foo_unittest.html files is fiddly, error-prone, and is not robust to
files being moved around. However, it does require the unittest js
file to be properly closure-compiled.

Start by obsoleting the 3 foo_unittest.html files in
gallery/image_editor, which are the only ones currently closure-
compiled.

Note that the dependency files are created _regardless_ of the
`closure_compilation = true` gn flag. i.e. this does not introduce a
dependency on the closure _compiler_: dependency information comes from
the GN build files alone. The closure_compilation flag only affects the
js_type_check(..) targets, but the approach here uses js_library(..)
targets.

Bug:  867700 
Cq-Include-Trybots: luci.chromium.try:closure_compilation
Change-Id: I91b3a726b999a9f7db721fefc23b3f6512fd3c0c
Reviewed-on: https://chromium-review.googlesource.com/1161720
Reviewed-by: Noel Gordon <noel@chromium.org>
Commit-Queue: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581795}
[modify] https://crrev.com/efca1917701b0c85a0d7c3316eadaf179dbf1cc7/chrome/browser/chromeos/file_manager/file_manager_jstest_base.cc
[modify] https://crrev.com/efca1917701b0c85a0d7c3316eadaf179dbf1cc7/chrome/browser/chromeos/file_manager/file_manager_jstest_base.h
[modify] https://crrev.com/efca1917701b0c85a0d7c3316eadaf179dbf1cc7/chrome/browser/chromeos/file_manager/gallery_jstest.cc
[modify] https://crrev.com/efca1917701b0c85a0d7c3316eadaf179dbf1cc7/chrome/test/BUILD.gn
[modify] https://crrev.com/efca1917701b0c85a0d7c3316eadaf179dbf1cc7/ui/file_manager/BUILD.gn
[add] https://crrev.com/efca1917701b0c85a0d7c3316eadaf179dbf1cc7/ui/file_manager/file_manager/foreground/js/metadata/metadata_dispatcher_mock_deps.js
[modify] https://crrev.com/efca1917701b0c85a0d7c3316eadaf179dbf1cc7/ui/file_manager/gallery/js/image_editor/BUILD.gn
[delete] https://crrev.com/070d9fe4b6dec96c4c155289def06f095aa9e26a/ui/file_manager/gallery/js/image_editor/exif_encoder_unittest.html
[delete] https://crrev.com/070d9fe4b6dec96c4c155289def06f095aa9e26a/ui/file_manager/gallery/js/image_editor/image_encoder_unittest.html
[delete] https://crrev.com/070d9fe4b6dec96c4c155289def06f095aa9e26a/ui/file_manager/gallery/js/image_editor/image_view_unittest.html
[add] https://crrev.com/efca1917701b0c85a0d7c3316eadaf179dbf1cc7/ui/file_manager/js_unit_test.py
[add] https://crrev.com/efca1917701b0c85a0d7c3316eadaf179dbf1cc7/ui/file_manager/js_unit_tests.gni
Project Member

Comment 7 by bugdroid1@chromium.org, Aug 10 

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

commit 847d66e5a729216c3a71e63ff6aa0941076da766
Author: Trent Apted <tapted@chromium.org>
Date: Fri Aug 10 09:49:29 2018

Gallery: Use externs_list properly under image_editor

The current approach to externs in file_manager js code requires
js_type_check in various modules to re-list all the externs used by
their dependencies, even if none of them are used in that module.

This CL kills the js_library("closure_compile_externs") in the Gallery
image_editor and puts externs named there where they belong.

To help audit this, the js_unit_tests() template is modified to generate
a js_type_check target for each unit test. This ensures that each unit
test has a complete dependency chain, including externs attached properly
to any of its dependencies.

The changes do not attempt to do full IWYU: if an extern is picked up
transitively in a js_type_check target, it is not listed more than once.
Eventually, breaking out more unit tests will ensure we have dependencies
better described, since each unit test has its own js_type_check.

Bug:  867700 
Change-Id: I06fc0359627728046408493088a5bc407e54484b
Reviewed-on: https://chromium-review.googlesource.com/1166610
Reviewed-by: Noel Gordon <noel@chromium.org>
Commit-Queue: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582100}
[add] https://crrev.com/847d66e5a729216c3a71e63ff6aa0941076da766/ui/file_manager/externs/BUILD.gn
[modify] https://crrev.com/847d66e5a729216c3a71e63ff6aa0941076da766/ui/file_manager/file_manager/common/js/BUILD.gn
[modify] https://crrev.com/847d66e5a729216c3a71e63ff6aa0941076da766/ui/file_manager/file_manager/foreground/elements/BUILD.gn
[modify] https://crrev.com/847d66e5a729216c3a71e63ff6aa0941076da766/ui/file_manager/file_manager/foreground/js/BUILD.gn
[modify] https://crrev.com/847d66e5a729216c3a71e63ff6aa0941076da766/ui/file_manager/file_manager/foreground/js/metadata/BUILD.gn
[modify] https://crrev.com/847d66e5a729216c3a71e63ff6aa0941076da766/ui/file_manager/file_manager/test/BUILD.gn
[modify] https://crrev.com/847d66e5a729216c3a71e63ff6aa0941076da766/ui/file_manager/gallery/js/BUILD.gn
[modify] https://crrev.com/847d66e5a729216c3a71e63ff6aa0941076da766/ui/file_manager/gallery/js/image_editor/BUILD.gn
[modify] https://crrev.com/847d66e5a729216c3a71e63ff6aa0941076da766/ui/file_manager/js_unit_tests.gni
Project Member

Comment 8 by bugdroid1@chromium.org, Aug 11 

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

commit 3c854146630ab98a03bbe7634b2fd5b17628a16f
Author: Trent Apted <tapted@chromium.org>
Date: Sat Aug 11 10:38:21 2018

Gallery: closure compile ribbon_unittest.js

Give it a js_unit_tests target, and lint the externs in gallery/js/BUILD.gn.

Delete externs/gallery_event.js - it's currently unreferenced and unused.
ribbon.js gets the definition of event.item from an assignment in
gallery_data_model.js - not from this extern file.

Bug:  867700 
Change-Id: Iae49ca0af61768b37274db2314630cbde9b120ef
Reviewed-on: https://chromium-review.googlesource.com/1170452
Reviewed-by: Noel Gordon <noel@chromium.org>
Commit-Queue: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582446}
[modify] https://crrev.com/3c854146630ab98a03bbe7634b2fd5b17628a16f/chrome/browser/chromeos/file_manager/gallery_jstest.cc
[modify] https://crrev.com/3c854146630ab98a03bbe7634b2fd5b17628a16f/ui/file_manager/BUILD.gn
[delete] https://crrev.com/b6cde39f3618f24c1a0b40419e3acd83fe0a4f0f/ui/file_manager/externs/gallery_event.js
[modify] https://crrev.com/3c854146630ab98a03bbe7634b2fd5b17628a16f/ui/file_manager/gallery/js/BUILD.gn
[delete] https://crrev.com/b6cde39f3618f24c1a0b40419e3acd83fe0a4f0f/ui/file_manager/gallery/js/ribbon_unittest.html
[modify] https://crrev.com/3c854146630ab98a03bbe7634b2fd5b17628a16f/ui/file_manager/gallery/js/ribbon_unittest.js
Project Member

Comment 9 by bugdroid1@chromium.org, Aug 13 

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

commit a98a9d467b0102a2d256d9c7e0d59d0e9ce2d372
Author: Trent Apted <tapted@chromium.org>
Date: Mon Aug 13 02:01:25 2018

Gallery: Closure compile gallery_util_unittest.js

The file itself already compiles, but when we audit the dependency tree,
we find a bunch of hidden dependencies between externs files.

To fix, move these externs to the volume_manager_wrapper target and
update all modules already depending on the wrapper already to get these
externs via that target instead of repeating them redundantly.

Some modules depend on (e.g.) the volume_info.js extern but don't
already have a dependency on volume_manager_wrapper. This CL doesn't
add one for those, but they may need updating later.

Bug:  867700 
Change-Id: I4e4e5b3c9fb6f59af5f06fa945c34c58832f8fe4
Reviewed-on: https://chromium-review.googlesource.com/1171970
Reviewed-by: Noel Gordon <noel@chromium.org>
Commit-Queue: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582494}
[modify] https://crrev.com/a98a9d467b0102a2d256d9c7e0d59d0e9ce2d372/chrome/browser/chromeos/file_manager/gallery_jstest.cc
[modify] https://crrev.com/a98a9d467b0102a2d256d9c7e0d59d0e9ce2d372/ui/file_manager/audio_player/js/BUILD.gn
[modify] https://crrev.com/a98a9d467b0102a2d256d9c7e0d59d0e9ce2d372/ui/file_manager/file_manager/foreground/js/BUILD.gn
[modify] https://crrev.com/a98a9d467b0102a2d256d9c7e0d59d0e9ce2d372/ui/file_manager/file_manager/foreground/js/ui/BUILD.gn
[modify] https://crrev.com/a98a9d467b0102a2d256d9c7e0d59d0e9ce2d372/ui/file_manager/gallery/js/BUILD.gn
[delete] https://crrev.com/8f99335e0353ec6aaf7ae8a6c6b864f7a8d59e04/ui/file_manager/gallery/js/gallery_util_unittest.html
[modify] https://crrev.com/a98a9d467b0102a2d256d9c7e0d59d0e9ce2d372/ui/file_manager/video_player/js/BUILD.gn
Project Member

Comment 10 by bugdroid1@chromium.org, Aug 13 

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

commit 80aae53bab59ba5526037b6ef625e654e6f95896
Author: Trent Apted <tapted@chromium.org>
Date: Mon Aug 13 02:43:39 2018

Gallery: closure compile dimmable_ui_controller_unittest.js

This one was straightforward. The deleted .html harness had a mock for
ContentMetadataProvider and some chrome.* stuff. However it appears to
be unnecessary: neither the test, nor dimmable_ui_controller.js itself
use these.

Bug:  867700 
Change-Id: Iec0bbe7d7126e7dcf8eaffe5f189c0655e6a2e74
Reviewed-on: https://chromium-review.googlesource.com/1171971
Commit-Queue: Trent Apted <tapted@chromium.org>
Reviewed-by: Noel Gordon <noel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582496}
[modify] https://crrev.com/80aae53bab59ba5526037b6ef625e654e6f95896/chrome/browser/chromeos/file_manager/gallery_jstest.cc
[modify] https://crrev.com/80aae53bab59ba5526037b6ef625e654e6f95896/ui/file_manager/gallery/js/BUILD.gn
[delete] https://crrev.com/0ef1727a92e833e4861e1c2ee8ad8fbaf01e4d57/ui/file_manager/gallery/js/dimmable_ui_controller_unittest.html
Project Member

Comment 11 by bugdroid1@chromium.org, Aug 13 

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

commit d110d84dd4df47d0167ebc4454fe2d702ddb1842
Author: Trent Apted <tapted@chromium.org>
Date: Mon Aug 13 23:36:55 2018

Gallery: closure compile gallery_data_model_unittest.js

Creates shared unit test helpers for MockMetadata and MockGalleryItem
since these concepts are used in multiple unittest files. This means
updating the corresponding unittest.html files that are not yet auto-
generated.

Bug:  867700 
Change-Id: Ia77bac5f9dc14abb1f92bb90a8c545d6998e0160
Reviewed-on: https://chromium-review.googlesource.com/1172170
Reviewed-by: Noel Gordon <noel@chromium.org>
Commit-Queue: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582746}
[modify] https://crrev.com/d110d84dd4df47d0167ebc4454fe2d702ddb1842/chrome/browser/chromeos/file_manager/gallery_jstest.cc
[modify] https://crrev.com/d110d84dd4df47d0167ebc4454fe2d702ddb1842/ui/file_manager/file_manager/foreground/js/actions_model_unittest.html
[modify] https://crrev.com/d110d84dd4df47d0167ebc4454fe2d702ddb1842/ui/file_manager/file_manager/foreground/js/actions_model_unittest.js
[modify] https://crrev.com/d110d84dd4df47d0167ebc4454fe2d702ddb1842/ui/file_manager/file_manager/foreground/js/metadata/BUILD.gn
[add] https://crrev.com/d110d84dd4df47d0167ebc4454fe2d702ddb1842/ui/file_manager/file_manager/foreground/js/metadata/mock_metadata.js
[modify] https://crrev.com/d110d84dd4df47d0167ebc4454fe2d702ddb1842/ui/file_manager/file_manager/foreground/js/task_controller_unittest.html
[modify] https://crrev.com/d110d84dd4df47d0167ebc4454fe2d702ddb1842/ui/file_manager/file_manager/foreground/js/task_controller_unittest.js
[modify] https://crrev.com/d110d84dd4df47d0167ebc4454fe2d702ddb1842/ui/file_manager/gallery/js/BUILD.gn
[delete] https://crrev.com/ebecd9c09916cbcbcf503d2c2f770f5aa618802f/ui/file_manager/gallery/js/gallery_data_model_unittest.html
[modify] https://crrev.com/d110d84dd4df47d0167ebc4454fe2d702ddb1842/ui/file_manager/gallery/js/gallery_data_model_unittest.js
[modify] https://crrev.com/d110d84dd4df47d0167ebc4454fe2d702ddb1842/ui/file_manager/gallery/js/gallery_item_unittest.html
[modify] https://crrev.com/d110d84dd4df47d0167ebc4454fe2d702ddb1842/ui/file_manager/gallery/js/gallery_item_unittest.js
[modify] https://crrev.com/d110d84dd4df47d0167ebc4454fe2d702ddb1842/ui/file_manager/gallery/js/image_editor/BUILD.gn
[modify] https://crrev.com/d110d84dd4df47d0167ebc4454fe2d702ddb1842/ui/file_manager/gallery/js/image_editor/image_view_unittest.js
[add] https://crrev.com/d110d84dd4df47d0167ebc4454fe2d702ddb1842/ui/file_manager/gallery/js/mock_gallery_item.js
Project Member

Comment 12 by bugdroid1@chromium.org, Aug 17 

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

commit dddfef8180bf63baa22068768bf408a8e4c95799
Author: Trent Apted <tapted@chromium.org>
Date: Fri Aug 17 08:12:21 2018

Gallery: Closure compile slide_mode_unittest.js

Bug:  867700 
Cq-Include-Trybots: luci.chromium.try:closure_compilation
Change-Id: I7e0fbb902d969ded9c4713be217c5bff4cba3281
Reviewed-on: https://chromium-review.googlesource.com/1177210
Commit-Queue: Trent Apted <tapted@chromium.org>
Reviewed-by: Noel Gordon <noel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#583990}
[modify] https://crrev.com/dddfef8180bf63baa22068768bf408a8e4c95799/chrome/browser/chromeos/file_manager/gallery_jstest.cc
[modify] https://crrev.com/dddfef8180bf63baa22068768bf408a8e4c95799/ui/file_manager/gallery/js/BUILD.gn
[delete] https://crrev.com/7aa2851d30a93d334978f87ece9aeb9601393c39/ui/file_manager/gallery/js/slide_mode_unittest.html
[modify] https://crrev.com/dddfef8180bf63baa22068768bf408a8e4c95799/ui/file_manager/gallery/js/slide_mode_unittest.js

Comment 15 by tapted@chromium.org, Aug 27

Status: Fixed (was: Assigned)
Fixed. Still need to fix
 - file_manager unittests
 - integration tests.
►

Sign in to add a comment