Add tests for third_party/closure_compiler/compile.py

third_party/closure_compiler/compiler_test.py

Index: third_party/closure_compiler/compiler_test.py
diff --git a/third_party/closure_compiler/compiler_customization_test.py b/third_party/closure_compiler/compiler_test.py
similarity index 66%
rename from third_party/closure_compiler/compiler_customization_test.py
rename to third_party/closure_compiler/compiler_test.py
index 9873c8fe6c9018b8b19649ae7183a4f432108db9..effd68a8c728748c96d832d26f3405649ddc1016 100755
--- a/third_party/closure_compiler/compiler_customization_test.py
+++ b/third_party/closure_compiler/compiler_test.py
@@ -21,7 +21,7 @@ _POLYMER_EXTERNS = os.path.join(_SRC_DIR, "third_party", "polymer", "v0_8",
                                 "polymer.externs.js")
 
 
-class CompilerCustomizationTest(unittest.TestCase):
+class CompilerTest(unittest.TestCase):
   _ASSERT_DEFINITION = Processor(_ASSERT_JS).contents
   _CR_DEFINE_DEFINITION = Processor(_CR_JS).contents
   _CR_UI_DECORATE_DEFINITION = Processor(_CR_UI_JS).contents
@@ -29,24 +29,57 @@ class CompilerCustomizationTest(unittest.TestCase):
   def setUp(self):
     self._checker = Checker()
 

[Dan Beam, 2015/05/19 18:23:33]

def tearDown(self):
  # remove all files regardless of success

[Theresa, 2015/05/19 19:21:02]

Done.

-  def _runChecker(self, source_code):
+  def _runChecker(self, source_code, output_wrapper=None):
     file_path = "/script.js"
     FileCache._cache[file_path] = source_code
-    return self._checker.check(file_path, externs=[_POLYMER_EXTERNS])
+    out_file, out_map = self._createOutFiles()
+
+    found_errors, stderr = self._checker.check(file_path,
+                                               externs=[_POLYMER_EXTERNS],
+                                               out_file=out_file,
+                                               output_wrapper=output_wrapper)
+    return found_errors, stderr, out_file, out_map
 
   def _runCheckerTestExpectError(self, source_code, expected_error):
-    _, stderr = self._runChecker(source_code)
+    _, stderr, out_file, out_map = self._runChecker(source_code)
 
     self.assertTrue(expected_error in stderr,
         msg="Expected chunk: \n%s\n\nOutput:\n%s\n" % (
             expected_error, stderr))
+    self.assertFalse(os.path.exists(out_file))
+    self.assertFalse(os.path.exists(out_map))
 
-  def _runCheckerTestExpectSuccess(self, source_code):
-    found_errors, stderr = self._runChecker(source_code)
+  def _runCheckerTestExpectSuccess(self, source_code, expected_output=None,
+                                   output_wrapper=None):
+    found_errors, stderr, out_file, out_map = self._runChecker(source_code,
+                                                               output_wrapper)
 
     self.assertFalse(found_errors,
         msg="Expected success, but got failure\n\nOutput:\n%s\n" % stderr)
 
+    self.assertTrue(os.path.exists(out_map))
+    self.assertTrue(os.path.exists(out_file))
+    if expected_output:
+      with open(out_file, 'r') as file:

[Dan Beam, 2015/05/19 18:23:33]

' -> " everywhere feasible

[Theresa, 2015/05/19 19:21:02]

Done.

+        self.assertEquals(file.read(), expected_output)
+
+    self._removeFiles([out_file, out_map])
+
+  def _createOutFiles(self):

[Dan Beam, 2015/05/19 18:23:32]

why do we need this?  shouldn't they be created by .check()?

[Theresa, 2015/05/19 18:29:23]

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.

[Theresa, 2015/05/19 19:21:02]

Done.

[Dan Beam, 2015/05/20 02:01:00]

why not move this to check() instead of main()?
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/closur...

[Dan Beam, 2015/05/20 02:05:33]

sorry, make a _ensure_out_file_exists() (or something) and call from check() and
check_multiple()?

[Theresa, 2015/05/20 02:07:37]

