Cleanup machine based on the state in configuration file for mini installer test.

Cleanup machine based on the state in configuration file for mini installer test.

RunCleanCommand in test_installer now will reset the machine if |force_clean| is True after running uninstaller.

The resetting is based on the state in config.config and assoicated .prop file.
The state for the initial cleanup before all test cases is 'clean'.
The state for the tearDown is the last state of each test case.

Cleanup only supports deleting directory and registry key now. Both of them will always be executed recursively.


BUG=6698997

[zmin, 2017-03-13 19:55:52]

Patchset #2 (id:20001) has been deleted

[zmin, 2017-03-13 20:35:34]

The CQ bit was checked by zmin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-13 20:36:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-13 21:40:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-13 21:40:47]

Dry run: This issue passed the CQ dry run.

[zmin, 2017-03-13 21:42:37]

zmin@chromium.org changed reviewers:
+ grt@chromium.org

[zmin, 2017-03-13 21:42:38]

Hi Greg,

Can you please take a look this CL please?

Owen

[grt (UTC plus 2), 2017-03-14 09:19:44]

This is a great start. See below for a proposal to make it a bit more elegant.

https://codereview.chromium.org/2747023002/diff/40001/chrome/test/mini_instal...
File chrome/test/mini_installer/cleanup.py (right):

