Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1128843007:
Add tests for third_party/closure_compiler/compile.py (Closed)
|
Created:
5 years, 7 months ago by Theresa Modified:
5 years, 7 months ago Reviewers:
Dan Beam CC:
chromium-reviews, vitalyp+closure_chromium.org, jlklein+watch-closure_chromium.org, dbeam+watch-closure_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd tests for third_party/closure_compiler/compile.py
Add basic tests for compile.py: valid JS compiles and produces
expected output files; invalid JS does not produce output files;
multiple JS files compile; output wrapper compiles.
BUG=486100
Committed: https://crrev.com/2cbf34d4a44319dcbfb22f42ad5b300ff20802ac
Cr-Commit-Position: refs/heads/master@{#330771}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Merge tests into compiler_test.py #
Total comments: 28
Patch Set 3 : Changes based on last review #
Total comments: 6
Patch Set 4 : More changes from review #Patch Set 5 : Move out_dir creation to _run_js_check() #
Messages
Total messages: 17 (2 generated)
twellington@chromium.org changed reviewers: + dbeam@chromium.org
ptal
https://codereview.chromium.org/1128843007/diff/1/third_party/closure_compile... File third_party/closure_compiler/compile_test.py (right): https://codereview.chromium.org/1128843007/diff/1/third_party/closure_compile... third_party/closure_compiler/compile_test.py:16: "polymer.externs.js") can we merge this in with the customization test? seems duplicative to me...
merged (and renamed) https://codereview.chromium.org/1128843007/diff/1/third_party/closure_compile... File third_party/closure_compiler/compile_test.py (right): https://codereview.chromium.org/1128843007/diff/1/third_party/closure_compile... third_party/closure_compiler/compile_test.py:16: "polymer.externs.js") On 2015/05/19 17:05:38, Dan Beam wrote: > can we merge this in with the customization test? seems duplicative to me... Done.
https://codereview.chromium.org/1128843007/diff/20001/third_party/closure_com... File third_party/closure_compiler/compiler_test.py (right): https://codereview.chromium.org/1128843007/diff/20001/third_party/closure_com... third_party/closure_compiler/compiler_test.py:31: def tearDown(self): # remove all files regardless of success https://codereview.chromium.org/1128843007/diff/20001/third_party/closure_com... third_party/closure_compiler/compiler_test.py:63: with open(out_file, 'r') as file: ' -> " everywhere feasible https://codereview.chromium.org/1128843007/diff/20001/third_party/closure_com... third_party/closure_compiler/compiler_test.py:68: def _createOutFiles(self): why do we need this? shouldn't they be created by .check()? https://codereview.chromium.org/1128843007/diff/20001/third_party/closure_com... third_party/closure_compiler/compiler_test.py:80: if(os.path.exists(file)): the python gods do not approve of your c-like syntax if os.path.exists(file): https://codereview.chromium.org/1128843007/diff/20001/third_party/closure_com... third_party/closure_compiler/compiler_test.py:273: var testScript = function() { console.log("hello world") };""", can you put some \n in here so the difference is less subtle (as well as the code's easier to read)? https://codereview.chromium.org/1128843007/diff/20001/third_party/closure_com... third_party/closure_compiler/compiler_test.py:274: """'use strict';var testScript=function(){console.log("hello world")};\n""") hmmm, why is 'use strict'; prepended? https://codereview.chromium.org/1128843007/diff/20001/third_party/closure_com... third_party/closure_compiler/compiler_test.py:277: self._runCheckerTestExpectSuccess(""" indent off https://codereview.chromium.org/1128843007/diff/20001/third_party/closure_com... third_party/closure_compiler/compiler_test.py:281: output_wrapper="(function(){%output%})();") make this prettier https://codereview.chromium.org/1128843007/diff/20001/third_party/closure_com... third_party/closure_compiler/compiler_test.py:284: source_file = "/tmp/script.js" this should use tempfile instead https://docs.python.org/2/library/tempfile.html https://codereview.chromium.org/1128843007/diff/20001/third_party/closure_com... third_party/closure_compiler/compiler_test.py:286: f.write(""" why are we actually writing these files rather than injecting them into FileCache._cache? https://codereview.chromium.org/1128843007/diff/20001/third_party/closure_com... third_party/closure_compiler/compiler_test.py:298: """) can you reverse the order of source_file/source_file2? and maybe source_file -> source_file1?
https://codereview.chromium.org/1128843007/diff/20001/third_party/closure_com... File third_party/closure_compiler/compiler_test.py (right): https://codereview.chromium.org/1128843007/diff/20001/third_party/closure_com... third_party/closure_compiler/compiler_test.py:68: def _createOutFiles(self): On 2015/05/19 18:23:32, Dan Beam wrote: > why do we need this? shouldn't they be created by .check()? The directory is created in main if it doesn't already exist, which is what this method is doing. I can rename it to createOutDir. https://codereview.chromium.org/1128843007/diff/20001/third_party/closure_com... third_party/closure_compiler/compiler_test.py:274: """'use strict';var testScript=function(){console.log("hello world")};\n""") On 2015/05/19 18:23:33, Dan Beam wrote: > hmmm, why is 'use strict'; prepended? Because the closure compiler always prepends it? In all of my testing, I have always seen the compiled file start with 'use strict' https://codereview.chromium.org/1128843007/diff/20001/third_party/closure_com... third_party/closure_compiler/compiler_test.py:286: f.write(""" On 2015/05/19 18:23:33, Dan Beam wrote: > why are we actually writing these files rather than injecting them into > FileCache._cache? I tried using FileCache._chache and the jar failed to find find the files while it was running. Not sure why, but I can try again (tried several times yesterday).
ptal https://codereview.chromium.org/1128843007/diff/20001/third_party/closure_com... File third_party/closure_compiler/compiler_test.py (right): https://codereview.chromium.org/1128843007/diff/20001/third_party/closure_com... third_party/closure_compiler/compiler_test.py:31: On 2015/05/19 18:23:33, Dan Beam wrote: > def tearDown(self): > # remove all files regardless of success Done. https://codereview.chromium.org/1128843007/diff/20001/third_party/closure_com... third_party/closure_compiler/compiler_test.py:63: with open(out_file, 'r') as file: On 2015/05/19 18:23:33, Dan Beam wrote: > ' -> " everywhere feasible Done. https://codereview.chromium.org/1128843007/diff/20001/third_party/closure_com... third_party/closure_compiler/compiler_test.py:68: def _createOutFiles(self): On 2015/05/19 18:29:23, Theresa Wellington wrote: > On 2015/05/19 18:23:32, Dan Beam wrote: > > why do we need this? shouldn't they be created by .check()? > > The directory is created in main if it doesn't already exist, which is what this > method is doing. I can rename it to createOutDir. Done. https://codereview.chromium.org/1128843007/diff/20001/third_party/closure_com... third_party/closure_compiler/compiler_test.py:80: if(os.path.exists(file)): On 2015/05/19 18:23:33, Dan Beam wrote: > the python gods do not approve of your c-like syntax > > if os.path.exists(file): Done. https://codereview.chromium.org/1128843007/diff/20001/third_party/closure_com... third_party/closure_compiler/compiler_test.py:273: var testScript = function() { console.log("hello world") };""", On 2015/05/19 18:23:33, Dan Beam wrote: > can you put some \n in here so the difference is less subtle (as well as the > code's easier to read)? Done. https://codereview.chromium.org/1128843007/diff/20001/third_party/closure_com... third_party/closure_compiler/compiler_test.py:274: """'use strict';var testScript=function(){console.log("hello world")};\n""") On 2015/05/19 18:29:23, Theresa Wellington wrote: > On 2015/05/19 18:23:33, Dan Beam wrote: > > hmmm, why is 'use strict'; prepended? > > Because the closure compiler always prepends it? In all of my testing, I have > always seen the compiled file start with 'use strict' (not just when running through compile.py but locally running the compiler jar as well) I'm going to guess it's from the _COMMON_CLOSURE_ARGS "--language_in=ECMASCRIPT5_STRICT", https://codereview.chromium.org/1128843007/diff/20001/third_party/closure_com... third_party/closure_compiler/compiler_test.py:277: self._runCheckerTestExpectSuccess(""" On 2015/05/19 18:23:33, Dan Beam wrote: > indent off Done. https://codereview.chromium.org/1128843007/diff/20001/third_party/closure_com... third_party/closure_compiler/compiler_test.py:281: output_wrapper="(function(){%output%})();") On 2015/05/19 18:23:33, Dan Beam wrote: > make this prettier Done. https://codereview.chromium.org/1128843007/diff/20001/third_party/closure_com... third_party/closure_compiler/compiler_test.py:284: source_file = "/tmp/script.js" On 2015/05/19 18:23:32, Dan Beam wrote: > this should use tempfile instead > https://docs.python.org/2/library/tempfile.html Done. https://codereview.chromium.org/1128843007/diff/20001/third_party/closure_com... third_party/closure_compiler/compiler_test.py:286: f.write(""" On 2015/05/19 18:29:22, Theresa Wellington wrote: > On 2015/05/19 18:23:33, Dan Beam wrote: > > why are we actually writing these files rather than injecting them into > > FileCache._cache? > > I tried using FileCache._chache and the jar failed to find find the files while > it was running. Not sure why, but I can try again (tried several times > yesterday). Tried again and it still failed. I think the difference is in check vs. check_multiple. check creates an expanded_file, which gets passed to the compiler. check_multiple just passes the sources files. https://codereview.chromium.org/1128843007/diff/20001/third_party/closure_com... third_party/closure_compiler/compiler_test.py:298: """) On 2015/05/19 18:23:33, Dan Beam wrote: > can you reverse the order of source_file/source_file2? and maybe source_file -> > source_file1? Done.
https://codereview.chromium.org/1128843007/diff/40001/third_party/closure_com... File third_party/closure_compiler/compiler_test.py (right): https://codereview.chromium.org/1128843007/diff/40001/third_party/closure_com... third_party/closure_compiler/compiler_test.py:38: nit: remove \n https://codereview.chromium.org/1128843007/diff/40001/third_party/closure_com... third_party/closure_compiler/compiler_test.py:75: out_file = "/tmp/gen/script.js" Also needs to use tempfile https://codereview.chromium.org/1128843007/diff/40001/third_party/closure_com... third_party/closure_compiler/compiler_test.py:289: + """{console.log("hello world")};})();\n""") I don't think you need the +, also indent more
https://codereview.chromium.org/1128843007/diff/40001/third_party/closure_com... File third_party/closure_compiler/compiler_test.py (right): https://codereview.chromium.org/1128843007/diff/40001/third_party/closure_com... third_party/closure_compiler/compiler_test.py:38: On 2015/05/19 23:04:38, Dan Beam wrote: > nit: remove \n Done. https://codereview.chromium.org/1128843007/diff/40001/third_party/closure_com... third_party/closure_compiler/compiler_test.py:75: out_file = "/tmp/gen/script.js" On 2015/05/19 23:04:39, Dan Beam wrote: > Also needs to use tempfile Done. out_file and out_map need the same base name, so I changed out_file to use tempfile and out_map to to be based on out_file.name. https://codereview.chromium.org/1128843007/diff/40001/third_party/closure_com... third_party/closure_compiler/compiler_test.py:289: + """{console.log("hello world")};})();\n""") On 2015/05/19 23:04:39, Dan Beam wrote: > I don't think you need the +, also indent more Done.
https://codereview.chromium.org/1128843007/diff/20001/third_party/closure_com... File third_party/closure_compiler/compiler_test.py (right): https://codereview.chromium.org/1128843007/diff/20001/third_party/closure_com... third_party/closure_compiler/compiler_test.py:68: def _createOutFiles(self): On 2015/05/19 18:29:23, Theresa Wellington wrote: > On 2015/05/19 18:23:32, Dan Beam wrote: > > why do we need this? shouldn't they be created by .check()? > > The directory is created in main if it doesn't already exist, which is what this > method is doing. I can rename it to createOutDir. why not move this to check() instead of main()? https://code.google.com/p/chromium/codesearch#chromium/src/third_party/closur...
https://codereview.chromium.org/1128843007/diff/20001/third_party/closure_com... File third_party/closure_compiler/compiler_test.py (right): https://codereview.chromium.org/1128843007/diff/20001/third_party/closure_com... third_party/closure_compiler/compiler_test.py:68: def _createOutFiles(self): On 2015/05/20 02:01:00, Dan Beam wrote: > On 2015/05/19 18:29:23, Theresa Wellington wrote: > > On 2015/05/19 18:23:32, Dan Beam wrote: > > > why do we need this? shouldn't they be created by .check()? > > > > The directory is created in main if it doesn't already exist, which is what > this > > method is doing. I can rename it to createOutDir. > > why not move this to check() instead of main()? > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/closur... sorry, make a _ensure_out_file_exists() (or something) and call from check() and check_multiple()?
https://codereview.chromium.org/1128843007/diff/20001/third_party/closure_com... File third_party/closure_compiler/compiler_test.py (right): https://codereview.chromium.org/1128843007/diff/20001/third_party/closure_com... third_party/closure_compiler/compiler_test.py:68: def _createOutFiles(self): On 2015/05/20 02:05:33, Dan Beam wrote: > On 2015/05/20 02:01:00, Dan Beam wrote: > > On 2015/05/19 18:29:23, Theresa Wellington wrote: > > > On 2015/05/19 18:23:32, Dan Beam wrote: > > > > why do we need this? shouldn't they be created by .check()? > > > > > > The directory is created in main if it doesn't already exist, which is what > > this > > > method is doing. I can rename it to createOutDir. > > > > why not move this to check() instead of main()? > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/closur... > > sorry, make a _ensure_out_file_exists() (or something) and call from check() and > check_multiple()? I moved it to _run_js_check() since it's not actually needed until the compiler is run.
lgtm
The CQ bit was checked by twellington@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1128843007/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/2cbf34d4a44319dcbfb22f42ad5b300ff20802ac Cr-Commit-Position: refs/heads/master@{#330771} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
