New expect_tests UI

New expect_tests UI

BUG=420727
R=agable@chromium.org

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

[pgervais, 2014-11-11 22:53:47]

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

[pgervais, 2014-11-11 22:53:47]

The new UI for expect_tests is here!

With that code, you can run things like this:

expect_tests.py list <path/to/package>
expect_tests.py list <path/to/package/subpackage>
expect_tests.py list <path/to/package>:<filter>

and it'll work (<filter> is a glob on the full test name)

At the same time:
- added tests! They're meant to be run by nosetests, so as to avoid testing the
tool with itself. So no expectations, sorry.
- coverage is probably all broken. I'll fix that in another (shorter) CL.

The review of death. Sorry for the size...

[pgervais, 2014-11-11 22:54:36]

On 2014/11/11 22:53:47, pgervais wrote:
> The new UI for expect_tests is here!
> 
> With that code, you can run things like this:
> 
> expect_tests.py list <path/to/package>
> expect_tests.py list <path/to/package/subpackage>
> expect_tests.py list <path/to/package>:<filter>
> 
> and it'll work (<filter> is a glob on the full test name)
> 
> At the same time:
> - added tests! They're meant to be run by nosetests, so as to avoid testing
the
> tool with itself. So no expectations, sorry.
> - coverage is probably all broken. I'll fix that in another (shorter) CL.
> 
> The review of death. Sorry for the size...

I forgot: you can review the first patchset first, then review the difference
with the second one. I uploaded in two parts for this reason.

[agable, 2014-11-12 03:15:10]

First round comments. I think this mostly looks pretty good actually.

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

https://codereview.chromium.org/709853003/diff/20001/expect_tests/pipeline.py...
expect_tests/pipeline.py:77: # at the filesystem root ...
Love the French spacing between words and punctuation.

https://codereview.chromium.org/709853003/diff/20001/expect_tests/pipeline.py...
expect_tests/pipeline.py:197: if not os.path.exists(os.path.join(start_path)):
os.path.join not necessary

https://codereview.chromium.org/709853003/diff/20001/expect_tests/pipeline.py...
expect_tests/pipeline.py:249: os.path.join(testing_context.cwd,
testing_context.package_name)
Haven't read all the way down yet, but it seems like the subpath could be a
field on the TestingContext too.

https://codereview.chromium.org/709853003/diff/20001/expect_tests/pipeline.py...
expect_tests/pipeline.py:278: process.
Indent +4 for continuation? I dunno.

https://codereview.chromium.org/709853003/diff/20001/expect_tests/pipeline.py...
expect_tests/pipeline.py:279: test_queue (multiprocessing.Queue):
Love these args with no descriptions

https://codereview.chromium.org/709853003/diff/20001/expect_tests/pipeline.py...
expect_tests/pipeline.py:312: if kill_switch.is_set():
I know this is copied, but doesn't it make sense to check the kill switch first,
so you don't waste any effort?

https://codereview.chromium.org/709853003/diff/20001/expect_tests/pipeline.py...
expect_tests/pipeline.py:550: #   os.path.join(self.cwd, self.package_name)
where to look for tests for.
"in which to look for tests"

https://codereview.chromium.org/709853003/diff/20001/expect_tests/pipeline.py...
expect_tests/pipeline.py:557: Returns:
"Returns" isn't exactly accurate for a generator.

https://codereview.chromium.org/709853003/diff/20001/expect_tests/pipeline.py...
expect_tests/pipeline.py:567: ## matcher = re.compile(
Not sure what these commented-out segments are/were for.

https://codereview.chromium.org/709853003/diff/20001/expect_tests/pipeline.py...
expect_tests/pipeline.py:585: # list of (path, filter) pairs.
This comment is out of place. I think it belongs about 5 lines down above
filters =

https://codereview.chromium.org/709853003/diff/20001/expect_tests/pipeline.py...
expect_tests/pipeline.py:586: # The path is where to look for tests for. Only
tests whose name matches the
"where to look for tests for" again

https://codereview.chromium.org/709853003/diff/20001/expect_tests/pipeline.py...
expect_tests/pipeline.py:601: assert len(cwd) == 1, \
Not clear why here you use assert, while above and below you use 'if not ...:
raise'.

https://codereview.chromium.org/709853003/diff/20001/expect_tests/pipeline.py...
expect_tests/pipeline.py:602: 'from_context_list processes contexts with the
same working '\
See comment below; this should be something like 'from_context_list was given
process contexts containing the following cwds (...), but can only except
contexts which all share a single cwd.'

