Refactored types to simplify pickling.

Refactored types to simplify pickling.

Removed any pickled function from objects sent the the result stage.
Replaced namedtuple base class by custom class to simplify pickling.

BUG=405428
R=dnj@chromium.org

Committed: https://chromium.googlesource.com/infra/testing/expect_tests/+/fa814a8d03cea99a500bfcbc39c03197912a01ea

[pgervais, 2014-09-16 21:26:16]

pgervais@chromium.org changed reviewers:
+ dnj@chromium.org, iannucci@chromium.org

[pgervais, 2014-09-16 21:26:16]

This is a followup on https://codereview.chromium.org/560173002/. I didn't
realize that test results also contained pickled functions.

I reworked the class hierarchy to make things easier to pickle. Having
namedtuple as a base class does not work out really well.

[dnj, 2014-09-16 23:13:26]

Thoughts on trashing the "*Info" classes in general and just overriding Test's
and MultiTest's "__getstate__" and "__setstate__" methods? This would let you
pickle and unpickle them directly into instances. Since the full object instance
setup doesn't have any overhead, this seems like a cleaner and more
straightforward option.

https://docs.python.org/2/library/pickle.html#object.__getstate__

https://codereview.chromium.org/554213004/diff/1/expect_tests/type_definition...
File expect_tests/type_definitions.py (right):

https://codereview.chromium.org/554213004/diff/1/expect_tests/type_definition...
expect_tests/type_definitions.py:153: TEST_COVERS_MATCH =
re.compile('.*/test/([^/]*)_test\.py$')
I know this is kind of outside of the immediate scope of the CL, but this would
match "foo/test/_test.py". I think the "*" should be a "+".

https://codereview.chromium.org/554213004/diff/1/expect_tests/type_definition...
expect_tests/type_definitions.py:192: match =
Test.TEST_COVERS_MATCH.match(test_file)
Make this a "classmethod" and use "cls.TEST_COVERS_MATCH".

[pgervais, 2014-09-16 23:22:07]

On 2014/09/16 23:13:26, dnj wrote:
> Thoughts on trashing the "*Info" classes in general and just overriding Test's
> and MultiTest's "__getstate__" and "__setstate__" methods? This would let you
> pickle and unpickle them directly into instances. Since the full object
instance
> setup doesn't have any overhead, this seems like a cleaner and more
> straightforward option.

I agree but both TestInfo and Test classes are pickled (same for MultiTest). The
Test class is pickled between test listing and test running, whereas TestInfo is
pickled between test running and result printing.

I'll fix the other nits.

[pgervais, 2014-09-17 01:44:40]

On 2014/09/16 23:22:07, pgervais wrote:
> On 2014/09/16 23:13:26, dnj wrote:
> > Thoughts on trashing the "*Info" classes in general and just overriding
Test's
> > and MultiTest's "__getstate__" and "__setstate__" methods? This would let
you
> > pickle and unpickle them directly into instances. Since the full object
> instance
> > setup doesn't have any overhead, this seems like a cleaner and more
> > straightforward option.
> 
> I agree but both TestInfo and Test classes are pickled (same for MultiTest).
The
> Test class is pickled between test listing and test running, whereas TestInfo
is
> pickled between test running and result printing.
> 
> I'll fix the other nits.

New patchset uploaded. 

I've thought a bit more about your point, and I think getting rid of the Info
classes is indeed possible. Test.__init__ performs some operations that prevent
it from being instantiated without a func_call argument. Now that I know the
code better, I think skipping these operations went func_call is None is fine.

[pgervais, 2014-09-17 18:28:53]

On 2014/09/17 01:44:40, pgervais wrote:
> On 2014/09/16 23:22:07, pgervais wrote:
> > On 2014/09/16 23:13:26, dnj wrote:
> > > Thoughts on trashing the "*Info" classes in general and just overriding
> Test's
> > > and MultiTest's "__getstate__" and "__setstate__" methods? This would let
> you
> > > pickle and unpickle them directly into instances. Since the full object
> > instance
> > > setup doesn't have any overhead, this seems like a cleaner and more
> > > straightforward option.
> > 
> > I agree but both TestInfo and Test classes are pickled (same for MultiTest).
> The
> > Test class is pickled between test listing and test running, whereas
TestInfo
> is
> > pickled between test running and result printing.
> > 
> > I'll fix the other nits.
> 
> New patchset uploaded. 
> 
> I've thought a bit more about your point, and I think getting rid of the Info
> classes is indeed possible. Test.__init__ performs some operations that
prevent
> it from being instantiated without a func_call argument. Now that I know the
> code better, I think skipping these operations went func_call is None is fine.

