Create run_javascript_tests.py

Create run_javascript_tests.py

Top level script driver for testing/resources/javascript.
Converts each input template file in that directory to a
.pdf file in the working directory (typically
out/Debug/gen/pdfium/testing/resources/javascript), invokes
pdfium_test on it to generate , and crudely diffs the
expected output.

We can now remove generated .pdfs from source control,
keeping only the hand-editable .in templates.

R=thestig@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/9f93baf8766c2505f9ad28ebfedb4f28ece7aa02

[Tom Sepez, 2015-02-10 21:12:08]

tsepez@chromium.org changed reviewers:
+ thestig@chromium.org

[Tom Sepez, 2015-02-10 21:25:08]

tsepez@chromium.org changed reviewers:
+ jam@chromium.org

[Tom Sepez, 2015-02-10 21:25:09]

Lei, please review (.py isn't my native language).
John, what else would you need to get this on the Bot?

[Lei Zhang, 2015-02-10 23:09:30]

https://codereview.chromium.org/912803004/diff/50001/testing/tools/fixup_pdf_...
File testing/tools/fixup_pdf_template.py (right):

https://codereview.chromium.org/912803004/diff/50001/testing/tools/fixup_pdf_...
testing/tools/fixup_pdf_template.py:73: def expand_file(input_filename,
output_dir):
Two blank lines between top-level definitions:
https://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Blank...

I admit to not always following this when I write Python code too.

https://codereview.chromium.org/912803004/diff/50001/testing/tools/fixup_pdf_...
testing/tools/fixup_pdf_template.py:74: (input_root, extension) =
os.path.splitext(input_filename)
BTW, if you don't need |extension|, you can just put _ in its place. You also
don't need the parenthesis.

https://codereview.chromium.org/912803004/diff/50001/testing/tools/run_javasc...
File testing/tools/run_javascript_tests.py (right):

https://codereview.chromium.org/912803004/diff/50001/testing/tools/run_javasc...
testing/tools/run_javascript_tests.py:2: # Copyright 2014 The PDFium Authors.
All rights reserved.
2015

https://codereview.chromium.org/912803004/diff/50001/testing/tools/run_javasc...
testing/tools/run_javascript_tests.py:16: expected_filename =
os.path.join(source_dir, input_filename + '_expected.txt')
over 80 chars

https://codereview.chromium.org/912803004/diff/50001/testing/tools/run_javasc...
testing/tools/run_javascript_tests.py:20:
subprocess.check_call(['testing/tools/fixup_pdf_template.py',
Not sure if this works on Windows.

You may want to instead load it as a Python module and call its main() or
something.

https://codereview.chromium.org/912803004/diff/50001/testing/tools/run_javasc...
testing/tools/run_javascript_tests.py:24: subprocess.check_call(['diff',
expected_filename, txt_filename])
Not sure if this works on Windows either.

https://codereview.chromium.org/912803004/diff/50001/testing/tools/run_javasc...
testing/tools/run_javascript_tests.py:32: parser.add_option('--source-dir',
It might be good to make this /path/to/chrome_checkout/src, and the base
everything else, including the build dir, off of it. Otherwise this script
assumes the CWD is /path/to/chrome_checkout/src, and that may not always be the
case. I think many buildbots have /path/to/chrome_checkout as their CWD.

https://codereview.chromium.org/912803004/diff/50001/testing/tools/run_javasc...
testing/tools/run_javascript_tests.py:35: bin_filename =
os.path.join(options.build_dir, 'pdfium_test')
|bin_filename| -> |pdfium_test_path| ?

I tend to think of |foo_filename| as just the file name and |foo_path| as the
relative/absolute path.

https://codereview.chromium.org/912803004/diff/50001/testing/tools/run_javasc...
testing/tools/run_javascript_tests.py:36: if (sys.platform.startswith('win')):
No need for the outer parenthesis here either. Style guide says "use sparingly"

https://codereview.chromium.org/912803004/diff/50001/testing/tools/run_javasc...
testing/tools/run_javascript_tests.py:36: if (sys.platform.startswith('win')):
Should there be a TODO here for Mac?

https://codereview.chromium.org/912803004/diff/50001/testing/tools/run_javasc...
testing/tools/run_javascript_tests.py:39: for arg in
os.listdir(options.source_dir):
Rename |arg| to |pdf_source| ?

[Tom Sepez, 2015-02-10 23:33:32]

https://codereview.chromium.org/912803004/diff/50001/testing/tools/fixup_pdf_...
File testing/tools/fixup_pdf_template.py (right):

https://codereview.chromium.org/912803004/diff/50001/testing/tools/fixup_pdf_...
testing/tools/fixup_pdf_template.py:73: def expand_file(input_filename,
output_dir):
On 2015/02/10 23:09:29, Lei Zhang wrote:
> Two blank lines between top-level definitions:
>
https://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Blank...
> 
> I admit to not always following this when I write Python code too.

Done.

https://codereview.chromium.org/912803004/diff/50001/testing/tools/fixup_pdf_...
testing/tools/fixup_pdf_template.py:74: (input_root, extension) =
os.path.splitext(input_filename)
On 2015/02/10 23:09:29, Lei Zhang wrote:
> BTW, if you don't need |extension|, you can just put _ in its place. You also
> don't need the parenthesis.

Done.

https://codereview.chromium.org/912803004/diff/50001/testing/tools/run_javasc...
File testing/tools/run_javascript_tests.py (right):

https://codereview.chromium.org/912803004/diff/50001/testing/tools/run_javasc...
testing/tools/run_javascript_tests.py:2: # Copyright 2014 The PDFium Authors.
All rights reserved.
On 2015/02/10 23:09:29, Lei Zhang wrote:
> 2015

Done.

https://codereview.chromium.org/912803004/diff/50001/testing/tools/run_javasc...
testing/tools/run_javascript_tests.py:16: expected_filename =
os.path.join(source_dir, input_filename + '_expected.txt')
On 2015/02/10 23:09:29, Lei Zhang wrote:
> over 80 chars

Done.

https://codereview.chromium.org/912803004/diff/50001/testing/tools/run_javasc...
testing/tools/run_javascript_tests.py:20:
subprocess.check_call(['testing/tools/fixup_pdf_template.py',
A codesearch shows a bunch of these in our existing code.  I'd prefer to leave
it at the moment, and come back when its windows time.

https://codereview.chromium.org/912803004/diff/50001/testing/tools/run_javasc...
testing/tools/run_javascript_tests.py:24: subprocess.check_call(['diff',
expected_filename, txt_filename])
On 2015/02/10 23:09:30, Lei Zhang wrote:
> Not sure if this works on Windows either.
Ditto.

https://codereview.chromium.org/912803004/diff/50001/testing/tools/run_javasc...
testing/tools/run_javascript_tests.py:32: parser.add_option('--source-dir',
Actually, in a standlone build, there isn't any /src directory, just a top-level
pdfium.  Which is why the relative paths work (I think)  in either case.

Turns out this is easy to setup, see 
https://code.google.com/p/chromium/codesearch#chromium/build/scripts/slave/re...

https://codereview.chromium.org/912803004/diff/50001/testing/tools/run_javasc...
testing/tools/run_javascript_tests.py:35: bin_filename =
os.path.join(options.build_dir, 'pdfium_test')
On 2015/02/10 23:09:29, Lei Zhang wrote:
> |bin_filename| -> |pdfium_test_path| ?
> 
> I tend to think of |foo_filename| as just the file name and |foo_path| as the
> relative/absolute path.

Done.

https://codereview.chromium.org/912803004/diff/50001/testing/tools/run_javasc...
testing/tools/run_javascript_tests.py:36: if (sys.platform.startswith('win')):
On 2015/02/10 23:09:29, Lei Zhang wrote:
> Should there be a TODO here for Mac?
Added.

https://codereview.chromium.org/912803004/diff/50001/testing/tools/run_javasc...
testing/tools/run_javascript_tests.py:36: if (sys.platform.startswith('win')):
On 2015/02/10 23:09:30, Lei Zhang wrote:
> No need for the outer parenthesis here either. Style guide says "use
sparingly"

Much better.

https://codereview.chromium.org/912803004/diff/50001/testing/tools/run_javasc...
testing/tools/run_javascript_tests.py:39: for arg in
os.listdir(options.source_dir):
On 2015/02/10 23:09:29, Lei Zhang wrote:
> Rename |arg| to |pdf_source| ?

Done.

[Lei Zhang, 2015-02-11 00:03:21]

https://codereview.chromium.org/912803004/diff/50001/testing/tools/run_javasc...
File testing/tools/run_javascript_tests.py (right):

https://codereview.chromium.org/912803004/diff/50001/testing/tools/run_javasc...
testing/tools/run_javascript_tests.py:32: parser.add_option('--source-dir',
On 2015/02/10 23:33:31, Tom Sepez wrote:
> Actually, in a standlone build, there isn't any /src directory, just a
top-level
> pdfium.  Which is why the relative paths work (I think)  in either case.
> 
> Turns out this is easy to setup, see 
>
https://code.google.com/p/chromium/codesearch#chromium/build/scripts/slave/re...

Oh right, this is pdfium. In any case, I think it's not friendly for a tool to
only work when the CWD is set to a specific value. A better suggestion is to
start by taking os.path.realpath(__file__), assume it's the path to
testing/tools/run_javascript_tests.py in the checkout , use dirname() a few
times to figure out the base directory, and then using os.path.join() to build
paths relative to the base directory.

[Tom Sepez, 2015-02-11 00:52:18]

> Oh right, this is pdfium. In any case, I think it's not friendly for a tool to
> only work when the CWD is set to a specific value. A better suggestion is to
> start by taking os.path.realpath(__file__), assume it's the path to
> testing/tools/run_javascript_tests.py in the checkout , use dirname() a few
> times to figure out the base directory, and then using os.path.join() to build
> paths relative to the base directory.
Sounds like a plan.

[jam, 2015-02-11 17:03:59]

On 2015/02/10 21:25:09, Tom Sepez wrote:
> Lei, please review (.py isn't my native language).
> John, what else would you need to get this on the Bot?

once it's committed you can tell me what arguments you want passed to it (if
any) and I can send you a recipe update.

[Tom Sepez, 2015-02-11 17:22:41]

On 2015/02/11 17:03:59, jam wrote:
> On 2015/02/10 21:25:09, Tom Sepez wrote:
> > Lei, please review (.py isn't my native language).
> > John, what else would you need to get this on the Bot?
> 
> once it's committed you can tell me what arguments you want passed to it (if
> any) and I can send you a recipe update.
No args.

[Tom Sepez, 2015-02-11 20:01:28]

Lei, please re-review.  Thanks.

[Lei Zhang, 2015-02-11 22:45:34]

lgtm

https://codereview.chromium.org/912803004/diff/190001/testing/tools/run_javas...
File testing/tools/run_javascript_tests.py (right):

https://codereview.chromium.org/912803004/diff/190001/testing/tools/run_javas...
testing/tools/run_javascript_tests.py:36: parser.add_option('--build-dir',
default=os.path.join('out', 'Debug'))
Add a help message:

  parser.add_option('--build-dir', default=os.path.join('out', 'Debug'),
                    help="relative path from the base source directory")

https://codereview.chromium.org/912803004/diff/190001/testing/tools/run_javas...
testing/tools/run_javascript_tests.py:79: input_file_re =
re.compile('^[a-zA-Z0-9_.]+[.]in$')
Alternatively, you can use glob:

input_paths = glob.glob(os.path.join(source_dir, '*.in'))
input_paths.sort() # maybe? so tests run in a predictable pattern.
for input_path in input_paths:
  input_filename = os.path.basename(input_path)
  generate_and_test(...)

You may also want to check and make sure |input_path| is a file, or you can just
pass it in and wait for fixup_pdf_template.py to reject it.

[Tom Sepez, 2015-02-12 18:17:53]

New patchsets have been uploaded after l-g-t-m from thestig@chromium.org

[Tom Sepez, 2015-02-12 18:18:49]

https://codereview.chromium.org/912803004/diff/190001/testing/tools/run_javas...
File testing/tools/run_javascript_tests.py (right):

https://codereview.chromium.org/912803004/diff/190001/testing/tools/run_javas...
testing/tools/run_javascript_tests.py:36: parser.add_option('--build-dir',
default=os.path.join('out', 'Debug'))
On 2015/02/11 22:45:34, Lei Zhang wrote:
> Add a help message:
> 
>   parser.add_option('--build-dir', default=os.path.join('out', 'Debug'),
>                     help="relative path from the base source directory")

Done.

https://codereview.chromium.org/912803004/diff/190001/testing/tools/run_javas...
testing/tools/run_javascript_tests.py:79: input_file_re =
re.compile('^[a-zA-Z0-9_.]+[.]in$')
On 2015/02/11 22:45:34, Lei Zhang wrote:
> Alternatively, you can use glob:
> 
> input_paths = glob.glob(os.path.join(source_dir, '*.in'))
> input_paths.sort() # maybe? so tests run in a predictable pattern.
> for input_path in input_paths:
>   input_filename = os.path.basename(input_path)
>   generate_and_test(...)
> 
> You may also want to check and make sure |input_path| is a file, or you can
just
> pass it in and wait for fixup_pdf_template.py to reject it.

Done.

[Tom Sepez, 2015-02-12 18:19:58]

Committed patchset #12 (id:210001) manually as
9f93baf8766c2505f9ad28ebfedb4f28ece7aa02 (presubmit successful).

[Tom Sepez, 2015-02-12 18:20:31]

Ok, John, could you take a shot at seeing if this will run on the Bot?

[jam, 2015-02-13 17:14:38]

https://codereview.chromium.org/912803004/diff/210001/testing/tools/run_javas...
File testing/tools/run_javascript_tests.py (right):

https://codereview.chromium.org/912803004/diff/210001/testing/tools/run_javas...
testing/tools/run_javascript_tests.py:44: if os.path.basename(pdfium_dir) !=
'pdfium':
looks like this assumes that the checkout directory is called 'pdfium'. locally,
I don't have that and I wanted to test this out.

seems like no reason to require this?

[Tom Sepez, 2015-02-13 17:30:41]

On 2015/02/13 17:14:38, jam wrote:
>
https://codereview.chromium.org/912803004/diff/210001/testing/tools/run_javas...
> File testing/tools/run_javascript_tests.py (right):
> 
>
https://codereview.chromium.org/912803004/diff/210001/testing/tools/run_javas...
> testing/tools/run_javascript_tests.py:44: if os.path.basename(pdfium_dir) !=
> 'pdfium':
> looks like this assumes that the checkout directory is called 'pdfium'.
locally,
> I don't have that and I wanted to test this out.
> 
> seems like no reason to require this?
Argh, you're right, the top-level directory name isn't part of the repo and is
arbitrary.
I'll put together another CL to deal with this.  Thanks.