Fix scoped options initialization in test

Review Request #2850 — Created Sept. 19, 2015 and submitted

peiyu
pants
peiyuwang:peiyu/fix-scoped-options-in-test
2225
1a56002...
pants-reviews
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
    initialized.
  • For task scopes' with more than two enclosing scopes, like
    cache.goal1.task1, cache.py.check, not all enclosing scopes are present.
    In the cache.goal1.task1 case, the intermediate scope cache.goal1 is missing.
    This causes cache.goal1.task1 fail to initialize. Implemented a simplified
    version of Options.complete_scopes in fakes, so scopes initialization can
    happen in their enclosing order, i.e. cache, cache.goal1, cache.goal1.task1

https://travis-ci.org/pantsbuild/pants/builds/81191336

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
about coverage.

  • 0
  • 0
  • 4
  • 0
  • 4
Description From Last Updated
BE
  1. 
      
  2. 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.

    1. You are right, I must have confused myself, the issue with test_noqa is the dot in the scope name py.check, which is fixed by other means.

      change reverted

    2. Turns out this is a different issue following up in https://rbcommons.com/s/twitter/r/2870/ and https://github.com/pantsbuild/pants/pull/2250

  3. 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.

    1. this is only to ensure assertTrue(_context.is_unlocked) can happen automatically after yield, also because I use context manager in the other place.

      now doesn't seem to justify the saving, removed contextmanager in both places.

      slightly changed assertTrue(_context.is_unlocked) in tearDown to be assertTrue(not _context or _context.is_unlocked)

  4. 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 = ...

    etc?

  5. The original code is correct and behaving as expected:

    >>> 'foo.bar.baz'.rpartition('.')[0]
    'foo.bar'
    >>> 'foo.bar.baz'.split('.')[0]
    'foo'
    

    and

    >>> 'foo'.rpartition('.')[0]
    ''  # This is GLOBAL_SCOPE, as expected
    >>> 'foo'.split('.')[0]
    '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.

    1. yea that's what i worried as well.

      The issue is to do with dot is as separator and task scope names may contain dot, 'goal1.task1', 'py.check'

      So like you said, for such scope names to use rpartition, we will need to prepare the intermediate scopes 'cache.goal1' and 'cache.py',

      I saw extra_scopes being passed around

      https://github.com/pantsbuild/pants/blob/5c6e9b7a5c882ea49eb894708058eaa33df4dfc0/tests/python/pants_test/base_test.py#L188

      but that seems to be only for scopes derived from subsystems, not sure if we can make that a parameter

    2. after some digging, adding what's similar to options.complete_scopes to fakes seems a more proper solution, that's non-test code solves this problem

      https://github.com/pantsbuild/pants/blob/ce4a69c00367297706252f1429b11d25f505a2b9/src/python/pants/option/options.py#L67

    3. Yes, I think that's the right solution. It's a bit repetitive of the non-test code, but unfortunately we already have a lot of such repetition around options...

    4. done.

  6. 
      
PE
PE
BE
  1. Thanks for this change!

  2. 
      
PE
Review request changed

Status: Closed (submitted)

Change Summary:

Merged as 07f0235fced6823134839c81e52fcd0e735c9ab9

Loading...