Info classes have been removed. ptal

[dnj, 2014-09-17 20:10:52]

LGTM w/ comments/nits. I like the change!

https://codereview.chromium.org/554213004/diff/40001/expect_tests/type_defini...
File expect_tests/type_definitions.py (right):

https://codereview.chromium.org/554213004/diff/40001/expect_tests/type_defini...
expect_tests/type_definitions.py:189: self._breakpoints = breakpoints
self._breakpoints = copy.copy(breakpoints or [])
...
build onto 'self._breakpoints'?

https://codereview.chromium.org/554213004/diff/40001/expect_tests/type_defini...
expect_tests/type_definitions.py:193: breakpoints = copy.copy(breakpoints)
You create a 'breakpoints' copy and build onto it, but never retain it anywhere.
Is this supposed to be in 'self._breakpoints'?

Since this isn't a terribly difficult calculation, consider performing it in the
'breakpoints' property function, just b/c it's that much less to retain/pickle.

https://codereview.chromium.org/554213004/diff/40001/expect_tests/type_defini...
expect_tests/type_definitions.py:243: def bind(self, *args, **kwargs):
Consider implementing '__copy__' or a 'copy()' function. Then this can be:
t = self.copy()
t._func_call = self._func_call.bind(*args, **kwargs)
return t

Can also use this with 'get_info'.

https://codereview.chromium.org/554213004/diff/40001/expect_tests/type_defini...
expect_tests/type_definitions.py:292: return os.path.join(expect_dir, name +
('.%s' % (ext or self.ext)))
(ext or self.ext,) if following explicit tupling convention.

[pgervais, 2014-09-17 21:06:15]

https://codereview.chromium.org/554213004/diff/40001/expect_tests/type_defini...
File expect_tests/type_definitions.py (right):

https://codereview.chromium.org/554213004/diff/40001/expect_tests/type_defini...
expect_tests/type_definitions.py:243: def bind(self, *args, **kwargs):
On 2014/09/17 20:10:52, dnj wrote:
> Consider implementing '__copy__' or a 'copy()' function. Then this can be:
> t = self.copy()
> t._func_call = self._func_call.bind(*args, **kwargs)
> return t
> 
> Can also use this with 'get_info'.

I don't get the point. I think copy will be something like:

def copy(self):
   return Test(self._name,
        self._func_call,
        expect_dir=self.expect_dir,
        expect_base=self.expect_base,
        ext=self._ext,
        covers=self._covers,
        breakpoints=self._breakpoints)

So what's the gain?

[dnj, 2014-09-17 21:07:39]

On 2014/09/17 21:06:15, pgervais wrote:
>
https://codereview.chromium.org/554213004/diff/40001/expect_tests/type_defini...
> File expect_tests/type_definitions.py (right):
> 
>
https://codereview.chromium.org/554213004/diff/40001/expect_tests/type_defini...
> expect_tests/type_definitions.py:243: def bind(self, *args, **kwargs):
> On 2014/09/17 20:10:52, dnj wrote:
> > Consider implementing '__copy__' or a 'copy()' function. Then this can be:
> > t = self.copy()
> > t._func_call = self._func_call.bind(*args, **kwargs)
> > return t
> > 
> > Can also use this with 'get_info'.
> 
> I don't get the point. I think copy will be something like:
> 
> def copy(self):
>    return Test(self._name,
>         self._func_call,
>         expect_dir=self.expect_dir,
>         expect_base=self.expect_base,
>         ext=self._ext,
>         covers=self._covers,
>         breakpoints=self._breakpoints)
> 
> So what's the gain?

It provides a generic function that can be used in at least two places in the
existing code and makes the delta between derivative tests more obvious.

[pgervais, 2014-09-17 21:15:19]

