Issue 354913003: Add module discovery and autoloading to expect_tests.

Issue 354913003: Add module discovery and autoloading to expect_tests. (Closed)

Created:
6 years, 5 months ago by iannucci

Modified:
6 years, 5 months ago

Reviewers:
Vadim Sh., agable, pgervais

CC:
chromium-reviews, pgervais+watch_chromium.org, kjellander-cc_chromium.org, cmp-cc_chromium.org, ilevy-cc_chromium.org, stip+watch_chromium.org

Base URL:
svn://svn.chromium.org/chrome/trunk/tools/build

Project:
tools

Visibility:
Public.

More Reviews

Description

Add module discovery and autoloading to expect_tests. The boilerplate to invoke expect_tests is now just: expect_tests.main() You may specify modules to load explicitly, but by default it just scans the __main__ module. This also paves the way to have expect_tests be an executable package in its own right. If expect_tests itself is __main__, then it will scan for all tests under the current directory. Also adds yieldable Cleanup too so that generators can do some prepwork and get a callback on the completion of the entire test suite. R=agable@chromium.org, vadimsh@chromium.org BUG=

Patch Set 1 #

Patch Set 2 : add --force_coverage option #

Total comments: 15

Patch Set 3 : Single-line imports and rename EVERYTHING object #

Patch Set 4 : Fix formatting + comment #

Total comments: 1

Created: 6 years, 5 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+216 lines, -121 lines)			Patch
M	scripts/slave/unittests/expect_tests/__init__.py	View	1 2	1 chunk	+11 lines, -2 lines	0 comments	Download
M	scripts/slave/unittests/expect_tests/cover.py	View		2 chunks	+2 lines, -2 lines	0 comments	Download
M	scripts/slave/unittests/expect_tests/handle_test.py	View		1 chunk	+1 line, -1 line	0 comments	Download
M	scripts/slave/unittests/expect_tests/main.py	View	1 2 3	6 chunks	+67 lines, -23 lines	0 comments	Download
M	scripts/slave/unittests/expect_tests/pipeline.py	View	1 2	7 chunks	+55 lines, -43 lines	0 comments	Download
M	scripts/slave/unittests/expect_tests/serialize.py	View		1 chunk	+1 line, -1 line	0 comments	Download
M	scripts/slave/unittests/expect_tests/type_definitions.py	View	1 2	3 chunks	+16 lines, -8 lines	0 comments	Download
M	scripts/slave/unittests/expect_tests/unittest_helper.py	View	1 2	3 chunks	+48 lines, -35 lines	0 comments	Download
M	scripts/slave/unittests/expect_tests/util.py	View	1 2	2 chunks	+13 lines, -5 lines	1 comment	Download
M	scripts/slave/unittests/recipe_simulation_test.py	View	1 2	2 chunks	+2 lines, -1 line	0 comments	Download

Messages

Total messages: 7 (0 generated)

Expand Messages | Collapse Messages

Vadim Sh.

https://codereview.chromium.org/354913003/diff/20001/scripts/slave/unittests/expect_tests/main.py File scripts/slave/unittests/expect_tests/main.py (right): https://codereview.chromium.org/354913003/diff/20001/scripts/slave/unittests/expect_tests/main.py#newcode19 scripts/slave/unittests/expect_tests/main.py:19: EVERYTHING = object() It's a not very descriptive name. ...

6 years, 5 months ago (2014-06-27 21:40:08 UTC) #2

iannucci

PTAL https://chromiumcodereview.appspot.com/354913003/diff/20001/scripts/slave/unittests/expect_tests/main.py File scripts/slave/unittests/expect_tests/main.py (right): https://chromiumcodereview.appspot.com/354913003/diff/20001/scripts/slave/unittests/expect_tests/main.py#newcode19 scripts/slave/unittests/expect_tests/main.py:19: EVERYTHING = object() On 2014/06/27 21:40:08, Vadim Sh. ...

6 years, 5 months ago (2014-06-28 16:22:17 UTC) #3

pgervais