I moved it to _run_js_check() since it's not actually needed until the compiler
is run.

+    out_file = "/tmp/gen/script.js"
+    out_map = "/tmp/gen/script.js.map"
+
+    out_dir = os.path.dirname(out_file)
+    if not os.path.exists(out_dir):
+      os.makedirs(out_dir)
+
+    return out_file, out_map
+
+  def _removeFiles(self, files):
+    for file in files:
+      if(os.path.exists(file)):

[Dan Beam, 2015/05/19 18:23:33]

the python gods do not approve of your c-like syntax

  if os.path.exists(file):

[Theresa, 2015/05/19 19:21:02]

Done.

+        os.remove(file)
+
   def testGetInstance(self):
     self._runCheckerTestExpectError("""
 var cr = {
@@ -235,6 +268,52 @@ function f() {
 }
 """)
 
+  def testValidScriptCompilation(self):
+    self._runCheckerTestExpectSuccess("""
+var testScript = function() { console.log("hello world") };""",

[Dan Beam, 2015/05/19 18:23:33]

can you put some \n in here so the difference is less subtle (as well as the
code's easier to read)?

[Theresa, 2015/05/19 19:21:02]

Done.

+"""'use strict';var testScript=function(){console.log("hello world")};\n""")

[Dan Beam, 2015/05/19 18:23:33]

hmmm, why is 'use strict'; prepended?

[Theresa, 2015/05/19 18:29:23]

Because the closure compiler always prepends it? In all of my testing, I have
always seen the compiled file start with 'use strict'

[Theresa, 2015/05/19 19:21:01]

I'm going to guess it's from the _COMMON_CLOSURE_ARGS
"--language_in=ECMASCRIPT5_STRICT",

+
+  def testOutputWrapper(self):
+        self._runCheckerTestExpectSuccess("""

[Dan Beam, 2015/05/19 18:23:33]

indent off

[Theresa, 2015/05/19 19:21:01]

Done.

+var testScript = function() { console.log("hello world") };
+""", """(function(){'use strict';var testScript=function()"""
++ """{console.log("hello world")};})();\n""",
+output_wrapper="(function(){%output%})();")

[Dan Beam, 2015/05/19 18:23:33]

make this prettier

[Theresa, 2015/05/19 19:21:01]

Done.

+
+  def testCheckMultiple(self):
+    source_file = "/tmp/script.js"

[Dan Beam, 2015/05/19 18:23:32]

this should use tempfile instead
https://docs.python.org/2/library/tempfile.html

[Theresa, 2015/05/19 19:21:01]

Done.

+    with open(source_file, 'w') as f:
+      f.write("""

[Dan Beam, 2015/05/19 18:23:33]

why are we actually writing these files rather than injecting them into
FileCache._cache?

[Theresa, 2015/05/19 18:29:22]

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). 

[Theresa, 2015/05/19 19:21:01]

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.

+goog.require('testScript');
+
+testScript();
+""")
+
+    source_file2 = "/tmp/script2.js"
+    with open(source_file2, 'w') as f:
+      f.write("""
+goog.provide('testScript');
+
+var testScript = function(){};
+""")

[Dan Beam, 2015/05/19 18:23:33]

can you reverse the order of source_file/source_file2?  and maybe source_file ->
source_file1?

[Theresa, 2015/05/19 19:21:01]

Done.

+
+    out_file, out_map = self._createOutFiles()
+    sources = [source_file, source_file2]
+    externs = [_POLYMER_EXTERNS]
+    found_errors, stderr = self._checker.check_multiple(sources,
+                                                        externs=externs,
+                                                        out_file=out_file)
+    self.assertFalse(found_errors,
+        msg="Expected success, but got failure\n\nOutput:\n%s\n" % stderr)
+
+    expected_output = "'use strict';var testScript=function(){};testScript();\n"
+    self.assertTrue(os.path.exists(out_map))
+    self.assertTrue(os.path.exists(out_file))
+    with open(out_file, 'r') as file:
+      self.assertEquals(file.read(), expected_output)
+
+    self._removeFiles([source_file, source_file2, out_file, out_map])
+
 
 if __name__ == "__main__":
   unittest.main()