https://codereview.chromium.org/709853003/diff/20001/expect_tests/pipeline.py...
expect_tests/pipeline.py:630: raise ValueError('All package must have the same
value for "cwd"')
I don't think that ValueError is the right thing to raise here. I would be more
appropriate if the cwd could be said to be the 'value' of the TestingContext,
but I'm not sure that's intuitive. Whether you keep this or define your own
exception class, make the error message more helpful: state which package had
the wrong cwd, and what cwd it should have had instead.

Actually, the above comment applies to your exceptions and asserts throughout --
they should include helpful debugging information instead of just saying "this
went wrong but I won't tell you when or for what".

https://codereview.chromium.org/709853003/diff/20001/expect_tests/test/data/....
File expect_tests/test/data/.expect_tests.cfg (right):

https://codereview.chromium.org/709853003/diff/20001/expect_tests/test/data/....
expect_tests/test/data/.expect_tests.cfg:4: skip=directory1
Do you mean package1? directory1 and directory2 don't exist. But maybe you're
trying to skip things that don't exist? It's not clear, and it should be.

https://codereview.chromium.org/709853003/diff/20001/expect_tests/test/data/o...
File expect_tests/test/data/other/README (right):

https://codereview.chromium.org/709853003/diff/20001/expect_tests/test/data/o...
expect_tests/test/data/other/README:1: This directory should just contain
package, and should not be a package
Perhaps include more information here (or in test/data/README) about what these
files are actually for and why they shouldn't be touched.

An alternative approach would be to describe this filesystem in the tests, and
instantiate the whole filesystem in a temp directory at test time. That's what
the depot_tools tests do. On the one hand, it's pretty cool. On the other hand,
it means encoding the contents of all of these files inside the test files. And
that sucks a lot. So I think this is the better solution, but it's something to
consider.

https://codereview.chromium.org/709853003/diff/20001/expect_tests/test/data/p...
File expect_tests/test/data/package1/file1_test.py (right):

https://codereview.chromium.org/709853003/diff/20001/expect_tests/test/data/p...
expect_tests/test/data/package1/file1_test.py:1: import unittest
It's not clear why file1_test has an actual file1 which it is testing, but
file2_test doesn't.

I think because all of this test data is divorced from the tests that are using
it, it would be good to copiously comment these files so readers can easily
refer back into the tests that use them.

https://codereview.chromium.org/709853003/diff/20001/expect_tests/test/data/p...
File expect_tests/test/data/package1/file2_test.py (right):

https://codereview.chromium.org/709853003/diff/20001/expect_tests/test/data/p...
expect_tests/test/data/package1/file2_test.py:6: 
Trailing newline.

https://codereview.chromium.org/709853003/diff/20001/expect_tests/test/data/p...
File expect_tests/test/data/package1/subpackage1_1/subpackage1_1_1/file4_test.py
(right):

https://codereview.chromium.org/709853003/diff/20001/expect_tests/test/data/p...
expect_tests/test/data/package1/subpackage1_1/subpackage1_1_1/file4_test.py:6: 
Here too.

https://codereview.chromium.org/709853003/diff/20001/expect_tests/test/pipeli...
File expect_tests/test/pipeline_test.py (right):

https://codereview.chromium.org/709853003/diff/20001/expect_tests/test/pipeli...
expect_tests/test/pipeline_test.py:15: content for data/.expect_tests.cfg"""
nit: First line of docstring should be a single line.

https://codereview.chromium.org/709853003/diff/20001/expect_tests/test/pipeli...
expect_tests/test/pipeline_test.py:16: path = os.path.join(SCRIPT_DIR, 'data')
You already have DATA_DIR defined.

https://codereview.chromium.org/709853003/diff/20001/expect_tests/test/pipeli...
expect_tests/test/pipeline_test.py:41: def test_get_python_root(self):
I know this looks nice and compact, but this test tests four different
behaviors:
1) getting a dir with no init returns that dir
2) getting a package root returns its parent
3) getting a nested subpackage gets the parent of the root
4) getting a non-existent path raises an error
That really should be four different tests, so that different behaviors can be
modified independently. The same comment applies to the other tests below,
especially get_runtime_context. One test per behavior per function, not just one
test per function.

https://codereview.chromium.org/709853003/diff/20001/expect_tests/test/pipeli...
expect_tests/test/pipeline_test.py:53: datadir = os.path.join(SCRIPT_DIR,
'data')
DATA_DIR

https://codereview.chromium.org/709853003/diff/20001/expect_tests/test/pipeli...
expect_tests/test/pipeline_test.py:99: 
Please have a test for what happens if you explicitly list an ignored dir (e.g.
test_globs = [os.path.join(datadir, 'other')]).

[iannucci, 2014-11-12 20:26:25]

Also mostly lg. added comments :)

https://codereview.chromium.org/709853003/diff/1/expect_tests/pipeline.py
File expect_tests/pipeline.py (right):

https://codereview.chromium.org/709853003/diff/1/expect_tests/pipeline.py#new...
expect_tests/pipeline.py:314: for g in globs if g[0] == '-'))
I think neg_matcher got lost in the new world?

https://codereview.chromium.org/709853003/diff/1/expect_tests/pipeline.py#new...
expect_tests/pipeline.py:540: """A test glob is composed of a path and a glob
expression like:
do we still support the negative glob e.g. `-path/to/something:*Slow*`?

https://codereview.chromium.org/709853003/diff/1/expect_tests/pipeline.py#new...
expect_tests/pipeline.py:541: '<path>:<glob>'. The path should point to a
directory or a file inside
TODO: let's scan packages too so that <path> can also be a glob. Then
expect_tests can use a default of '*:*' when no tests are specified.

https://codereview.chromium.org/709853003/diff/1/expect_tests/pipeline.py#new...
expect_tests/pipeline.py:548: matching test1/*_test.py.
not strictly true, right? non-unittest tests would also match if they had a
similar name, correct?

TestA* would also be matching the unittest classname so a single test might be
`TestABunchOfStuff.testCatFoodDeployment`?

https://codereview.chromium.org/709853003/diff/1/expect_tests/pipeline.py#new...
expect_tests/pipeline.py:561: if '/' in test_filter:
Hm, I'm not sure this is strictly true... do we enforce this in Test.__init__?
There are use cases where the test name is e.g. `foo/simple/test_bar.in`, which
would correspond to an input file on disk.

https://codereview.chromium.org/709853003/diff/1/expect_tests/pipeline.py#new...
expect_tests/pipeline.py:574: class PackageTestingContext(object):
may be worth having all this context stuff in it's own context.py instead of
pipeline.py

https://codereview.chromium.org/709853003/diff/1/expect_tests/pipeline.py#new...
expect_tests/pipeline.py:588: def from_path(cls, path, filters='*'):
maybe make filters `('*',)` so that you don't need the basestring case (and
filters will always be an iterable-of-strings). Or make empty-sequence mean '*'
and then have it default to `()`

https://codereview.chromium.org/709853003/diff/1/expect_tests/pipeline.py#new...
expect_tests/pipeline.py:613: 'name only.'
may be friendlier to have `def merge_contexts(cls, contexts) -> contexts` which
would simplify a set of contexts to their minimal set of contexts.

Is there a way to derive package_name or cwd from each other so that it becomes
impossible to have two different packages with the same cwd, or vice versa? Then
the derivable one could be an @property

https://codereview.chromium.org/709853003/diff/1/expect_tests/pipeline.py#new...
expect_tests/pipeline.py:639: for contexts in groups.itervalues()]
Yeah, I think the merge contexts function I mentioned above would be a cleaner
interface. It could even be a standalone function?

https://codereview.chromium.org/709853003/diff/1/expect_tests/test/pipeline_t...
File expect_tests/test/pipeline_test.py (right):

https://codereview.chromium.org/709853003/diff/1/expect_tests/test/pipeline_t...
expect_tests/test/pipeline_test.py:35: the I/O functions.
+1 :)

You could also have done a setUpClass sort of thing to populate a temporary
directory. Either way is better than mocking the filesystem.

https://codereview.chromium.org/709853003/diff/1/expect_tests/test/pipeline_t...
expect_tests/test/pipeline_test.py:47: '____non-existing-path____')
this isn't meant to be a python identifier, is it? If it is, the dashes are
problematic.

https://codereview.chromium.org/709853003/diff/1/expect_tests/test/pipeline_t...
expect_tests/test/pipeline_test.py:118: self.assertEqual(context.filters[0][1],
'*')
it wouldn't be possible to break this function up into a series of shorter,
possibly-more-desrciptive tests, would it?

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

https://codereview.chromium.org/709853003/diff/20001/expect_tests/pipeline.py...
expect_tests/pipeline.py:279: test_queue (multiprocessing.Queue):
On 2014/11/12 03:15:10, agable wrote:
> Love these args with no descriptions

I think this was my fault :)

https://codereview.chromium.org/709853003/diff/20001/expect_tests/pipeline.py...
expect_tests/pipeline.py:312: if kill_switch.is_set():
On 2014/11/12 03:15:09, agable wrote:
> I know this is copied, but doesn't it make sense to check the kill switch
first,
> so you don't waste any effort?

We do that in the loop condition.

https://codereview.chromium.org/709853003/diff/20001/expect_tests/pipeline.py...
expect_tests/pipeline.py:567: ## matcher = re.compile(
On 2014/11/12 03:15:09, agable wrote:
> Not sure what these commented-out segments are/were for.

I think they were copied from above and then translated

https://codereview.chromium.org/709853003/diff/20001/expect_tests/test/pipeli...
File expect_tests/test/pipeline_test.py (right):

https://codereview.chromium.org/709853003/diff/20001/expect_tests/test/pipeli...
expect_tests/test/pipeline_test.py:41: def test_get_python_root(self):
On 2014/11/12 03:15:10, agable wrote:
> I know this looks nice and compact, but this test tests four different
> behaviors:
> 1) getting a dir with no init returns that dir
> 2) getting a package root returns its parent
> 3) getting a nested subpackage gets the parent of the root
> 4) getting a non-existent path raises an error
> That really should be four different tests, so that different behaviors can be
> modified independently. The same comment applies to the other tests below,
> especially get_runtime_context. One test per behavior per function, not just
one
> test per function.

I'm not sure I entirely agree with that.. 1 assert per test seems way too fine
grained, and you'll end up duplicating a lot to set up that 1 assert. However I
do agree that some of the longer tests could be broken up. I'd expect these to
look something like:

<common setup>  # probably a function call, maybe self.setUp()
<differential setup>
<assert block (1-5 asserts)>

IMO it's also great to do table-based tests, e.g.:

TESTS = [
  (<input>, <expected>)
]

for input, output in TESTS:
  # stuff
  # assert with context of (input, output)

The benefit being that you can clearly see the input and output side-by-side,
and it's clear to add another case, and easy to do so. But I'm not sure how well
it applies here.

[agable, 2014-11-12 23:45:11]

https://codereview.chromium.org/709853003/diff/20001/expect_tests/test/pipeli...
File expect_tests/test/pipeline_test.py (right):

https://codereview.chromium.org/709853003/diff/20001/expect_tests/test/pipeli...
expect_tests/test/pipeline_test.py:41: def test_get_python_root(self):
Please refer to go/bhnol (internal link, sorry). Test behaviors, not methods.
There can and often should be multiple asserts per test. But each of the four
commented blocks in test_get_runtime_context should obviously be a separate test
method.

[pgervais, 2014-11-13 00:28:27]

Thanks for your reviews. I'll file another patchset soon.

https://chromiumcodereview.appspot.com/709853003/diff/20001/expect_tests/pipe...
File expect_tests/pipeline.py (right):

https://chromiumcodereview.appspot.com/709853003/diff/20001/expect_tests/pipe...
expect_tests/pipeline.py:77: # at the filesystem root ...
On 2014/11/12 03:15:09, agable wrote:
> Love the French spacing between words and punctuation.

ALERT: troll detected in comment line 77.

https://chromiumcodereview.appspot.com/709853003/diff/20001/expect_tests/pipe...
expect_tests/pipeline.py:197: if not os.path.exists(os.path.join(start_path)):
On 2014/11/12 03:15:10, agable wrote:
> os.path.join not necessary

Done.

https://chromiumcodereview.appspot.com/709853003/diff/20001/expect_tests/pipe...
expect_tests/pipeline.py:279: test_queue (multiprocessing.Queue):
On 2014/11/12 20:26:24, iannucci wrote:
> On 2014/11/12 03:15:10, agable wrote:
> > Love these args with no descriptions
> 
> I think this was my fault :)

This time I think it was mine, I added them last time because the type
information is useful, and the description awkward.

https://chromiumcodereview.appspot.com/709853003/diff/20001/expect_tests/pipe...
expect_tests/pipeline.py:550: #   os.path.join(self.cwd, self.package_name)
where to look for tests for.
On 2014/11/12 03:15:10, agable wrote:
> "in which to look for tests"

Done. Thanks for the English review!

https://chromiumcodereview.appspot.com/709853003/diff/20001/expect_tests/pipe...
expect_tests/pipeline.py:557: Returns:
On 2014/11/12 03:15:09, agable wrote:
> "Returns" isn't exactly accurate for a generator.

Done.

https://chromiumcodereview.appspot.com/709853003/diff/20001/expect_tests/pipe...
expect_tests/pipeline.py:567: ## matcher = re.compile(
On 2014/11/12 20:26:24, iannucci wrote:
> On 2014/11/12 03:15:09, agable wrote:
> > Not sure what these commented-out segments are/were for.
> 
> I think they were copied from above and then translated

Removed them.

https://chromiumcodereview.appspot.com/709853003/diff/20001/expect_tests/pipe...
expect_tests/pipeline.py:585: # list of (path, filter) pairs.
On 2014/11/12 03:15:09, agable wrote:
> This comment is out of place. I think it belongs about 5 lines down above
> filters =

Dropped

https://chromiumcodereview.appspot.com/709853003/diff/20001/expect_tests/pipe...
expect_tests/pipeline.py:586: # The path is where to look for tests for. Only
tests whose name matches the
On 2014/11/12 03:15:09, agable wrote:
> "where to look for tests for" again

Done.

https://chromiumcodereview.appspot.com/709853003/diff/20001/expect_tests/pipe...
expect_tests/pipeline.py:601: assert len(cwd) == 1, \
On 2014/11/12 03:15:10, agable wrote:
> Not clear why here you use assert, while above and below you use 'if not ...:
> raise'.

Good catch. Fixed.

https://chromiumcodereview.appspot.com/709853003/diff/20001/expect_tests/pipe...
expect_tests/pipeline.py:602: 'from_context_list processes contexts with the
same working '\
On 2014/11/12 03:15:09, agable wrote:
> See comment below; this should be something like 'from_context_list was given
> process contexts containing the following cwds (...), but can only except
> contexts which all share a single cwd.'

Done.

https://chromiumcodereview.appspot.com/709853003/diff/20001/expect_tests/pipe...
expect_tests/pipeline.py:630: raise ValueError('All package must have the same
value for "cwd"')
On 2014/11/12 03:15:10, agable wrote:
> I don't think that ValueError is the right thing to raise here. I would be
more
> appropriate if the cwd could be said to be the 'value' of the TestingContext,
> but I'm not sure that's intuitive. Whether you keep this or define your own
> exception class, make the error message more helpful: state which package had
> the wrong cwd, and what cwd it should have had instead.
> 
> Actually, the above comment applies to your exceptions and asserts throughout
--
> they should include helpful debugging information instead of just saying "this
> went wrong but I won't tell you when or for what".

I've fixed the error messages in the above two places.

As for ValueError, while I agree it can be too vague, I also think there is a
value to having some general classes of exception. In the present case, I don't
think I would have a need for a different class here. The argument is that the
root cause is that the provided value of an argument is wrong. It doesn't change
much to have a way to say 'the object you provided is the right one, but one
internal value in there is wrong'.

https://chromiumcodereview.appspot.com/709853003/diff/20001/expect_tests/test...
File expect_tests/test/data/.expect_tests.cfg (right):

https://chromiumcodereview.appspot.com/709853003/diff/20001/expect_tests/test...
expect_tests/test/data/.expect_tests.cfg:4: skip=directory1
On 2014/11/12 03:15:10, agable wrote:
> Do you mean package1? directory1 and directory2 don't exist. But maybe you're
> trying to skip things that don't exist? It's not clear, and it should be.

The tests don't require those directory to exist (except for 'ignored'). I'll
add a comment to make that clear.

https://chromiumcodereview.appspot.com/709853003/diff/20001/expect_tests/test...
File expect_tests/test/data/other/README (right):

https://chromiumcodereview.appspot.com/709853003/diff/20001/expect_tests/test...
expect_tests/test/data/other/README:1: This directory should just contain
package, and should not be a package
On 2014/11/12 03:15:10, agable wrote:
> Perhaps include more information here (or in test/data/README) about what
these
> files are actually for and why they shouldn't be touched.
> 
> An alternative approach would be to describe this filesystem in the tests, and
> instantiate the whole filesystem in a temp directory at test time. That's what
> the depot_tools tests do. On the one hand, it's pretty cool. On the other
hand,
> it means encoding the contents of all of these files inside the test files.
And
> that sucks a lot. So I think this is the better solution, but it's something
to
> consider.

Having a read-only structure checked out with the code means you never run into
issues like files impossible to remove or non-writable directories. Beside, it's
easier to debug when you have the actual directory structure to look at.

I'll add more info in test/data/README.

https://chromiumcodereview.appspot.com/709853003/diff/20001/expect_tests/test...
File expect_tests/test/data/package1/file1_test.py (right):

https://chromiumcodereview.appspot.com/709853003/diff/20001/expect_tests/test...
expect_tests/test/data/package1/file1_test.py:1: import unittest
On 2014/11/12 03:15:10, agable wrote:
> It's not clear why file1_test has an actual file1 which it is testing, but
> file2_test doesn't.
> 
It was not clear to me either until you asked: I should put a unittest.TestCase
in file1.py to make sure it is not detected. Originally, the purpose was to test
the coverage functions, but I think I'll give up on that.

> I think because all of this test data is divorced from the tests that are
using
> it, it would be good to copiously comment these files so readers can easily
> refer back into the tests that use them.

Will do, though it will get out of sync pretty quickly.

https://chromiumcodereview.appspot.com/709853003/diff/20001/expect_tests/test...
File expect_tests/test/data/package1/file2_test.py (right):

https://chromiumcodereview.appspot.com/709853003/diff/20001/expect_tests/test...
expect_tests/test/data/package1/file2_test.py:6: 
On 2014/11/12 03:15:10, agable wrote:
> Trailing newline.

Done.

https://chromiumcodereview.appspot.com/709853003/diff/20001/expect_tests/test...
File expect_tests/test/data/package1/subpackage1_1/subpackage1_1_1/file4_test.py
(right):

https://chromiumcodereview.appspot.com/709853003/diff/20001/expect_tests/test...
expect_tests/test/data/package1/subpackage1_1/subpackage1_1_1/file4_test.py:6: 
On 2014/11/12 03:15:10, agable wrote:
> Here too.

Done.

https://chromiumcodereview.appspot.com/709853003/diff/20001/expect_tests/test...
File expect_tests/test/pipeline_test.py (right):

https://chromiumcodereview.appspot.com/709853003/diff/20001/expect_tests/test...
expect_tests/test/pipeline_test.py:15: content for data/.expect_tests.cfg"""
On 2014/11/12 03:15:10, agable wrote:
> nit: First line of docstring should be a single line.

Done.

https://chromiumcodereview.appspot.com/709853003/diff/20001/expect_tests/test...
expect_tests/test/pipeline_test.py:16: path = os.path.join(SCRIPT_DIR, 'data')
On 2014/11/12 03:15:10, agable wrote:
> You already have DATA_DIR defined.

Done.

https://chromiumcodereview.appspot.com/709853003/diff/20001/expect_tests/test...
expect_tests/test/pipeline_test.py:41: def test_get_python_root(self):
On 2014/11/12 23:45:11, agable wrote:
> Please refer to go/bhnol (internal link, sorry). Test behaviors, not methods.
> There can and often should be multiple asserts per test. But each of the four
> commented blocks in test_get_runtime_context should obviously be a separate
test
> method.

While I agree with what is stated in this document (separating behavior because
it helps with test maintenance) I think it is overkill in that case to write
separate functions, since testing each behavior is a single line each time. What
counts is that separated behaviors are tested in separate *places*, not
necessarily separate *functions*.

I'll add a comment here to tell that we're testing separate behaviors. And split
test_get_runtime_context into separate functions.

https://chromiumcodereview.appspot.com/709853003/diff/20001/expect_tests/test...
expect_tests/test/pipeline_test.py:53: datadir = os.path.join(SCRIPT_DIR,
'data')
On 2014/11/12 03:15:10, agable wrote:
> DATA_DIR

Done.

https://chromiumcodereview.appspot.com/709853003/diff/20001/expect_tests/test...
expect_tests/test/pipeline_test.py:99: 
On 2014/11/12 03:15:10, agable wrote:
> Please have a test for what happens if you explicitly list an ignored dir
(e.g.
> test_globs = [os.path.join(datadir, 'other')]).

I did not write such a test because the behavior is currently undefined. 
Shall we force processing even if the directory is ignored or not?

[pgervais, 2014-11-13 01:00:13]

New patchset uploaded!

[pgervais, 2014-11-13 17:55:47]

I hopefully addressed all review comments from patchset 1.

PTAL

https://chromiumcodereview.appspot.com/709853003/diff/1/expect_tests/pipeline.py
File expect_tests/pipeline.py (right):

https://chromiumcodereview.appspot.com/709853003/diff/1/expect_tests/pipeline...
expect_tests/pipeline.py:314: for g in globs if g[0] == '-'))
On 2014/11/12 20:26:24, iannucci wrote:
> I think neg_matcher got lost in the new world?

(discussed that offline) Yes, the neg_matcher was removed because it does not
make sense with the current UI. We'll have to improve it to bring it back.
Namely:

expect_tests <action> package:testname package:-badtestname

Would run all tests that match 'testname' in 'package, _then_ run all tests that
don't match 'badtestname' in 'package', which is probably not what you want.
Instead, we should have something like:

expect_tests <action> package:testname,-badtestname

which will run all tests in package that matches 'testname' and don't match
'badtestname'. Most of the tooling for this is already in place, but I'd rather
do that in another CL.

https://chromiumcodereview.appspot.com/709853003/diff/1/expect_tests/pipeline...
expect_tests/pipeline.py:540: """A test glob is composed of a path and a glob
expression like:
On 2014/11/12 20:26:24, iannucci wrote:
> do we still support the negative glob e.g. `-path/to/something:*Slow*`?

Not currently (see above comment)

https://chromiumcodereview.appspot.com/709853003/diff/1/expect_tests/pipeline...
expect_tests/pipeline.py:541: '<path>:<glob>'. The path should point to a
directory or a file inside
On 2014/11/12 20:26:24, iannucci wrote:
> TODO: let's scan packages too so that <path> can also be a glob. Then
> expect_tests can use a default of '*:*' when no tests are specified.

TODO added.

https://chromiumcodereview.appspot.com/709853003/diff/1/expect_tests/pipeline...
expect_tests/pipeline.py:548: matching test1/*_test.py.
On 2014/11/12 20:26:24, iannucci wrote:
> not strictly true, right? non-unittest tests would also match if they had a
> similar name, correct?
> 
> TestA* would also be matching the unittest classname so a single test might be
> `TestABunchOfStuff.testCatFoodDeployment`?

True, the definition of a 'test name' is not very clear here. I'll try to
clarify that in the docstring.

https://chromiumcodereview.appspot.com/709853003/diff/1/expect_tests/pipeline...
expect_tests/pipeline.py:561: if '/' in test_filter:
On 2014/11/12 20:26:24, iannucci wrote:
> Hm, I'm not sure this is strictly true... do we enforce this in Test.__init__?
> There are use cases where the test name is e.g. `foo/simple/test_bar.in`,
which
> would correspond to an input file on disk.

The test filter is only applied to test names, which are Python names. Thus
don't contain any slashes.

https://chromiumcodereview.appspot.com/709853003/diff/1/expect_tests/pipeline...
expect_tests/pipeline.py:588: def from_path(cls, path, filters='*'):
On 2014/11/12 20:26:24, iannucci wrote:
> maybe make filters `('*',)` so that you don't need the basestring case (and
> filters will always be an iterable-of-strings). Or make empty-sequence mean
'*'
> and then have it default to `()`

This is one of the features of Python that I like: being able to drop the
iterable in a parameter when you only have one item. I've fixed that.

That said, you're right that the empty sequence means ('*',), and this case is
not taken into account right now.

https://chromiumcodereview.appspot.com/709853003/diff/1/expect_tests/pipeline...
expect_tests/pipeline.py:613: 'name only.'
On 2014/11/12 20:26:24, iannucci wrote:
> may be friendlier to have `def merge_contexts(cls, contexts) -> contexts`
which
> would simplify a set of contexts to their minimal set of contexts.
> 
> Is there a way to derive package_name or cwd from each other so that it
becomes
> impossible to have two different packages with the same cwd, or vice versa?
Then
> the derivable one could be an @property

If you consider only the RuntimeContext object, the answer is no. However, they
are constrained by the filesystem because the os.path.join of them has to exist.

But there could be two packages in the same directory or two packages with the
same name in different directories (though it's not common).

https://chromiumcodereview.appspot.com/709853003/diff/1/expect_tests/test/pip...
File expect_tests/test/pipeline_test.py (right):

https://chromiumcodereview.appspot.com/709853003/diff/1/expect_tests/test/pip...
expect_tests/test/pipeline_test.py:35: the I/O functions.
On 2014/11/12 20:26:24, iannucci wrote:
> +1 :)
> 
> You could also have done a setUpClass sort of thing to populate a temporary
> directory. Either way is better than mocking the filesystem.

I replied to this elsewhere: the less I touch the filesystem the better.
Creating files means deleting them and deleting means trouble on windows (and is
generally dangerous).

https://chromiumcodereview.appspot.com/709853003/diff/1/expect_tests/test/pip...
expect_tests/test/pipeline_test.py:47: '____non-existing-path____')
On 2014/11/12 20:26:24, iannucci wrote:
> this isn't meant to be a python identifier, is it? If it is, the dashes are
> problematic.

It is meant to be a filename. Nothing is loaded by get_python_root so the name
can be arbitrary. That's possibly a bad example though.

https://chromiumcodereview.appspot.com/709853003/diff/1/expect_tests/test/pip...
expect_tests/test/pipeline_test.py:118: self.assertEqual(context.filters[0][1],
'*')
On 2014/11/12 20:26:24, iannucci wrote:
> it wouldn't be possible to break this function up into a series of shorter,
> possibly-more-desrciptive tests, would it?

Done.

[agable, 2014-11-14 20:00:09]

LGTM modulo a bunch of testing nits.

https://codereview.chromium.org/709853003/diff/20001/expect_tests/test/pipeli...
File expect_tests/test/pipeline_test.py (right):

https://codereview.chromium.org/709853003/diff/20001/expect_tests/test/pipeli...
expect_tests/test/pipeline_test.py:99: 
On 2014/11/13 at 00:28:27, pgervais wrote:
> On 2014/11/12 03:15:10, agable wrote:
> > Please have a test for what happens if you explicitly list an ignored dir
(e.g.
> > test_globs = [os.path.join(datadir, 'other')]).
> 
> I did not write such a test because the behavior is currently undefined. 
> Shall we force processing even if the directory is ignored or not?

Yeah, I asked for the test because the behavior is undefined and I wanted to
define it :) I don't know what should happen, but you should choose one and then
test it :)

https://codereview.chromium.org/709853003/diff/60001/expect_tests/test/data/R...
File expect_tests/test/data/README (right):

https://codereview.chromium.org/709853003/diff/60001/expect_tests/test/data/R...
expect_tests/test/data/README:3: themselves do not test anything (e.g. there are
assert(True) in many places)
assertTrue(True)

https://codereview.chromium.org/709853003/diff/60001/expect_tests/test/data/p...
File expect_tests/test/data/package1/README (right):

https://codereview.chromium.org/709853003/diff/60001/expect_tests/test/data/p...
expect_tests/test/data/package1/README:7: 
Nit: empty line

https://codereview.chromium.org/709853003/diff/60001/expect_tests/test/pipeli...
File expect_tests/test/pipeline_test.py (right):

https://codereview.chromium.org/709853003/diff/60001/expect_tests/test/pipeli...
expect_tests/test/pipeline_test.py:115: def
test_package_testing_context_from_path(self):
I still think this should be three methods:
test_package_testing_context_no_filters
""_empty_filters
""_with_filters

I haven't seen any reason *not* to break it up, and doing so lets more granular
results be reported.

https://codereview.chromium.org/709853003/diff/60001/expect_tests/test/pipeli...
expect_tests/test/pipeline_test.py:147: """Merging PackageTestingContexts
pointing at the same package.
Four methods. In the current method, it's not even clear *why* the second and
third test cases are supposed to raise!
test_merging_package_testing_context_same_package
test_merging_package_testing_context_???
test_merging_simlar_package_testing_context_raises
test_merging_package_and_subpackage_testing_contexts

https://codereview.chromium.org/709853003/diff/60001/expect_tests/test/pipeli...
expect_tests/test/pipeline_test.py:185: def test_filter_glob_manipulation(self):
test_filter_glob_respects_star
test_filter_glob_adds_implied_star

https://codereview.chromium.org/709853003/diff/60001/expect_tests/test/pipeli...
expect_tests/test/pipeline_test.py:192: filters=('a*',))
nit: indentation

https://codereview.chromium.org/709853003/diff/60001/expect_tests/test/pipeli...
expect_tests/test/pipeline_test.py:197: print matcher.pattern
left a debugging print in?

https://codereview.chromium.org/709853003/diff/60001/expect_tests/test/pipeli...
expect_tests/test/pipeline_test.py:206: filters=('a',))
nit: intendation

https://codereview.chromium.org/709853003/diff/60001/expect_tests/test/pipeli...
expect_tests/test/pipeline_test.py:254: # Fails because there are two different
cwd.
test_processing_context_requires_same_cwd

[agable, 2014-11-14 20:00:10]

LGTM modulo a bunch of testing nits.

[pgervais, 2014-11-14 22:35:04]

Committed patchset #5 (id:80001) manually as
924c6c885ff44e705132eefbce398cd6f190aef6 (presubmit successful).