https://chromiumcodereview.appspot.com/354913003/diff/20001/scripts/slave/unittests/expect_tests/main.py File scripts/slave/unittests/expect_tests/main.py (right): https://chromiumcodereview.appspot.com/354913003/diff/20001/scripts/slave/unittests/expect_tests/main.py#newcode172 scripts/slave/unittests/expect_tests/main.py:172: def Gen(): On 2014/06/28 16:22:17, iannucci wrote: > On ...

6 years, 5 months ago (2014-06-30 13:56:03 UTC) #4

Vadim Sh.

https://codereview.chromium.org/354913003/diff/20001/scripts/slave/unittests/expect_tests/main.py File scripts/slave/unittests/expect_tests/main.py (right): https://codereview.chromium.org/354913003/diff/20001/scripts/slave/unittests/expect_tests/main.py#newcode172 scripts/slave/unittests/expect_tests/main.py:172: def Gen(): On 2014/06/30 13:56:03, pgervais wrote: > On ...

6 years, 5 months ago (2014-06-30 17:15:46 UTC) #5

https://codereview.chromium.org/354913003/diff/20001/scripts/slave/unittests/...
File scripts/slave/unittests/expect_tests/main.py (right):

https://codereview.chromium.org/354913003/diff/20001/scripts/slave/unittests/...
scripts/slave/unittests/expect_tests/main.py:172: def Gen():
On 2014/06/30 13:56:03, pgervais wrote:
> On 2014/06/28 16:22:17, iannucci wrote:
> > On 2014/06/27 21:40:08, Vadim Sh. wrote:
> > > You seemed to converge on convention to use CapitalCase for generators and
> > > lower_case_and_underscores for regular functions?.. I'm personally don't
> like
> > > this :) Just saying.
> > 
> > I think it's just inconsistent. The idea was CamelCase for (exported from
> > module) and snake_case for everything else.
> > 
> 
> Don't we have a style guide for this? I really don't like the added mental
load
> of knowing if the function is exported or before knowing how to write it. I
> consider this to be a mistake in Go, please don't get this into our Python
> codebase as well.
> 
> > I can rename. Keep in mind, this is inside a docstring, even though it
doesn't
> > look like it :p
> 

+1 to keeping single naming convention for public and private functions

https://codereview.chromium.org/354913003/diff/20001/scripts/slave/unittests/...
File scripts/slave/unittests/expect_tests/pipeline.py (right):

https://codereview.chromium.org/354913003/diff/20001/scripts/slave/unittests/...
scripts/slave/unittests/expect_tests/pipeline.py:239:
atexit.register(obj.func_call)
On 2014/06/28 16:22:17, iannucci wrote:
> On 2014/06/27 21:40:08, Vadim Sh. wrote:
> > em... so func_call is crossing process boundaries?
> 
> yep. I'm using this to delete the temporary directory (best effort) that the
> test generator created.
> 
> I'm wondering if 'temporary directory' should be a service that expect_tests
> just provides...

I hope it's clearly marked in docstring that cleanup function is executing in
another process? Since fork happens early, this clean up function may see stale
values of globals, etc. It may be confusing.

If it's only for temp dirs, then I personally prefer "service" approach.

https://codereview.chromium.org/354913003/diff/60001/scripts/slave/unittests/...
File scripts/slave/unittests/expect_tests/util.py (right):

https://codereview.chromium.org/354913003/diff/60001/scripts/slave/unittests/...
scripts/slave/unittests/expect_tests/util.py:7: EXPECT_TESTS_COVER_FUNCTION =
'EXPECT_TESTS_COVER_FUNCTION'
constant for this seems to be an overkill :) but as you prefer

iannucci

On 2014/06/30 17:15:46, Vadim Sh. wrote: > https://codereview.chromium.org/354913003/diff/20001/scripts/slave/unittests/expect_tests/main.py > File scripts/slave/unittests/expect_tests/main.py (right): > > https://codereview.chromium.org/354913003/diff/20001/scripts/slave/unittests/expect_tests/main.py#newcode172 ...

6 years, 5 months ago (2014-06-30 22:49:00 UTC) #6

