Remove output redirect from corpus test runner.

Remove output redirect from corpus test runner.

The corpus tests redirect output to a string but they just output it
later anyway. This CL removes the output redirect and removes the
redirect_output flag which will always be False now.

Committed: https://pdfium.googlesource.com/pdfium/+/3b5cb78353f92ad7d23ace4e452ed26acf9aeaa7

[dsinclair, 2016-04-27 15:59:57]

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

[dsinclair, 2016-04-27 15:59:58]

PTAL.

This brings corpus tests closer to pixel/javascript tests when executed. Things
seem to be working correctly locally without the redirect.

[Tom Sepez, 2016-04-27 17:04:03]

On 2016/04/27 15:59:58, dsinclair wrote:
> PTAL.
> 
> This brings corpus tests closer to pixel/javascript tests when executed.
Things
> seem to be working correctly locally without the redirect.

How does this impact javascript tests?  I think this got originally added to
capture the app.alert() output and diff it against .txt files.  Can we still do
that?

[dsinclair, 2016-04-27 17:06:21]

On 2016/04/27 17:04:03, Tom Sepez wrote:
> On 2016/04/27 15:59:58, dsinclair wrote:
> > PTAL.
> > 
> > This brings corpus tests closer to pixel/javascript tests when executed.
> Things
> > seem to be working correctly locally without the redirect.
> 
> How does this impact javascript tests?  I think this got originally added to
> capture the app.alert() output and diff it against .txt files.  Can we still
do
> that?

This is the corpus test runner, the javascript test runner is different. Or, are
you saying the corpus directory has javascript files in it as well?

[dsinclair, 2016-04-27 17:07:54]

On 2016/04/27 17:06:21, dsinclair wrote:
> On 2016/04/27 17:04:03, Tom Sepez wrote:
> > On 2016/04/27 15:59:58, dsinclair wrote:
> > > PTAL.
> > > 
> > > This brings corpus tests closer to pixel/javascript tests when executed.
> > Things
> > > seem to be working correctly locally without the redirect.
> > 
> > How does this impact javascript tests?  I think this got originally added to
> > capture the app.alert() output and diff it against .txt files.  Can we still
> do
> > that?
> 
> This is the corpus test runner, the javascript test runner is different. Or,
are
> you saying the corpus directory has javascript files in it as well?


(The test_runner.py does a with open(txt_path) and then calls check_call
directly to output to the file which is how javascript tests work over there)

[Lei Zhang, 2016-04-27 20:51:32]

Description was changed from

==========
Remove output redirect from corpus test runner.

The corpus tests rediret output to a string but they just output it later
anyway. This CL removes the output redirect and removes the redirect_output
flag which will always be False now.
==========

to

==========
Remove output redirect from corpus test runner.

The corpus tests redirect output to a string but they just output it
later anyway. This CL removes the output redirect and removes the
redirect_output flag which will always be False now.
==========

[Lei Zhang, 2016-04-27 21:03:05]

The intention of capturing the output and outputting them afterwards is so
output from parallel pdfium_test binaries are not intermangled.

Currently, the run_corpus_tests output in parallel mode looks like:

Rendering PDF file foo.pdf.
Linearized path...
Rendered 2 pages.
Skipped 0 bad pages.
Checking foo.pdf.0.png
diff: 0.00% passed
Rendering PDF file bar.pdf.
Linearized path...
Rendered 2 pages.
Skipped 0 bad pages.
Checking bar.pdf.0.png
diff: 0.00% passed

With this patch, you'd likely get:

Rendering PDF file foo.pdf.
Linearized path...
Rendering PDF file bar.pdf.
Linearized path...
Rendered 2 pages.
Rendered 2 pages.
Skipped 0 bad pages.
Checking foo.pdf.0.png
diff: 0.00% passed
Skipped 0 bad pages.
Checking bar.pdf.0.png
diff: 0.00% passed

[dsinclair, 2016-04-27 21:05:49]

