Simplify objects passed to the result stage

Simplify objects passed to the result stage

In the case where the run stage is skipped, Test and MultiTest objects
are trimmed down to avoid passing module objects. Those are tricky to
get through queues because the module must be importable at both ends.

BUG=405428
R=dnj@chromium.org, tandrii@chromium.org

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

[pgervais, 2014-09-10 22:45:39]

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

[pgervais, 2014-09-10 22:45:40]

This is a first step towards running tests in separate packages in separate
processes (one process in the run stage has only to import the module containing
the tests, nothing else). The final result stage is necessarily running in a
single process, so it should not import any module containing tests. This
prevents objects sent by the result queue to contain modules, because they have
to imported at the receiving end.

The present implementation is sketchy. In particular, I think the class names
could be improved.

[pgervais, 2014-09-10 23:39:03]

pgervais@chromium.org changed reviewers:
+ agable@chromium.org

[pgervais, 2014-09-10 23:39:03]

+agable

[tandrii(chromium), 2014-09-12 17:27:03]

tandrii@chromium.org changed reviewers:
+ tandrii@chromium.org

[tandrii(chromium), 2014-09-12 17:27:03]

LGTM

However, since you asked for cleaner alternatives, I've proposed something in a
comment. I'm not very happy about it either, yet I think it does improve
readability a bit.

Or, in current form, I'd just replace _Test with FakeTest (or my name TestInfo).

Of course, the same about MultiTest.

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

https://codereview.chromium.org/560173002/diff/1/expect_tests/type_definition...
expect_tests/type_definitions.py:156: pass
maybe, this:

class TestInfo(namedtuple('TestInfo', 'name expect_dir expect_base ext')):
  def __new__(cls, test):
    """Strips test instance of hard-to-pickle stuff"""
    super(TestInfo, cls).__new__(cls, test.name, test.expect_dir,
test.expect_base, test.ext)

then gen_stage_loop can be simpler:
  ...
  if isinstance(test, Test):
    test = TestInfo(test)
  ...

[dnj, 2014-09-12 17:35:37]

dnj@chromium.org changed reviewers:
+ dnj@chromium.org

[dnj, 2014-09-12 17:35:39]

https://codereview.chromium.org/560173002/diff/1/expect_tests/handle_list.py
File expect_tests/handle_list.py (right):

https://codereview.chromium.org/560173002/diff/1/expect_tests/handle_list.py#...
expect_tests/handle_list.py:5: from expect_tests.type_definitions import
Handler, Test, FakeTest, MultiTest
Since FakeTest is only generated/used here, it would make sense to move its
definition into this module.

https://codereview.chromium.org/560173002/diff/1/expect_tests/handle_list.py#...
expect_tests/handle_list.py:75: )
Maybe put a TypeError guard up in case someone creates another test type and
forgets to add it here. If you do this, make sure you pass 'FakeTest' instances
:)

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

https://codereview.chromium.org/560173002/diff/1/expect_tests/type_definition...
expect_tests/type_definitions.py:257: class FakeMultiTest(_MultiTest):
Is this used?

[pgervais, 2014-09-12 21:28:31]

I think I've found a satisfactory solution. ptal

https://codereview.chromium.org/560173002/diff/1/expect_tests/handle_list.py
File expect_tests/handle_list.py (right):

https://codereview.chromium.org/560173002/diff/1/expect_tests/handle_list.py#...
expect_tests/handle_list.py:5: from expect_tests.type_definitions import
Handler, Test, FakeTest, MultiTest
On 2014/09/12 17:35:37, dnj wrote:
> Since FakeTest is only generated/used here, it would make sense to move its
> definition into this module.

True, but I'd rather keep all definitions together, for convenience.

https://codereview.chromium.org/560173002/diff/1/expect_tests/handle_list.py#...
expect_tests/handle_list.py:75: )
On 2014/09/12 17:35:37, dnj wrote:
> Maybe put a TypeError guard up in case someone creates another test type and
> forgets to add it here. If you do this, make sure you pass 'FakeTest'
instances
> :)

Done.

[dnj, 2014-09-12 21:35:07]

https://codereview.chromium.org/560173002/diff/1/expect_tests/handle_list.py
File expect_tests/handle_list.py (right):

https://codereview.chromium.org/560173002/diff/1/expect_tests/handle_list.py#...
expect_tests/handle_list.py:5: from expect_tests.type_definitions import
Handler, Test, FakeTest, MultiTest
On 2014/09/12 21:28:30, pgervais wrote:
> On 2014/09/12 17:35:37, dnj wrote:
> > Since FakeTest is only generated/used here, it would make sense to move its
> > definition into this module.
> 
> True, but I'd rather keep all definitions together, for convenience.

I see your your point. As a counterpoint, I think putting it alongside the first
class test classes is confusing, implying that is useful in a larger context
when it is, in fact, constructed and utilized in only that one module.

Isolating it there gives it context and removes the ambiguity.

I'm fine either way; this just seems more intuitive to me.

[pgervais, 2014-09-12 21:52:17]

On 2014/09/12 21:35:07, dnj wrote:
> https://codereview.chromium.org/560173002/diff/1/expect_tests/handle_list.py
> File expect_tests/handle_list.py (right):
> 
>
https://codereview.chromium.org/560173002/diff/1/expect_tests/handle_list.py#...
> expect_tests/handle_list.py:5: from expect_tests.type_definitions import
> Handler, Test, FakeTest, MultiTest
> On 2014/09/12 21:28:30, pgervais wrote:
> > On 2014/09/12 17:35:37, dnj wrote:
> > > Since FakeTest is only generated/used here, it would make sense to move
its
> > > definition into this module.
> > 
> > True, but I'd rather keep all definitions together, for convenience.
> 
> I see your your point. As a counterpoint, I think putting it alongside the
first
> class test classes is confusing, implying that is useful in a larger context
> when it is, in fact, constructed and utilized in only that one module.
> 
> Isolating it there gives it context and removes the ambiguity.
> 
> I'm fine either way; this just seems more intuitive to me.

I solved the problem by creating factories instead, so there is no need to
import those classes anymore. New patch as been uploaded, could you take a look?

[dnj, 2014-09-12 22:00:14]

lgtm

[pgervais, 2014-09-12 23:12:11]

On 2014/09/12 22:00:14, dnj wrote:
> lgtm

Thanks. But this depends on https://codereview.chromium.org/556313004/. Could
you have a quick look?

[pgervais, 2014-09-12 23:25:41]

Committed patchset #3 (id:40001) manually as 5c273d9 (presubmit successful).