Config files

Config files

Skipped subdirectories are now read from .expect_tests.cfg files instead of from __init__.py files.

BUG=405428
R=dnj@chromium.org

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

[pgervais, 2014-09-19 00:29:17]

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

[pgervais, 2014-09-19 00:29:18]

Here's the final step before we can use expect_tests to test appengine apps in
infra.git. 

Here's the rationale: 
appengine applications are closed worlds, with the exception of the appengine
sdk, which should be accessible from sys.path. When testing several packages at
the same time, in separate processes, we should modify sys.path only when
necessary. Storing this information in a config file stored inside the relevant
repository is a simple way to achieve that.

Another solution would be to provide the same info from the command-line, but
I'm afraid that'll be hairy very fast.

wdyt?

[dnj, 2014-09-19 00:32:24]

On 2014/09/19 00:29:18, pgervais wrote:
> Here's the final step before we can use expect_tests to test appengine apps in
> infra.git. 
> 
> Here's the rationale: 
> appengine applications are closed worlds, with the exception of the appengine
> sdk, which should be accessible from sys.path. When testing several packages
at
> the same time, in separate processes, we should modify sys.path only when
> necessary. Storing this information in a config file stored inside the
relevant
> repository is a simple way to achieve that.
> 
> Another solution would be to provide the same info from the command-line, but
> I'm afraid that'll be hairy very fast.
> 
> wdyt?

Is there any reason not to just require developers to have the AppEngine SDK
installed on their system like other Python library dependencies? Maybe via
'infra' wheel? I don't think it's harmful to have it in 'sys.path' for
non-AppEngine programs; they just won't use it.

[pgervais, 2014-09-19 00:39:41]

On 2014/09/19 00:32:24, dnj wrote:
> On 2014/09/19 00:29:18, pgervais wrote:
> > Here's the final step before we can use expect_tests to test appengine apps
in
> > infra.git. 
> > 
> > Here's the rationale: 
> > appengine applications are closed worlds, with the exception of the
appengine
> > sdk, which should be accessible from sys.path. When testing several packages
> at
> > the same time, in separate processes, we should modify sys.path only when
> > necessary. Storing this information in a config file stored inside the
> relevant
> > repository is a simple way to achieve that.
> > 
> > Another solution would be to provide the same info from the command-line,
but
> > I'm afraid that'll be hairy very fast.
> > 
> > wdyt?
> 
> Is there any reason not to just require developers to have the AppEngine SDK
> installed on their system like other Python library dependencies? Maybe via
> 'infra' wheel? I don't think it's harmful to have it in 'sys.path' for
> non-AppEngine programs; they just won't use it.

infra.git is supposed to be self-contained, and already checks out the appengine
sdk using runhooks. We'll also have to come up with a convenient way to launch
the development server using this exact sdk anyway. So we could as well do that
for testing.

As for having google_appengine always in the path, it's probably harmless, but
who knows...

[pgervais, 2014-09-19 21:14:42]

Uploaded new patchset, PTAL

[dnj, 2014-09-19 21:15:59]

https://codereview.chromium.org/587493002/diff/20001/expect_tests/pipeline.py
File expect_tests/pipeline.py (right):

https://codereview.chromium.org/587493002/diff/20001/expect_tests/pipeline.py...
expect_tests/pipeline.py:77: black_list_filename = os.path.join(path,
CONFIG_FILE_NAME)
If you're generalizing this to config file, the variable sholud be
"config_filename" or thereabouts.

[pgervais, 2014-09-19 22:06:26]

https://codereview.chromium.org/587493002/diff/20001/expect_tests/pipeline.py
File expect_tests/pipeline.py (right):

https://codereview.chromium.org/587493002/diff/20001/expect_tests/pipeline.py...
expect_tests/pipeline.py:77: black_list_filename = os.path.join(path,
CONFIG_FILE_NAME)
On 2014/09/19 21:15:59, dnj wrote:
> If you're generalizing this to config file, the variable sholud be
> "config_filename" or thereabouts.

Done.

[dnj, 2014-09-19 22:21:04]

I think it's a solid direction. I'm somewhat concerned about the idea of adding
new paths in general during testing. It seems to me that it undermines the point
of an "all in one" repository like Infra. I feel like anything that is needed 
should be accessible from the standard environment.

Is there a specific use case that you're addressing here?

Either way I like generalizing the blacklist into a config file. Just not sure
adding arbitrary paths should be part of that config.

[pgervais, 2014-09-19 22:49:44]