https://codereview.chromium.org/2747023002/diff/40001/chrome/test/mini_instal...
chrome/test/mini_installer/cleanup.py:87:
self._cleaners[verifier_name].VerifyInput(verifier_input,
rather than using a Verifier to do the cleaning, i think it's more elegant to
turn walking a state's property dict into something more like a proper Visitor
pattern. verification would look sorta like:

// Traverse a state's property dict, invoking |visitor|'s
// "VisitPropertyName" methods along the way.
WalkPropertyDict(visitor):
  for each (name, input) in property dict:
    // invoke the visitor's "VisitFooBar" method
    getattr(visitor, 'Visit' + name)(input, expander)

// A visitor for state property dict walking that verifies the state.
VerifierVisitor:
  VisitFiles:
    self.file_verifier.VerifyInput
  VisitProcesses:
    self.process_verifier.VerifyInput
  VisitRegistryEntries:
    self.registry_verifier.VerifyInput

VerifyAll:
  WalkPropertyDict(new VerifierVisitor)

and then cleaning would look like:

// A visitor for state property dict walking that cleans to make the state
valid.
CleanerVisitor:
  VisitFiles:
    self.file_cleaner.CleanInput
  VisitProcesses:
    self.process_cleaner.CleanInput
  VisitRegistryEntries:
    self.registry_cleaner.CleanInput

CleanAll:
  WalkPropertyDict(new CleanerVisitor)

https://codereview.chromium.org/2747023002/diff/40001/chrome/test/mini_instal...
File chrome/test/mini_installer/file_verifier.py (right):

https://codereview.chromium.org/2747023002/diff/40001/chrome/test/mini_instal...
chrome/test/mini_installer/file_verifier.py:31: self._file_path =
variable_expander.Expand(expectation_name)
why store state in the verifier? one instance is used repeatedly over many
inputs, so i don't see why this is necessary or beneficial.

https://codereview.chromium.org/2747023002/diff/40001/chrome/test/mini_instal...
File chrome/test/mini_installer/mini_installer_test.gni (right):

https://codereview.chromium.org/2747023002/diff/40001/chrome/test/mini_instal...
chrome/test/mini_installer/mini_installer_test.gni:10: "cleanup.py",
looks like we've added some config files over the years that we haven't added to
this list. would you please update everything in here while you're at it?
thanks.

https://codereview.chromium.org/2747023002/diff/40001/chrome/test/mini_instal...
File chrome/test/mini_installer/test_installer.py (right):

https://codereview.chromium.org/2747023002/diff/40001/chrome/test/mini_instal...
chrome/test/mini_installer/test_installer.py:125: RunCleanCommand(True,
self._config.states[self._test[-1]],
rather than baking in the assumption that the last state for every test is the
clean state, what do you think about addressing the clean state by name here
using the value parsed from the command line (default='clean') as i suggest
below?

https://codereview.chromium.org/2747023002/diff/40001/chrome/test/mini_instal...
chrome/test/mini_installer/test_installer.py:301: DEFAULT_CLEAN_STATE = 'clean'
how about making this an argument for the parser with a default value of
'clean'?

https://codereview.chromium.org/2747023002/diff/40001/chrome/test/mini_instal...
File chrome/test/mini_installer/verifier_runner.py (right):

https://codereview.chromium.org/2747023002/diff/40001/chrome/test/mini_instal...
chrome/test/mini_installer/verifier_runner.py:21: FILES:
file_verifier.FileVerifier(),
why this change?

[zmin, 2017-03-14 23:09:02]

Patchset #3 (id:60001) has been deleted

[zmin, 2017-03-14 23:10:21]

The CQ bit was checked by zmin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 23:11:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[zmin, 2017-03-14 23:13:00]

I have refactor the code and I'll try to test for it with more test cases
tomorrow. I think maybe we shall create a unit_test for this test?

https://codereview.chromium.org/2747023002/diff/40001/chrome/test/mini_instal...
File chrome/test/mini_installer/cleanup.py (right):

https://codereview.chromium.org/2747023002/diff/40001/chrome/test/mini_instal...
chrome/test/mini_installer/cleanup.py:87:
self._cleaners[verifier_name].VerifyInput(verifier_input,
On 2017/03/14 09:19:44, grt (UTC plus 1) wrote:
> rather than using a Verifier to do the cleaning, i think it's more elegant to
> turn walking a state's property dict into something more like a proper Visitor
> pattern. verification would look sorta like:
> 
> // Traverse a state's property dict, invoking |visitor|'s
> // "VisitPropertyName" methods along the way.
> WalkPropertyDict(visitor):
>   for each (name, input) in property dict:
>     // invoke the visitor's "VisitFooBar" method
>     getattr(visitor, 'Visit' + name)(input, expander)
> 
> // A visitor for state property dict walking that verifies the state.
> VerifierVisitor:
>   VisitFiles:
>     self.file_verifier.VerifyInput
>   VisitProcesses:
>     self.process_verifier.VerifyInput
>   VisitRegistryEntries:
>     self.registry_verifier.VerifyInput
> 
> VerifyAll:
>   WalkPropertyDict(new VerifierVisitor)
> 
> and then cleaning would look like:
> 
> // A visitor for state property dict walking that cleans to make the state
> valid.
> CleanerVisitor:
>   VisitFiles:
>     self.file_cleaner.CleanInput
>   VisitProcesses:
>     self.process_cleaner.CleanInput
>   VisitRegistryEntries:
>     self.registry_cleaner.CleanInput
> 
> CleanAll:
>   WalkPropertyDict(new CleanerVisitor)

Ok, I'll refactor the code.

https://codereview.chromium.org/2747023002/diff/40001/chrome/test/mini_instal...
File chrome/test/mini_installer/mini_installer_test.gni (right):

https://codereview.chromium.org/2747023002/diff/40001/chrome/test/mini_instal...
chrome/test/mini_installer/mini_installer_test.gni:10: "cleanup.py",
On 2017/03/14 09:19:44, grt (UTC plus 1) wrote:
> looks like we've added some config files over the years that we haven't added
to
> this list. would you please update everything in here while you're at it?
> thanks.

Done.

https://codereview.chromium.org/2747023002/diff/40001/chrome/test/mini_instal...
File chrome/test/mini_installer/test_installer.py (right):

https://codereview.chromium.org/2747023002/diff/40001/chrome/test/mini_instal...
chrome/test/mini_installer/test_installer.py:125: RunCleanCommand(True,
self._config.states[self._test[-1]],
On 2017/03/14 09:19:44, grt (UTC plus 1) wrote:
> rather than baking in the assumption that the last state for every test is the
> clean state, what do you think about addressing the clean state by name here
> using the value parsed from the command line (default='clean') as i suggest
> below?

Done.

[commit-bot: I haz the power, 2017-03-14 23:22:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-14 23:22:12]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[zmin, 2017-03-14 23:27:11]

The CQ bit was checked by zmin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 23:28:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-15 02:41:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-15 02:41:40]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[grt (UTC plus 2), 2017-03-15 09:00:08]

i think this can be simplified further. will this work?

---- visitor.py ---
class Visitor:
  """An abstract visitor for use with WalkState."""
  def VisitFile(self, variable_expander, expectation_name, expectation):
    raise NotImplementedError

  def VisitProcess(self, variable_expander, expectation_name, expectation):
    raise NotImplementedError

  def VisitRegistryEntry(self, variable_expander, expectation_name,
expectation):
    raise NotImplementedError


---- state_walker.py ---
class StateWalker:
  # Map each type name to its corresponding Visitor method.
  _type_to_method = {
    'Files': 'VisitFile',
    'Processes': 'VisitProcess',
    'RegistryEntries': 'RegistryEntry',
  }

  def __init__(self, visitor):
    self._visitor = visitor

  def Walk(self, variable_expander, property_dict):
    """Traverses |property_dict|, invoking |_visitor|'s VisitFoo method for each
     expectation."""
    for type_name, expectations in property_dict.iteritems():
      for expectation_name, expectation_dict in expectations.iteritems():
        # Skip over expectations with conditions that aren't satisfied.
        if 'condition' in expectation_dict:
          condition = variable_expander.Expand(expectation_dict['condition'])
          if not self._EvaluateCondition(condition):
            continue
        getattr(self._visitor, _type_to_method[type_name])(
          variable_expander, expectation_name, expectation)

  def _EvaluateCondition(self, condition):
    """Evaluates |condition| using eval().

    Args:
      condition: A condition string.

    Returns:
      The result of the evaluated condition.
    """
    return eval(condition, {'__builtins__': {'False': False, 'True': True}})


---- verifier_visitor.py ---
class VerifierVisitor(Visitor):
  def VisitFile(self, variable_expander, expectation_name, expectation):
    # FileVerifier._VerifyExpectation, more or less. you could either pull each
    # of these single-expectation-verifier functions into their own files if
    # you wanted to, so that VerifierVisitor is a collection of single-line
    # functions that call the external verifiers. or just inline the
    # verification.

  def VisitProcess(self, variable_expander, expectation_name, expectation):
    # ProcessVerifier._VerifyExpectation, more or less

  def VisitRegistryEntry(self, variable_expander, expectation_name,
expectation):
    # RegistryVerifier._VerifyExpectation, more or less


---- test_installer.py ----
# in InstallerTest.__init__:
    self._verifier = StateWalker(VerifierVisitor())

# in InstallerTest._VerifyState:
    self._verifier.Walk(self._variable_expander, self.config.states[state])

https://codereview.chromium.org/2747023002/diff/100001/chrome/test/mini_insta...
File chrome/test/mini_installer/visitor_runner.py (right):

https://codereview.chromium.org/2747023002/diff/100001/chrome/test/mini_insta...
chrome/test/mini_installer/visitor_runner.py:31: visitor_name =
self._Convert(property_name) + '_visitor'
i don't understand this part -- why is there a per-type visitor? in a typical
visitor pattern, the visitor interface has a virtual method for each "thing"
being visited. there's no magic in finding which visitor to invoke -- there's
just one visitor to which each "thing" is dispatched.

[zmin, 2017-03-15 20:37:23]

On 2017/03/15 09:00:08, grt (UTC plus 1) wrote:
> i think this can be simplified further. will this work?
> 
> ---- visitor.py ---
> class Visitor:
>   """An abstract visitor for use with WalkState."""
>   def VisitFile(self, variable_expander, expectation_name, expectation):
>     raise NotImplementedError
> 
>   def VisitProcess(self, variable_expander, expectation_name, expectation):
>     raise NotImplementedError
> 
>   def VisitRegistryEntry(self, variable_expander, expectation_name,
> expectation):
>     raise NotImplementedError
> 
> 
> ---- state_walker.py ---
> class StateWalker:
>   # Map each type name to its corresponding Visitor method.
>   _type_to_method = {
>     'Files': 'VisitFile',
>     'Processes': 'VisitProcess',
>     'RegistryEntries': 'RegistryEntry',
>   }
> 
>   def __init__(self, visitor):
>     self._visitor = visitor
> 
>   def Walk(self, variable_expander, property_dict):
>     """Traverses |property_dict|, invoking |_visitor|'s VisitFoo method for
each
>      expectation."""
>     for type_name, expectations in property_dict.iteritems():
>       for expectation_name, expectation_dict in expectations.iteritems():
>         # Skip over expectations with conditions that aren't satisfied.
>         if 'condition' in expectation_dict:
>           condition = variable_expander.Expand(expectation_dict['condition'])
>           if not self._EvaluateCondition(condition):
>             continue
>         getattr(self._visitor, _type_to_method[type_name])(
>           variable_expander, expectation_name, expectation)
> 
>   def _EvaluateCondition(self, condition):
>     """Evaluates |condition| using eval().
> 
>     Args:
>       condition: A condition string.
> 
>     Returns:
>       The result of the evaluated condition.
>     """
>     return eval(condition, {'__builtins__': {'False': False, 'True': True}})
> 
> 
> ---- verifier_visitor.py ---
> class VerifierVisitor(Visitor):
>   def VisitFile(self, variable_expander, expectation_name, expectation):
>     # FileVerifier._VerifyExpectation, more or less. you could either pull
each
>     # of these single-expectation-verifier functions into their own files if
>     # you wanted to, so that VerifierVisitor is a collection of single-line
>     # functions that call the external verifiers. or just inline the
>     # verification.
> 
>   def VisitProcess(self, variable_expander, expectation_name, expectation):
>     # ProcessVerifier._VerifyExpectation, more or less
> 
>   def VisitRegistryEntry(self, variable_expander, expectation_name,
> expectation):
>     # RegistryVerifier._VerifyExpectation, more or less
> 
> 
> ---- test_installer.py ----
> # in InstallerTest.__init__:
>     self._verifier = StateWalker(VerifierVisitor())
> 
> # in InstallerTest._VerifyState:
>     self._verifier.Walk(self._variable_expander, self.config.states[state])
> 
>
https://codereview.chromium.org/2747023002/diff/100001/chrome/test/mini_insta...
> File chrome/test/mini_installer/visitor_runner.py (right):
> 
>
https://codereview.chromium.org/2747023002/diff/100001/chrome/test/mini_insta...
> chrome/test/mini_installer/visitor_runner.py:31: visitor_name =
> self._Convert(property_name) + '_visitor'
> i don't understand this part -- why is there a per-type visitor? in a typical
> visitor pattern, the visitor interface has a virtual method for each "thing"
> being visited. there's no magic in finding which visitor to invoke -- there's
> just one visitor to which each "thing" is dispatched.

Ah, guess i didn't see the keyword 'pattern' yesterday and misunderstood your
idea.
I thought visitor here just means visiting config file.

I think mine here is more like a template method pattern. (I can rename the
class later, can't come up with a good name now)
the base class defines the structure of visiting each property.
the subclasses define how to get and handle data from property and OS.

I didn't have a strong opinion of which one is better here. VisitFile,
VisitRegistry and VisitProcess are not similar actions, so I didn't see the
advantage of putting them together.
And it will increase the complexity when adding new property in the future. 


_Convert here just for the reflection, changing the CamelCase in prop file into
snake_case which is used by python variable so that the subclass can be get with
getattr()
Or I can use map, either way.

[grt (UTC plus 2), 2017-03-16 08:36:22]

On 2017/03/15 20:37:23, zmin wrote:
> Ah, guess i didn't see the keyword 'pattern' yesterday and misunderstood your
> idea.
> I thought visitor here just means visiting config file.
> 
> I think mine here is more like a template method pattern. (I can rename the
> class later, can't come up with a good name now)
> the base class defines the structure of visiting each property.
> the subclasses define how to get and handle data from property and OS.
> 
> I didn't have a strong opinion of which one is better here. VisitFile,
> VisitRegistry and VisitProcess are not similar actions, so I didn't see the
> advantage of putting them together.

They're not necessarily put together via the visitor pattern. Their
implementations can be wherever you like. For example, you could have a
file_operations.py that has all of the code for VerifyFile and CleanFile and
their various helper functions. Each VerifyVisitor method is just a single
function call that connects the traversal to the proper utility function. This
way you have the file things grouped together (same for the registry and process
things), with a very simple Visitor class connecting the dots. I think Visitor
pattern also makes the traversal itself simpler. The various Runner classes
you've created are more complex than the Walker I propose.

> And it will increase the complexity when adding new property in the future. 

I don't see this. Yes, the addition of a new type of node (File, Registry,
Process, ???) requires the addition of a new method in each Visitor. It is, of
course, necessary to add new method(s) somewhere. In the visitor pattern, it's
super clear where the additions must take place. The code implementing the new
Verify/Clean operations can be wherever makes the most sense (perhaps in a .py
file dedicated to whatever that new node type is).

[zmin, 2017-03-22 23:22:29]

The CQ bit was checked by zmin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 23:23:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[zmin, 2017-03-22 23:25:21]

I refactor the code to use the visitor pattern.

[commit-bot: I haz the power, 2017-03-23 00:31:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-23 00:31:33]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[grt (UTC plus 2), 2017-03-23 10:26:09]

Hi Owen. I don't understand the design here, perhaps because I'm still mentally
stuck on a classical Visitor pattern. I envision a clear separation of concerns
like this:

- StateWalker does nothing more than walk the property dict, evaluate
conditions, and then dispatch to the type-specific method on the Visitor it is
given. It doesn't know what the Visitor is going to do and doesn't need to know
about any type-specific implementation (i.e., File, Process, Registry classes).

- Concrete Visitor types dispatch type-specific methods to the appropriate
behavior.

So rather than:

Walk -> create entry (File, Registry, Process) -> entry.Check -> visitor.Visit

we would have:

Walk -> visitor.Visit -> (somehow the operation is done)

Referring to my proposal in comment 19, here's one possibility for VisitFile for
each type of visitor:

--- verifier_visitor.py ---

import file_handler

class VerifierVisitor(Visitor):
  def __init__(self):
    self._file_handler = file_handler.FileHandler()
    self._process_handler = ...

  def VisitFile(self, variable_expander, expectation_name, expectation):
    self._file_handler.Verify(variable_expander, expectation_name, expectation)

--- cleaner_visitor.py ---

import file_handler

class CleanerVisitor(Visitor):
  def __init__(self):
    self._file_handler = file_handler.FileHandler()
    self._process_handler = ...

  def VisitFile(self, variable_expander, expectation_name, expectation):
    self._file_handler.Clean(variable_expander, expectation_name, expectation)

--- file_handler.py ---

class FileHandler:
  def Verify(variable_expander, expectation_name, expectation):
    file_path = variable_expander.Expand(expectation_name)
    file_exists = os.path.exists(file_path)
    assert expectation['exists'] == file_exists, \
        ('File %s exists' % file_path) if file_exists else \
        ('File %s is missing' % file_path)

  def Clean(variable_expander, expectation_name, expectation):
    """Delete files and directories that would cause a non-existence
       expectation to fail."""
    # Nothing to do if the expectation is that the file should exist.
    if expectation['exists']:
      return
    file_path = variable_expander.Expand(expectation_name)
    if os.path.exists(file_path):
      ...delete file_path...

Alternatively, if that seems like too much hoop jumping (Walker ->
OperationVisitor -> TypeHandler), you could do away with the visitor and more
tightly couple the walking with the operation by baking the visitor into the
Walker and indicating the operation via an in param or something:

class StateWalker:
  _type_to_handler = {
    'Files': file_handler.FileHandler(),
    'Processes': process_handler.ProcessHandler(),
    'RegistryEntries': registry_entry_handler.RegistryEntryHandler(),
  }

  def Walk(self, variable_expander, property_dict, operation):
    """Traverses |property_dict|, invoking the relevant handler's |operation|
       ('Verify' or 'Clean') method for each expectation."""
    for type_name, expectations in property_dict.iteritems():
      for expectation_name, expectation_dict in expectations.iteritems():
        # Skip over expectations with conditions that aren't satisfied.
        if 'condition' in expectation_dict:
          condition = variable_expander.Expand(expectation_dict['condition'])
          if not self._EvaluateCondition(condition):
            continue
        getattr(StateWalker._type_to_handler[type_name], operation)(
          variable_expander, expectation_name, expectation)

Does this make sense? I'm totally fine if you want to keep things simple and
skip the Visitor classes.