Issue 888043002: Use a python script to configure unit tests and app tests to run.

Issue 888043002: Use a python script to configure unit tests and app tests to run. (Closed)

Created:
5 years, 10 months ago by qsr

Modified:
5 years, 10 months ago

Reviewers:
viettrungluu

CC:
mojo-reviews_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin (slow to review), ben+mojo_chromium.org

Base URL:
https://github.com/domokit/mojo.git@master

Target Ref:
refs/heads/master

Project:
mojo

Visibility:
Public.

More Reviews

Description

Use a python script to configure unit tests and app tests to run. R=viettrungluu@chromium.org Committed: https://chromium.googlesource.com/external/mojo/+/75b30343e6c8ea18f40f9792a044ac24c17c2317

Patch Set 3 : Follow review #

Created: 5 years, 10 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+83 lines, -93 lines)			Patch
M	mojo/tools/apptest_runner.py	View		2 chunks	+7 lines, -7 lines	0 comments	Download
M	mojo/tools/data/apptests	View	1 2	2 chunks	+39 lines, -27 lines	0 comments	Download
M	mojo/tools/data/unittests	View	1 2	2 chunks	+30 lines, -29 lines	0 comments	Download
M	mojo/tools/mopy/config.py	View		1 chunk	+0 lines, -22 lines	0 comments	Download
M	mojo/tools/test_runner.py	View		2 chunks	+7 lines, -8 lines	0 comments	Download

Messages

Total messages: 7 (0 generated)

Expand Messages | Collapse Messages

viettrungluu

https://codereview.chromium.org/888043002/diff/1/mojo/tools/data/apptests File mojo/tools/data/apptests (right): https://codereview.chromium.org/888043002/diff/1/mojo/tools/data/apptests#newcode30 mojo/tools/data/apptests:30: if config.target_os == Config.OS_LINUX: You should be able to ...

5 years, 10 months ago (2015-01-30 20:43:27 UTC) #2

qsr

5 years, 10 months ago (2015-02-03 10:44:25 UTC) #3

viettrungluu

On 2015/02/03 10:44:25, qsr wrote: > https://codereview.chromium.org/888043002/diff/1/mojo/tools/data/apptests > File mojo/tools/data/apptests (right): > > https://codereview.chromium.org/888043002/diff/1/mojo/tools/data/apptests#newcode30 > ...

5 years, 10 months ago (2015-02-03 16:41:23 UTC) #4

qsr

> a) Since the executor provides config, it's silly for it to appear as if ...

5 years, 10 months ago (2015-02-04 15:03:20 UTC) #5