Well, the solution you suggest is actually not equivalent to the current
code, because of the processing performed in __init__. We would have to
make sure __init__ is idempotent, which is a strong requirement. Otherwise,
copy() would do something close to a copy, but not exactly, and I think
that's a bad idea.

On Wed, Sep 17, 2014 at 2:07 PM, <dnj@chromium.org> wrote:

> On 2014/09/17 21:06:15, pgervais wrote:
>
> https://codereview.chromium.org/554213004/diff/40001/
> expect_tests/type_definitions.py
>
>> File expect_tests/type_definitions.py (right):
>>
>
>
> https://codereview.chromium.org/554213004/diff/40001/
> expect_tests/type_definitions.py#newcode243
>
>> expect_tests/type_definitions.py:243: def bind(self, *args, **kwargs):
>> On 2014/09/17 20:10:52, dnj wrote:
>> > Consider implementing '__copy__' or a 'copy()' function. Then this can
>> be:
>> > t = self.copy()
>> > t._func_call = self._func_call.bind(*args, **kwargs)
>> > return t
>> >
>> > Can also use this with 'get_info'.
>>
>
>  I don't get the point. I think copy will be something like:
>>
>
>  def copy(self):
>>     return Test(self._name,
>>          self._func_call,
>>          expect_dir=self.expect_dir,
>>          expect_base=self.expect_base,
>>          ext=self._ext,
>>          covers=self._covers,
>>          breakpoints=self._breakpoints)
>>
>
>  So what's the gain?
>>
>
> It provides a generic function that can be used in at least two places in
> the
> existing code and makes the delta between derivative tests more obvious.
>
> https://codereview.chromium.org/554213004/
>

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

[dnj, 2014-09-17 21:17:58]

On 2014/09/17 21:15:19, pgervais wrote:
> Well, the solution you suggest is actually not equivalent to the current
> code, because of the processing performed in __init__. We would have to
> make sure __init__ is idempotent, which is a strong requirement. Otherwise,
> copy() would do something close to a copy, but not exactly, and I think
> that's a bad idea.

It would if you took my other suggestion and moved the initialization into the
properties functions.

But yeah, this is a flimsy case. It was just a thought. If you don't like it,
that's fine.

[pgervais, 2014-09-17 21:26:29]

On Wed, Sep 17, 2014 at 2:17 PM, <dnj@chromium.org> wrote:

> On 2014/09/17 21:15:19, pgervais wrote:
>
>> Well, the solution you suggest is actually not equivalent to the current
>> code, because of the processing performed in __init__. We would have to
>> make sure __init__ is idempotent, which is a strong requirement.
>> Otherwise,
>> copy() would do something close to a copy, but not exactly, and I think
>> that's a bad idea.
>>
>
> It would if you took my other suggestion and moved the initialization into
> the
> properties functions.
>

I will take your other suggestions, because they solve real issues. My
point was more that I think it imposes a constraint (idempotency) on
__init__ that is non-obvious and overkill given the gain.

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

[pgervais, 2014-09-17 21:32:51]

Uploaded new patchset. ptal

https://codereview.chromium.org/554213004/diff/40001/expect_tests/type_defini...
File expect_tests/type_definitions.py (right):

https://codereview.chromium.org/554213004/diff/40001/expect_tests/type_defini...
expect_tests/type_definitions.py:193: breakpoints = copy.copy(breakpoints)
On 2014/09/17 20:10:52, dnj wrote:
> You create a 'breakpoints' copy and build onto it, but never retain it
anywhere.
> Is this supposed to be in 'self._breakpoints'?
> 
> Since this isn't a terribly difficult calculation, consider performing it in
the
> 'breakpoints' property function, just b/c it's that much less to
retain/pickle.

Done.

https://codereview.chromium.org/554213004/diff/40001/expect_tests/type_defini...
expect_tests/type_definitions.py:292: return os.path.join(expect_dir, name +
('.%s' % (ext or self.ext)))
On 2014/09/17 20:10:52, dnj wrote:
> (ext or self.ext,) if following explicit tupling convention.

Done.

[pgervais, 2014-09-17 23:19:23]

Committed patchset #4 (id:60001) manually as fa814a8 (presubmit successful).