• 
      

    Refactor p4 invocation from PerforceClient, and add unit tests.

    Review Request #3710 — Created Jan. 3, 2013 and submitted

    Information

    RBTools
    master

    Reviewers

    Refactor p4 invocation from PerforceClient, and add unit tests.
    
    In an effort to clean up PerforceClient a little and make it testable,
    this change consolidates all the p4 invocation into a class called
    P4Wrapper. A P4Wrapper class is passed to PerforceClient (defaults to
    P4Wrapper, but unit tests can override this), and an instance will be
    created from this. Callers can call into this and just deal with the
    results, regardless of how p4 is called.
    
    This allowed some unit tests to be written. While this is by no means
    comprehensive, it does add some basic testing for things like old-style
    and new-style Perforce counters, get_repository_info, and basic diff
    building.
    
    I'll be making use of these new tests for Perforce moved file support.
    
    Going forward, we probably should look into using p4 -G to get
    marshalled Python objects as results from the commands (in order to
    avoid doing our own parsing), and add some exceptions for failure so
    that PerforceClient doesn't have to parse error strings.
    Unit tests pass.
    
    I played around with post-review a little with a Perforce client and
    didn't see any regressions, but further, more real-world testing would
    be helpful.
    Description From Last Updated

    Is there a reason you wanted to keep this p4 invocation separate? I know it's simple, but we're going against …

    SM smacleod

    Remove this blank line.

    SM smacleod

    Col: 80 E501 line too long (81 > 79 characters)

    reviewbotreviewbot

    Col: 80 E501 line too long (97 > 79 characters)

    reviewbotreviewbot

    Col: 13 E128 continuation line under-indented for visual indent

    reviewbotreviewbot
    SM
    1. 
        
    2. rbtools/clients/perforce.py (Diff revision 1)
       
       
      Show all issues
      Is there a reason you wanted to keep this p4 invocation separate? I know it's simple, but we're going against the docstring of P4Wrapper which says "All calls out to p4 go through an instance of this class."
    3. rbtools/clients/tests.py (Diff revision 1)
       
       
      Show all issues
      Remove this blank line.
    4. 
        
    chipx86
    reviewbot
    1. This is a review from Review Bot.
        Tool: PEP8 Style Checker
        Processed Files:
          rbtools/clients/tests.py
          rbtools/utils/testbase.py
          rbtools/clients/perforce.py
        Ignored Files:
      
      
    2. rbtools/clients/tests.py (Diff revision 2)
       
       
      Show all issues
      Col: 80
       E501 line too long (81 > 79 characters)
      
    3. rbtools/clients/tests.py (Diff revision 2)
       
       
      Show all issues
      Col: 80
       E501 line too long (97 > 79 characters)
      
    4. rbtools/clients/tests.py (Diff revision 2)
       
       
      Show all issues
      Col: 13
       E128 continuation line under-indented for visual indent
      
      1. Wouldn't you usually pull 'diff_content,' down a line, which would get rid of this?
    5. 
        
    SM
    1. Ship It!
    2. 
        
    chipx86
    Review request changed
    Status:
    Completed
    Change Summary:
    Pushed to master (8d532d1)