Fix scoped options initialization in test
Review Request #2850 - Created Sept. 20, 2015 and submitted
|benjyw, jsirois, stuhood|
This is to fix the task scoped options not being initialized for tests,
discovered from debugging https://rbcommons.com/s/twitter/r/2815/
TL;DR a new option was added to CacheSetup subsystem and subsequently passed to
CacheFactory contructor, previous options are also passed but not used so
failing to pass them in test hasn't caused us any trouble until now.
Two issues in context initialization for scoped options in test, one is
how it is used, the other is in fakes.create_options_for_optionables
- for_task_types is needed so subsystem CacheSetup can be discovered and
- For task scopes' with more than two enclosing scopes, like
cache.py.check, not all enclosing scopes are present.
cache.goal1.task1case, the intermediate scope
cache.goal1.task1fail to initialize. Implemented a simplified
fakes, so scopes initialization can
happen in their enclosing order, i.e.
And with the fix https://rbcommons.com/s/twitter/r/2815/
now passed https://travis-ci.org/peiyuwang/pants/builds/81186394
previously failed https://travis-ci.org/pantsbuild/pants/builds/81072424
I am confused about the coverage drop " Coverage decreased (-1.7%) to 73.86%",
https://coveralls.io/builds/3618389 it's supposed not to change anything
You shouldn't need to pass this in explicitly here. TaskTestBase.context() should do this automatically, using cls.task_type().
If it doesn't work without this then something else is wrong, so let's find and fix whatever that is.
Why yield and not return? It's a bit weird since the superclass method doesn't yield, so it's a subtle change of contract.
These with-contexts seem like overkill (see comment above about yield vs. return). There's no RAII-style cleanup happening on the scope boundary (as far as I can tell), so why not just assign:
task1 = ...
task2 = ...
The original code is correct and behaving as expected:
>>> 'foo.bar.baz'.rpartition('.') 'foo.bar' >>> 'foo.bar.baz'.split('.') 'foo'
>>> 'foo'.rpartition('.') '' # This is GLOBAL_SCOPE, as expected >>> 'foo'.split('.') 'foo'
This sets enclosing_scope to the immediately enclosing scope.
Your change is papering over a problem elsewhere (presumably the intermediate scopes need to be in the known scopes). So let's find and fix that problem.
Address Benji's comments
Revision 2 (+94 -72)
updated description to reflect change