On 2014/09/19 22:21:04, dnj wrote:
> I think it's a solid direction. I'm somewhat concerned about the idea of
adding
> new paths in general during testing. It seems to me that it undermines the
point
> of an "all in one" repository like Infra. I feel like anything that is needed 
> should be accessible from the standard environment.
> 
> Is there a specific use case that you're addressing here?
> 
The single use case I can think of is testing appengine in infra.git. I'm not
expecting anybody using it beside that. Note that we're staying inside the 'all
in one' repo, we're just trying to accommodate conflicting requests: the
appengine sdk wants to be installed system-wide, we want a local install.

> Either way I like generalizing the blacklist into a config file. Just not sure
> adding arbitrary paths should be part of that config.

Since there is a single use case, it might be better to create an
appengine-specific system. But I don't think it's either feasible nor a good
thing in general.

[dnj, 2014-09-19 23:07:04]

> The single use case I can think of is testing appengine in infra.git. I'm not
> expecting anybody using it beside that. Note that we're staying inside the
'all
> in one' repo, we're just trying to accommodate conflicting requests: the
> appengine sdk wants to be installed system-wide, we want a local install.

Since 'infra.git' installs an AppEngine SDK wheel anyway, that should be what
the test runner uses.

I'm worried that once infra floodgates open, we'll get all kinds of wonky custom
path stuff. In this case, not having this feature actually helps enforce the
good behavior and continuity of the repository.

> Since there is a single use case, it might be better to create an
> appengine-specific system. But I don't think it's either feasible nor a good
> thing in general.

Yeah, if AppEngine can be accommodated within the current system, I'm all for
it. Does it not work without this change?

[pgervais, 2014-09-19 23:46:00]

On 2014/09/19 23:07:04, dnj wrote:
> > The single use case I can think of is testing appengine in infra.git. I'm
not
> > expecting anybody using it beside that. Note that we're staying inside the
> 'all
> > in one' repo, we're just trying to accommodate conflicting requests: the
> > appengine sdk wants to be installed system-wide, we want a local install.
> 
> Since 'infra.git' installs an AppEngine SDK wheel anyway, that should be what
> the test runner uses.
> 
> I'm worried that once infra floodgates open, we'll get all kinds of wonky
custom
> path stuff. In this case, not having this feature actually helps enforce the
> good behavior and continuity of the repository.
> 
> > Since there is a single use case, it might be better to create an
> > appengine-specific system. But I don't think it's either feasible nor a good
> > thing in general.
> 
> Yeah, if AppEngine can be accommodated within the current system, I'm all for
> it. Does it not work without this change?

Summary of offline discussion: if we relax the constraint that appengine is only
on the path for appengine app, then it is enough to add it to PYTHONPATH before
calling the expect_tests. Then modifying sys.path dynamically is not necessary
anymore, which is better.

Thus dropping pythonpath support, but keeping the switch to config files.

Please ignore patchset 4, rebasing mistake.

[dnj, 2014-09-19 23:56:06]

LGTM w/ comment, +nit!

https://codereview.chromium.org/587493002/diff/100001/expect_tests/pipeline.py
File expect_tests/pipeline.py (right):

https://codereview.chromium.org/587493002/diff/100001/expect_tests/pipeline.p...
expect_tests/pipeline.py:90: black_list = parser.get('expect_tests',
'skip').splitlines()
This should be 'black_list.update(...)'. Function says it returns a set, plus
otherwise you're creating and then discarding the default empty set.

https://codereview.chromium.org/587493002/diff/100001/expect_tests/pipeline.p...
expect_tests/pipeline.py:236: for modname in test_modules:
nit: Why split this? I think it was fine before.

[pgervais, 2014-09-20 00:26:55]

Fixed nits and updated commit message. Will commit soon.

https://codereview.chromium.org/587493002/diff/100001/expect_tests/pipeline.py
File expect_tests/pipeline.py (right):

https://codereview.chromium.org/587493002/diff/100001/expect_tests/pipeline.p...
expect_tests/pipeline.py:236: for modname in test_modules:
On 2014/09/19 23:56:05, dnj wrote:
> nit: Why split this? I think it was fine before.

merge problem I think. will fix.

[pgervais, 2014-09-20 00:30:06]

https://codereview.chromium.org/587493002/diff/120001/expect_tests/pipeline.py
File expect_tests/pipeline.py (right):

https://codereview.chromium.org/587493002/diff/120001/expect_tests/pipeline.p...
expect_tests/pipeline.py:163: test_modules += ['.'.join(base_module_name
This should be .extend(), sigh.

[pgervais, 2014-09-20 00:34:00]

Committed patchset #9 (id:160001) manually as f2e31f4 (presubmit successful).