Split ScriptStreamer and ScriptStreamerImpl, preparing for unit testing

Split ScriptStreamer and ScriptStreamerImpl, preparing for unit testing

In order to create a mock subclass of ScriptStreamer for unit testing of
PendingScript (e.g. in https://codereview.chromium.org/2653923008/).

This CL shouldn't change the behavior.

BUG=694702

[hiroshige, 2017-04-18 23:40:35]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-18 23:41:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2017-04-18 23:46:38]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-18 23:47:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-19 00:06:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-19 00:06:53]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[hiroshige, 2017-04-19 00:43:11]

Description was changed from

==========
Split ScriptStreamer and ScriptStreamerImpl, preparing for unit testing

BUG=
==========

to

==========
Split ScriptStreamer and ScriptStreamerImpl, preparing for unit testing

In order to create a mock subclass of ScriptStreamer for unit testing.

This CL shouldn't change the behavior.

BUG=694702
==========

[hiroshige, 2017-04-19 00:43:11]

hiroshige@chromium.org changed reviewers:
+ marja@chromium.org

[hiroshige, 2017-04-19 00:43:26]

hiroshige@chromium.org changed reviewers:
+ kouhei@chromium.org

[hiroshige, 2017-04-19 00:43:26]

PTAL.

[hiroshige, 2017-04-19 01:20:24]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-19 01:20:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-19 03:06:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-19 03:06:20]

Dry run: This issue passed the CQ dry run.

[haraken, 2017-04-19 07:39:28]

LGTM

[marja, 2017-04-19 08:03:40]

What kind of unit testing does this enable? Can you e.g., give an example of a
test that can be added now?

[marja, 2017-04-19 08:45:14]

marja@chromium.org changed reviewers:
+ vogelheim@chromium.org

[marja, 2017-04-19 08:45:14]

+ vogelheim who's working on the streaming now

[hiroshige, 2017-04-19 16:06:17]

On 2017/04/19 08:03:40, marja wrote:
> What kind of unit testing does this enable? Can you e.g., give an example of a
> test that can be added now?

ClassicPendingScriptTest.cpp in
https://codereview.chromium.org/2653923008/.
Currently it contains a single test case for catching a crash in prefinalizer,
but enabling mocking of ScriptStreamer would enable further unit testing around
PendingScript-ScriptStreamer interactions.

[hiroshige, 2017-04-19 16:07:07]

Description was changed from

==========
Split ScriptStreamer and ScriptStreamerImpl, preparing for unit testing

In order to create a mock subclass of ScriptStreamer for unit testing.

This CL shouldn't change the behavior.

BUG=694702
==========

to

==========
Split ScriptStreamer and ScriptStreamerImpl, preparing for unit testing

