Add support for disabling the preparser when testing modules

Add support for disabling the preparser when testing modules

R=adamk@chromium.org
BUG=
LOG=N

Committed: https://crrev.com/0d1687b9df3bbba2d9e24e36f238b395645b6a67
Cr-Commit-Position: refs/heads/master@{#36035}

[nickie, 2016-05-04 15:06:21]

Description was changed from

==========
Add support for disabling the preparser when testing modules

R=adamk@chromium.org
BUG=
LOG=N
==========

to

==========
Add support for disabling the preparser when testing modules

R=adamk@chromium.org
BUG=
LOG=N
==========

[nickie, 2016-05-04 15:07:37]

This has been discussed in comments here:
https://codereview.chromium.org/1723313002/

[mike3, 2016-05-04 15:38:28]

On 2016/05/04 15:07:37, nickie wrote:
> This has been discussed in comments here:
> https://codereview.chromium.org/1723313002/

This gets us part of the way there. The problem is that it degrades test
coverage. As written, this patch disables valid tests for the PreParser. Fixing
that will require forwarding this flag from `RunModuleParserSyncTest` through
`RunParserSyncTest`, `TestParserSync` and finally to `TestParserSyncWithFlags`.

I've also been working on a patch, and I've been thinking about a clean way of
doing this. What do you think about a new EnumSet for "test run" options? This
can replace the current `is_module` parameter and support values `kAsModule`
and `kWithPreParser`. What do you think?

If that sounds agreeable, I'd be happy to continue working on my branch and
submit something shortly.

[nickie, 2016-05-04 15:45:08]

On 2016/05/04 15:38:28, mike3 wrote:
> This gets us part of the way there. The problem is that it degrades test
> coverage. As written, this patch disables valid tests for the PreParser.
Fixing
> that will require forwarding this flag from `RunModuleParserSyncTest` through
> `RunParserSyncTest`, `TestParserSync` and finally to
`TestParserSyncWithFlags`.

Sorry, there must have been some confusion (possibly from my part).  I've
introduced a flag with a default of 'true'.  When the flag is on, the behaviour
is exactly the same as it was before.  There's nowhere (in the existing tests)
that the flag is set to 'false'.  How does this degrade test coverage?

[mike3, 2016-05-04 15:58:15]

On 2016/05/04 15:45:08, nickie wrote:
> On 2016/05/04 15:38:28, mike3 wrote:
> > This gets us part of the way there. The problem is that it degrades test
> > coverage. As written, this patch disables valid tests for the PreParser.
> Fixing
> > that will require forwarding this flag from `RunModuleParserSyncTest`
through
> > `RunParserSyncTest`, `TestParserSync` and finally to
> `TestParserSyncWithFlags`.
> 
> Sorry, there must have been some confusion (possibly from my part).  I've
> introduced a flag with a default of 'true'.  When the flag is on, the
behaviour
> is exactly the same as it was before.  There's nowhere (in the existing tests)
> that the flag is set to 'false'.  How does this degrade test coverage?

Oh! You are correct, code coverage remains the same with this patch applied.

The code is clear, and the confusion is mine. I misread the default value,
probably because of my bias for the solution (e.g. requiring explicit opt-in to
PreParsing). The usability concern is still present, but I suppose that this is
enough for future tests to extend the functions as I mentioned before.

My concerns about the boolean trap remain, but it is test code, after all. I
imagine you want to get back to solving real problems :P This looks good to me
(for what it's worth).

[adamk, 2016-05-04 16:43:40]

lgtm, and sorry for the trouble this caused for your branch (I was thinking
Mike's original solution was fine given the early state of module parsing
tests).

[nickie, 2016-05-04 16:51:50]

The CQ bit was checked by nikolaos@chromium.org

[commit-bot: I haz the power, 2016-05-04 16:51:56]

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

[commit-bot: I haz the power, 2016-05-04 17:22:00]

Description was changed from

==========
Add support for disabling the preparser when testing modules

R=adamk@chromium.org
BUG=
LOG=N
==========

to

==========
Add support for disabling the preparser when testing modules

R=adamk@chromium.org
BUG=
LOG=N
==========

[commit-bot: I haz the power, 2016-05-04 17:22:01]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-05-04 17:22:28]

Description was changed from

==========
Add support for disabling the preparser when testing modules

R=adamk@chromium.org
BUG=
LOG=N
==========

to

==========
Add support for disabling the preparser when testing modules

R=adamk@chromium.org
BUG=
LOG=N

Committed: https://crrev.com/0d1687b9df3bbba2d9e24e36f238b395645b6a67
Cr-Commit-Position: refs/heads/master@{#36035}
==========

[commit-bot: I haz the power, 2016-05-04 17:22:29]

Patchset 1 (id:??) landed as
https://crrev.com/0d1687b9df3bbba2d9e24e36f238b395645b6a67
Cr-Commit-Position: refs/heads/master@{#36035}