On 2016/04/27 21:03:05, Lei Zhang wrote:
> The intention of capturing the output and outputting them afterwards is so
> output from parallel pdfium_test binaries are not intermangled.
> 
> Currently, the run_corpus_tests output in parallel mode looks like:
> 
> Rendering PDF file foo.pdf.
> Linearized path...
> Rendered 2 pages.
> Skipped 0 bad pages.
> Checking foo.pdf.0.png
> diff: 0.00% passed
> Rendering PDF file bar.pdf.
> Linearized path...
> Rendered 2 pages.
> Skipped 0 bad pages.
> Checking bar.pdf.0.png
> diff: 0.00% passed
> 
> With this patch, you'd likely get:
> 
> Rendering PDF file foo.pdf.
> Linearized path...
> Rendering PDF file bar.pdf.
> Linearized path...
> Rendered 2 pages.
> Rendered 2 pages.
> Skipped 0 bad pages.
> Checking foo.pdf.0.png
> diff: 0.00% passed
> Skipped 0 bad pages.
> Checking bar.pdf.0.png
> diff: 0.00% passed


Do we care? Does anyone try to read the output from corpus tests? My usual
workflow is to run them all and then if anything fails run the individual test.
There is too much output to find the actual failure.

This change should make it easier to integrate corpus into the shared
test_runner as that test_runner does some output redirection already to handle
javascript tests.

[Lei Zhang, 2016-04-27 21:55:52]

On 2016/04/27 21:05:49, dsinclair wrote:
> Do we care? Does anyone try to read the output from corpus tests? My usual
> workflow is to run them all and then if anything fails run the individual
test.
> There is too much output to find the actual failure.

I cared enough to implement it. I suppose if this failed, even if the output is
readable, one can only gather so much information about what went wrong, and
likely need to re-run the test locally to debug.

> This change should make it easier to integrate corpus into the shared
> test_runner as that test_runner does some output redirection already to handle
> javascript tests.

Oh, are we duplicating functionality?

[dsinclair, 2016-04-27 23:30:42]

On 2016/04/27 21:55:52, Lei Zhang wrote:
> On 2016/04/27 21:05:49, dsinclair wrote:
> > Do we care? Does anyone try to read the output from corpus tests? My usual
> > workflow is to run them all and then if anything fails run the individual
> test.
> > There is too much output to find the actual failure.
> 
> I cared enough to implement it. I suppose if this failed, even if the output
is
> readable, one can only gather so much information about what went wrong, and
> likely need to re-run the test locally to debug.
> 
> > This change should make it easier to integrate corpus into the shared
> > test_runner as that test_runner does some output redirection already to
handle
> > javascript tests.
> 
> Oh, are we duplicating functionality?


We were triplicating, I merged the pixel runner with the javascript runner
earlier this week. I'd like to merge corpus into the other the other runner as
well. That would also add the multiprocessing to the other runners which maybe
useful in the future.

The main goal is to only have one place we need to change if we want to update
the test runners (things like Dr.Memory needed to be added 3 times originally).

[Lei Zhang, 2016-04-28 01:46:04]

lgtm

If this makes merging easier, then let's go for it. Maybe after we have a single
test runner, I can revisit this and improve the parallel testing output.

[dsinclair, 2016-04-28 13:17:19]

The CQ bit was checked by dsinclair@chromium.org

[commit-bot: I haz the power, 2016-04-28 13:17:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1927633002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1927633002/1

[commit-bot: I haz the power, 2016-04-28 13:17:44]

Description was changed from

==========
Remove output redirect from corpus test runner.

The corpus tests redirect output to a string but they just output it
later anyway. This CL removes the output redirect and removes the
redirect_output flag which will always be False now.
==========

to

==========
Remove output redirect from corpus test runner.

The corpus tests redirect output to a string but they just output it
later anyway. This CL removes the output redirect and removes the
redirect_output flag which will always be False now.

Committed:
https://pdfium.googlesource.com/pdfium/+/3b5cb78353f92ad7d23ace4e452ed26acf9a...
==========

[commit-bot: I haz the power, 2016-04-28 13:17:45]

Committed patchset #1 (id:1) as
https://pdfium.googlesource.com/pdfium/+/3b5cb78353f92ad7d23ace4e452ed26acf9a...