In order to create a mock subclass of ScriptStreamer for unit testing of
PendingScript (e.g. in https://codereview.chromium.org/2653923008/).

This CL shouldn't change the behavior.

BUG=694702
==========

[blink-reviews, 2017-04-19 16:20:05]

On Wed, Apr 19, 2017 at 6:06 PM, <hiroshige@chromium.org> wrote:

> On 2017/04/19 08:03:40, marja wrote:
> > What kind of unit testing does this enable? Can you e.g., give an
> example of a
> > test that can be added now?
>
> ClassicPendingScriptTest.cpp in
> https://codereview.chromium.org/2653923008/.
> Currently it contains a single test case for catching a crash in
> prefinalizer,
> but enabling mocking of ScriptStreamer would enable further unit testing
> around
> PendingScript-ScriptStreamer interactions.
>
> https://codereview.chromium.org/2830533002/
>

Ah, interesting.

Wouldn't it suffice for mocking to merely make the methods of interest
virtual?

(That said, I appreciate anything that makes this code more testable, and
certainly don't want to hold up your CLs.)

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2017-04-19 16:20:05]

On Wed, Apr 19, 2017 at 6:06 PM, <hiroshige@chromium.org> wrote:

> On 2017/04/19 08:03:40, marja wrote:
> > What kind of unit testing does this enable? Can you e.g., give an
> example of a
> > test that can be added now?
>
> ClassicPendingScriptTest.cpp in
> https://codereview.chromium.org/2653923008/.
> Currently it contains a single test case for catching a crash in
> prefinalizer,
> but enabling mocking of ScriptStreamer would enable further unit testing
> around
> PendingScript-ScriptStreamer interactions.
>
> https://codereview.chromium.org/2830533002/
>

Ah, interesting.

Wouldn't it suffice for mocking to merely make the methods of interest
virtual?

(That said, I appreciate anything that makes this code more testable, and
certainly don't want to hold up your CLs.)

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[hiroshige, 2017-04-19 16:43:53]

hiroshige@chromium.org changed reviewers:
+ japhet@chromium.org

[hiroshige, 2017-04-19 16:43:54]

> Wouldn't it suffice for mocking to merely make the methods of
> interest virtual?
Probably. Or I might be able to write the test with unmodified
ScriptStreamer if I do the similar thing in ScriptStreamerTest.cpp.
(ScriptStreamerTest.cpp looks testing PendingScript-ScriptStreamer interactions,
so perhaps it's better to put the test to ScriptStreamerTest.cpp)
I'll try.

[hiroshige, 2017-04-19 16:44:29]

On 2017/04/19 16:43:54, hiroshige wrote:
> > Wouldn't it suffice for mocking to merely make the methods of
> > interest virtual?
> Probably. Or I might be able to write the test with unmodified
> ScriptStreamer if I do the similar thing in ScriptStreamerTest.cpp.
> (ScriptStreamerTest.cpp looks testing PendingScript-ScriptStreamer
interactions,
> so perhaps it's better to put the test to ScriptStreamerTest.cpp)
> I'll try.

+japhet@ FYI if you're interested in unit testing around PendingScript.

[hiroshige, 2017-04-19 23:44:27]

On 2017/04/19 16:44:29, hiroshige wrote:
> On 2017/04/19 16:43:54, hiroshige wrote:
> > > Wouldn't it suffice for mocking to merely make the methods of
> > > interest virtual?
> > Probably. Or I might be able to write the test with unmodified
> > ScriptStreamer if I do the similar thing in ScriptStreamerTest.cpp.
> > (ScriptStreamerTest.cpp looks testing PendingScript-ScriptStreamer
> interactions,
> > so perhaps it's better to put the test to ScriptStreamerTest.cpp)
> > I'll try.
> 
> +japhet@ FYI if you're interested in unit testing around PendingScript.

How about https://codereview.chromium.org/2828973002/ (which doesn't change
ScriptStreamer)?
If that looks good, then I'll close this CL.

[blink-reviews, 2017-04-20 08:53:23]

On Thu, Apr 20, 2017 at 1:44 AM, <hiroshige@chromium.org> wrote:

> On 2017/04/19 16:44:29, hiroshige wrote:
> > On 2017/04/19 16:43:54, hiroshige wrote:
> > > > Wouldn't it suffice for mocking to merely make the methods of
> > > > interest virtual?
> > > Probably. Or I might be able to write the test with unmodified
> > > ScriptStreamer if I do the similar thing in ScriptStreamerTest.cpp.
> > > (ScriptStreamerTest.cpp looks testing PendingScript-ScriptStreamer
> > interactions,
> > > so perhaps it's better to put the test to ScriptStreamerTest.cpp)
> > > I'll try.
> >
> > +japhet@ FYI if you're interested in unit testing around PendingScript.
>
> How about https://codereview.chromium.org/2828973002/ (which doesn't
> change
> ScriptStreamer)?
> If that looks good, then I'll close this CL.
>

That does look very nice. :)

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2017-04-20 08:53:23]

On Thu, Apr 20, 2017 at 1:44 AM, <hiroshige@chromium.org> wrote:

> On 2017/04/19 16:44:29, hiroshige wrote:
> > On 2017/04/19 16:43:54, hiroshige wrote:
> > > > Wouldn't it suffice for mocking to merely make the methods of
> > > > interest virtual?
> > > Probably. Or I might be able to write the test with unmodified
> > > ScriptStreamer if I do the similar thing in ScriptStreamerTest.cpp.
> > > (ScriptStreamerTest.cpp looks testing PendingScript-ScriptStreamer
> > interactions,
> > > so perhaps it's better to put the test to ScriptStreamerTest.cpp)
> > > I'll try.
> >
> > +japhet@ FYI if you're interested in unit testing around PendingScript.
>
> How about https://codereview.chromium.org/2828973002/ (which doesn't
> change
> ScriptStreamer)?
> If that looks good, then I'll close this CL.
>

That does look very nice. :)

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.