> a) Since the executor provides config, it's silly for it to appear as if it
> doesn't provide everything required to make sense of it. (They're class
> variables, rather than globals, for this reason.)
> b) We really don't want to encourage people to do arbitrary things in these
> files (even though they're script, and they can); imports lead down this
> slippery slope. (I hate slippery slope arguments, but anyway.) Basically, the
> files should be self-contained, and only rely on the execution environment
> that's set up for them.
> 

 Ok, done. But I'm not really convince. If we change this to be a python script,
we can expect that someone someday will start using the full length of rope we
just enabled.

> > 
> >
>
https://codereview.chromium.org/888043002/diff/1/mojo/tools/data/apptests#new...
> > mojo/tools/data/apptests:31: tests+= [
> > On 2015/01/30 20:43:27, viettrungluu wrote:
> > > nit: space before "+="
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/888043002/diff/1/mojo/tools/data/apptests#new...
> > mojo/tools/data/apptests:36: "shell-args":
> > ["--url-mappings=mojo:example_service=mojo:python_example_service"],
> > On 2015/01/30 20:43:27, viettrungluu wrote:
> > > Arguably, you should wrap to fit in 80 columns.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/888043002/diff/1/mojo/tools/data/apptests#new...
> > mojo/tools/data/apptests:41: tests+= [
> > On 2015/01/30 20:43:27, viettrungluu wrote:
> > > ...
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/888043002/diff/1/mojo/tools/data/apptests#new...
> > mojo/tools/data/apptests:49: "shell-args":
> > ["--url-mappings=mojo:example_service=test:example_service"],
> > On 2015/01/30 20:43:27, viettrungluu wrote:
> > > ...
> > 
> > Done.
> > 
> > https://codereview.chromium.org/888043002/diff/1/mojo/tools/data/unittests
> > File mojo/tools/data/unittests (right):
> > 
> >
>
https://codereview.chromium.org/888043002/diff/1/mojo/tools/data/unittests#ne...
> > mojo/tools/data/unittests:7: from mopy.config import Config
> > On 2015/01/30 20:43:27, viettrungluu wrote:
> > > ...
> > 
> > Same thing. Will change if you insist, but I think the import makes sense
> here.

viettrungluu

On 2015/02/04 15:03:20, qsr wrote: > > a) Since the executor provides config, it's silly ...

5 years, 10 months ago (2015-02-04 15:48:21 UTC) #6

On 2015/02/04 15:03:20, qsr wrote:
> > a) Since the executor provides config, it's silly for it to appear as if it
> > doesn't provide everything required to make sense of it. (They're class
> > variables, rather than globals, for this reason.)
> > b) We really don't want to encourage people to do arbitrary things in these
> > files (even though they're script, and they can); imports lead down this
> > slippery slope. (I hate slippery slope arguments, but anyway.) Basically,
the
> > files should be self-contained, and only rely on the execution environment
> > that's set up for them.
> > 
> 
>  Ok, done. But I'm not really convince. If we change this to be a python
script,
> we can expect that someone someday will start using the full length of rope we
> just enabled.

There's certainly that danger. I wonder how we can make it difficult for them to
do so....

Anyhow, LGTM.

> 
> > > 
> > >
> >
>
https://codereview.chromium.org/888043002/diff/1/mojo/tools/data/apptests#new...
> > > mojo/tools/data/apptests:31: tests+= [
> > > On 2015/01/30 20:43:27, viettrungluu wrote:
> > > > nit: space before "+="
> > > 
> > > Done.
> > > 
> > >
> >
>
https://codereview.chromium.org/888043002/diff/1/mojo/tools/data/apptests#new...
> > > mojo/tools/data/apptests:36: "shell-args":
> > > ["--url-mappings=mojo:example_service=mojo:python_example_service"],
> > > On 2015/01/30 20:43:27, viettrungluu wrote:
> > > > Arguably, you should wrap to fit in 80 columns.
> > > 
> > > Done.
> > > 
> > >
> >
>
https://codereview.chromium.org/888043002/diff/1/mojo/tools/data/apptests#new...
> > > mojo/tools/data/apptests:41: tests+= [
> > > On 2015/01/30 20:43:27, viettrungluu wrote:
> > > > ...
> > > 
> > > Done.
> > > 
> > >
> >
>
https://codereview.chromium.org/888043002/diff/1/mojo/tools/data/apptests#new...
> > > mojo/tools/data/apptests:49: "shell-args":
> > > ["--url-mappings=mojo:example_service=test:example_service"],
> > > On 2015/01/30 20:43:27, viettrungluu wrote:
> > > > ...
> > > 
> > > Done.
> > > 
> > > https://codereview.chromium.org/888043002/diff/1/mojo/tools/data/unittests
> > > File mojo/tools/data/unittests (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/888043002/diff/1/mojo/tools/data/unittests#ne...
> > > mojo/tools/data/unittests:7: from mopy.config import Config
> > > On 2015/01/30 20:43:27, viettrungluu wrote:
> > > > ...
> > > 
> > > Same thing. Will change if you insist, but I think the import makes sense
> > here.

qsr

5 years, 10 months ago (2015-02-04 15:54:59 UTC) #7

Message was sent while issue was closed.

Committed patchset #3 (id:40001) manually as
75b30343e6c8ea18f40f9792a044ac24c17c2317 (presubmit successful).

Expand Messages | Collapse Messages