rbt fails to post a review from Perforce client - regression from 0.7.4

Review Request #7751 — Created Nov. 2, 2015 and discarded

Information

RBTools
release-0.7.x

Reviewers

rbt will fail (with Python exception) to post review based on Perforce working directory for revision range specified like this:

rbt post //repo/path/...@123,@456

Note: My familiarity with the code base is poor and most likely this fix is NOT good for production. I mentioned it in the bug description. I post this review so you guys can fix it properly.

The comments section in the get_changenum method says that revisions dict comes from parse_revision_spec. So, it's likely that the real problem is in the parse_revision_spec, or in the way its return value is stored and later reused with get_changenum. I have no idea, really.

The fix I post here (as per Barret's request in the ticket response) prevents RBTools from failing so devs in my team can still use it for posting reviews.

I ran unit tests as recommended here: https://www.reviewboard.org/docs/codebase/dev/getting-started/
I don't know why so many of them are failing. Maybe my environment is missing some components. I also don't have all relevant SCM tools on my machine (we only use Perforce).

So far my team has great difficulties using RB with Perforce. We can't use P4 generated diffs neither in unified diff format, nor in Perforce format, see https://hellosplat.com/s/beanbag/tickets/4002/ .
We're trying to use lastest RBTools, but as mentioend above the tool fails, as well.

Only Perforce related tests are shown.

$ nosetests -v
...
Testing P4Wrapper.counters ... ok
Testing P4Wrapper.info ... ok
Testing PerforceClient.normalize_exclude_patterns ... ok
Testing PerforceClient.diff with a submitted changelist ... ERROR
Testing PerforceClient.diff with moved files and capability off ... ERROR
Testing PerforceClient.diff with moved files and capability on ... ERROR
Testing PerforceClient.diff with a pending changelist ... ERROR
Testing PerforceClient.parse_revision_spec with invalid specifications ... ok
Testing PerforceClient.parse_revision_spec with no specified revisions ... ERROR
Testing PerforceClient.parse_revision_spec with a pending changelist ... ok
Testing PerforceClient.parse_revision_spec with a shelved changelist ... ok
Testing PerforceClient.parse_revision_spec with a submitted changelist ... ok
Testing PerforceClient.parse_revision_spec with two changelists ... ok
Testing PerforceClient.get_repository_info ... ok
Testing PerforceClient.get_repository_info outside client root ... ok
Testing PerforceClient.scan_for_server_counter with reviewboard.url ... ok
Testing PerforceClient.scan_for_server_counter with encoded reviewboard.url.http:|| ... ok
...
Description From Last Updated

This is a syntax error. Can you run through a series of tests with this change, and update the Description, …

chipx86chipx86
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        rbtools/clients/perforce.py
    
    
  2. 
      
A.
chipx86
  1. 
      
  2. rbtools/clients/perforce.py (Diff revision 1)
     
     
    Show all issues

    This is a syntax error.

    Can you run through a series of tests with this change, and update the Description, Testing Done, and Bug number with all the necessary info? Specifically, I want to have info on why this failed, what situations it caused to fail, and why the fix fixes it, along with detailed testing information showing its prior failure and fix.

    1. Detailed info on why this failed is under the bug ticket. It includes instructions how to reproduce the failure with detailed information about P4 client/server versions and RBTools version.

    2. That may be, but the change summary and description (which becomes the commit) should state, at minimum, the problem and how it was fixed (in a high level way). You may want to have a look at https://www.reviewboard.org/docs/codebase/dev/writing-good-descriptions/

      Your testing done isn't very useful. Usually it suffices to say "Ran unit tests." for running the unit tests, but many many tests are failing with this patch.

    3. Alex: It really helps us to have it here for the reasons Barret stated. To elaborate further, we strive to have all relevant details in our commit history, which helps contributors to understand the purpose for a change without having to read other things, helps us when writing release notes, and helps when grepping commits for related keywords. If you look through our commit history, you'll see all commits are detailed and follow the guidelines in the link above.

      Thanks!

  3. 
      
A.
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/clients/perforce.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/clients/perforce.py
    
    
  2. 
      
A.
A.
Review request changed
Status:
Discarded