[Telemetry] discover.DiscoverClasses() does not add inherited classes.

[Telemetry] discover.DiscoverClasses() does not add inheritted classes.

If a PageAction inherits another PageAction class, then DiscoverClasses() 
might mess up the map from simple module name to class object name. See 
bug for more details.

BUG=281568
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=220649

[shadi, 2013-08-29 18:14:14]

PTAL

[tonyg, 2013-08-29 19:19:41]

https://codereview.chromium.org/23602015/diff/1/tools/telemetry/telemetry/cor...
File tools/telemetry/telemetry/core/discover.py (right):

https://codereview.chromium.org/23602015/diff/1/tools/telemetry/telemetry/cor...
tools/telemetry/telemetry/core/discover.py:69: issubclass(obj, base_class) and
obj.__module__ == module.__name__):
Can we add a case to discover_unittest.py?

[shadi, 2013-08-29 21:12:06]

PTAL.

With the current (unpatched) implementation of DiscoverClasses, this unittest
would fail.

[shadi, 2013-08-29 21:12:06]

PTAL.

With the current (unpatched) implementation of DiscoverClasses, this unittest
would fail.

[dtu, 2013-08-29 22:17:21]

lgtm, but Google Style says "Use imports for packages and modules only." The
actions/play module has MediaAction as a member only because it's importing that
class instead of the module.

Also, for an alternative/supplemental fix, discover should ignore classes that
have subclasses. In every case (in Telemetry, except for PageTest), those
classes exist to share code between subclasses, and are never meant to be used
by themselves.

[dtu, 2013-08-29 22:18:30]

On 2013/08/29 22:17:21, Dave Tu wrote:
> lgtm, but Google Style says "Use imports for packages and modules only." The
> actions/play module has MediaAction as a member only because it's importing
that
> class instead of the module.
> 
> Also, for an alternative/supplemental fix, discover should ignore classes that
> have subclasses. In every case (in Telemetry, except for PageTest), those
> classes exist to share code between subclasses, and are never meant to be used
> by themselves.

Sorry, to be clear, I mean that we should fix all three of these things :)

[shadi, 2013-08-29 22:48:30]

On 2013/08/29 22:18:30, Dave Tu wrote:
> On 2013/08/29 22:17:21, Dave Tu wrote:
> > lgtm, but Google Style says "Use imports for packages and modules only." The
> > actions/play module has MediaAction as a member only because it's importing
> that
> > class instead of the module.
> > 
> > Also, for an alternative/supplemental fix, discover should ignore classes
that
> > have subclasses. In every case (in Telemetry, except for PageTest), those
> > classes exist to share code between subclasses, and are never meant to be
used
> > by themselves.
> 
> Sorry, to be clear, I mean that we should fix all three of these things :)
To be clear, what is the third item? 
1- Fix the action classes to import modules instead of classes.
2- Fix discover to ignore classes that contain sub-classes, except for PageTest
one?

[dtu, 2013-08-29 22:52:10]

On 2013/08/29 22:48:30, shadi wrote:
> On 2013/08/29 22:18:30, Dave Tu wrote:
> > On 2013/08/29 22:17:21, Dave Tu wrote:
> > > lgtm, but Google Style says "Use imports for packages and modules only."
The
> > > actions/play module has MediaAction as a member only because it's
importing
> > that
> > > class instead of the module.
> > > 
> > > Also, for an alternative/supplemental fix, discover should ignore classes
> that
> > > have subclasses. In every case (in Telemetry, except for PageTest), those
> > > classes exist to share code between subclasses, and are never meant to be
> used
> > > by themselves.
> > 
> > Sorry, to be clear, I mean that we should fix all three of these things :)
> To be clear, what is the third item? 
> 1- Fix the action classes to import modules instead of classes.
> 2- Fix discover to ignore classes that contain sub-classes, except for
PageTest
> one?

This patch as-is :p

[shadi, 2013-08-29 23:28:56]

Thanks for the comments!

I will postpone `1- Fix the action classes to import modules instead of
classes.` to a separate clean up CL.

As for ignoring base classes, at the moment DiscoverClasses() is used for:
profiler.Profiler
profile_creator.ProfileCreator
page_test_module.PageTest
test.Test
page_measurement.PageMeasurement
page_action.PageAction

Can you confirm that all these except PageTest class are not meant to be used as
is? That is only their sub-classes are used?

[shadi, 2013-08-29 23:29:46]

and PTAL :)


On Thu, Aug 29, 2013 at 4:28 PM, <shadi@chromium.org> wrote:

> Thanks for the comments!
>
> I will postpone `1- Fix the action classes to import modules instead of
> classes.` to a separate clean up CL.
>
> As for ignoring base classes, at the moment DiscoverClasses() is used for:
> profiler.Profiler
> profile_creator.ProfileCreator
> page_test_module.PageTest
> test.Test
> page_measurement.**PageMeasurement
> page_action.PageAction
>
> Can you confirm that all these except PageTest class are not meant to be
> used as
> is? That is only their sub-classes are used?
>
>
>
>
https://codereview.chromium.**org/23602015/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dtu, 2013-08-30 00:35:21]

On 2013/08/29 23:29:46, shadi wrote:
> and PTAL :)
> 
> 
> On Thu, Aug 29, 2013 at 4:28 PM, <mailto:shadi@chromium.org> wrote:
> 
> > Thanks for the comments!
> >
> > I will postpone `1- Fix the action classes to import modules instead of
> > classes.` to a separate clean up CL.
> >
> > As for ignoring base classes, at the moment DiscoverClasses() is used for:
> > profiler.Profiler
> > profile_creator.ProfileCreator
> > page_test_module.PageTest
> > test.Test
> > page_measurement.**PageMeasurement
> > page_action.PageAction
> >
> > Can you confirm that all these except PageTest class are not meant to be
> > used as
> > is? That is only their sub-classes are used?
> >
> >
> >
> >
>
https://codereview.chromium.**org/23602015/%3Chttps://codereview.chromium.org...>
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

lgtm
Yep, actually PageTest and PageMeasurement are also intended to be subclassed; I
was mistaken. So it's all of them. You can remove the optional parameter if
you'd like.

[shadi, 2013-08-30 01:02:50]

I removed the parameter, we can add it if we see a use case that needs it.

Thanks for the fast review!

[commit-bot: I haz the power, 2013-08-30 16:31:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shadi@chromium.org/23602015/16002

[commit-bot: I haz the power, 2013-08-30 16:44:03]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-08-30 18:05:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shadi@chromium.org/23602015/33001

[commit-bot: I haz the power, 2013-08-30 20:51:08]

Change committed as 220649