On 2014/06/30 17:15:46, Vadim Sh. wrote:
>
https://codereview.chromium.org/354913003/diff/20001/scripts/slave/unittests/...
> File scripts/slave/unittests/expect_tests/main.py (right):
> 
>
https://codereview.chromium.org/354913003/diff/20001/scripts/slave/unittests/...
> scripts/slave/unittests/expect_tests/main.py:172: def Gen():
> On 2014/06/30 13:56:03, pgervais wrote:
> > On 2014/06/28 16:22:17, iannucci wrote:
> > > On 2014/06/27 21:40:08, Vadim Sh. wrote:
> > > > You seemed to converge on convention to use CapitalCase for generators
and
> > > > lower_case_and_underscores for regular functions?.. I'm personally don't
> > like
> > > > this :) Just saying.
> > > 
> > > I think it's just inconsistent. The idea was CamelCase for (exported from
> > > module) and snake_case for everything else.
> > > 
> > 
> > Don't we have a style guide for this? I really don't like the added mental
> load
> > of knowing if the function is exported or before knowing how to write it. I
> > consider this to be a mistake in Go, please don't get this into our Python
> > codebase as well.
> > 
> > > I can rename. Keep in mind, this is inside a docstring, even though it
> doesn't
> > > look like it :p
> > 
> 
> +1 to keeping single naming convention for public and private functions

Ok, so CamelCase for types, snake_case for everything else. Sorry about that.

> 
>
https://codereview.chromium.org/354913003/diff/20001/scripts/slave/unittests/...
> File scripts/slave/unittests/expect_tests/pipeline.py (right):
> 
>
https://codereview.chromium.org/354913003/diff/20001/scripts/slave/unittests/...
> scripts/slave/unittests/expect_tests/pipeline.py:239:
> atexit.register(obj.func_call)
> On 2014/06/28 16:22:17, iannucci wrote:
> > On 2014/06/27 21:40:08, Vadim Sh. wrote:
> > > em... so func_call is crossing process boundaries?
> > 
> > yep. I'm using this to delete the temporary directory (best effort) that the
> > test generator created.
> > 
> > I'm wondering if 'temporary directory' should be a service that expect_tests
> > just provides...
> 
> I hope it's clearly marked in docstring that cleanup function is executing in
> another process? Since fork happens early, this clean up function may see
stale
> values of globals, etc. It may be confusing.
> 
> If it's only for temp dirs, then I personally prefer "service" approach.
> 
>
https://codereview.chromium.org/354913003/diff/60001/scripts/slave/unittests/...
> File scripts/slave/unittests/expect_tests/util.py (right):
> 
>
https://codereview.chromium.org/354913003/diff/60001/scripts/slave/unittests/...
> scripts/slave/unittests/expect_tests/util.py:7: EXPECT_TESTS_COVER_FUNCTION =
> 'EXPECT_TESTS_COVER_FUNCTION'
> constant for this seems to be an overkill :) but as you prefer

This was the downstream fix. This is landed in the other cl:
https://chromiumcodereview.appspot.com/355143002/

iannucci

6 years, 5 months ago (2014-06-30 22:50:15 UTC) #7

On 2014/06/30 13:56:03, pgervais wrote:
>
https://chromiumcodereview.appspot.com/354913003/diff/20001/scripts/slave/uni...
> File scripts/slave/unittests/expect_tests/main.py (right):
> 
>
https://chromiumcodereview.appspot.com/354913003/diff/20001/scripts/slave/uni...
> scripts/slave/unittests/expect_tests/main.py:172: def Gen():
> On 2014/06/28 16:22:17, iannucci wrote:
> > On 2014/06/27 21:40:08, Vadim Sh. wrote:
> > > You seemed to converge on convention to use CapitalCase for generators and
> > > lower_case_and_underscores for regular functions?.. I'm personally don't
> like
> > > this :) Just saying.
> > 
> > I think it's just inconsistent. The idea was CamelCase for (exported from
> > module) and snake_case for everything else.
> > 
> 
> Don't we have a style guide for this? I really don't like the added mental
load
> of knowing if the function is exported or before knowing how to write it. I
> consider this to be a mistake in Go, please don't get this into our Python
> codebase as well.

I disagree for go, but only because 'gofix' exists and the compiler checks all
names at compile time. Renaming from lower->upper (or vice versa) is trivial.

Not the case for python.

> 
> > I can rename. Keep in mind, this is inside a docstring, even though it
doesn't
> > look like it :p

Expand Messages | Collapse Messages