Sandbox reviewboard.hostingsvcs.service.HostingService

Review Request #6652 — Created Nov. 28, 2014 and discarded

Information

Review Board
master

Reviewers

Extensions that create a HostingService subclass (using the HostingServiceHook) can throw exceptions inside Reviewboard. The parts of Reviewboard that call those methods have been sandboxed.

Now when a HostingService subclass from an extension throws an exception; Reviewboard logs the errors with enough information to find which method in the HostingService subclass threw the exception.

Unit tests have been written to make sure that functions from a Widget subclass have been called, and when an exception is thrown it gets logged.

The tests fail without the sanboxing, and succeed with it.

Description From Last Updated

'SCMTool' imported but unused

reviewbotreviewbot

This will make it fail silently rather than reporting any errors to the user.

daviddavid

This is still making it fail silently (reporting zero branches). Before it would tell the user about an error occurring. …

daviddavid

Same here.

daviddavid

And here.

daviddavid
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/hostingsvcs/models.py
        reviewboard/hostingsvcs/tests.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/hostingsvcs/models.py
        reviewboard/hostingsvcs/tests.py
    
    
  2. 
      
justy777
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/scmtools/tests.py
        reviewboard/hostingsvcs/models.py
        reviewboard/hostingsvcs/tests.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/scmtools/tests.py
        reviewboard/hostingsvcs/models.py
        reviewboard/hostingsvcs/tests.py
    
    
  2. reviewboard/scmtools/tests.py (Diff revision 2)
     
     
    Show all issues
     'SCMTool' imported but unused
    
  3. 
      
justy777
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/scmtools/tests.py
        reviewboard/hostingsvcs/models.py
        reviewboard/scmtools/models.py
        reviewboard/hostingsvcs/tests.py
        reviewboard/hostingsvcs/service.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/scmtools/tests.py
        reviewboard/hostingsvcs/models.py
        reviewboard/scmtools/models.py
        reviewboard/hostingsvcs/tests.py
        reviewboard/hostingsvcs/service.py
    
    
  2. 
      
justy777
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/hostingsvcs/models.py
        reviewboard/scmtools/models.py
        reviewboard/webapi/resources/remote_repository.py
        reviewboard/scmtools/tests.py
        reviewboard/webapi/tests/test_remote_repository.py
        reviewboard/hostingsvcs/service.py
        reviewboard/hostingsvcs/tests.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/hostingsvcs/models.py
        reviewboard/scmtools/models.py
        reviewboard/webapi/resources/remote_repository.py
        reviewboard/scmtools/tests.py
        reviewboard/webapi/tests/test_remote_repository.py
        reviewboard/hostingsvcs/service.py
        reviewboard/hostingsvcs/tests.py
    
    
  2. 
      
justy777
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/hostingsvcs/models.py
        reviewboard/scmtools/models.py
        reviewboard/webapi/resources/remote_repository.py
        reviewboard/scmtools/tests.py
        reviewboard/scmtools/admin.py
        reviewboard/webapi/tests/test_remote_repository.py
        reviewboard/hostingsvcs/service.py
        reviewboard/hostingsvcs/tests.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/hostingsvcs/models.py
        reviewboard/scmtools/models.py
        reviewboard/webapi/resources/remote_repository.py
        reviewboard/scmtools/tests.py
        reviewboard/scmtools/admin.py
        reviewboard/webapi/tests/test_remote_repository.py
        reviewboard/hostingsvcs/service.py
        reviewboard/hostingsvcs/tests.py
    
    
  2. 
      
david
  1. 
      
  2. reviewboard/scmtools/models.py (Diff revision 5)
     
     
     
     
    Show all issues

    This will make it fail silently rather than reporting any errors to the user.

  3. reviewboard/scmtools/models.py (Diff revision 5)
     
     
     
     
    Show all issues

    Same here.

  4. reviewboard/scmtools/models.py (Diff revision 5)
     
     
     
    Show all issues

    And here.

  5. 
      
justy777
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/hostingsvcs/models.py
        reviewboard/scmtools/models.py
        reviewboard/webapi/resources/remote_repository.py
        reviewboard/scmtools/tests.py
        reviewboard/scmtools/admin.py
        reviewboard/webapi/tests/test_remote_repository.py
        reviewboard/hostingsvcs/service.py
        reviewboard/hostingsvcs/tests.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/hostingsvcs/models.py
        reviewboard/scmtools/models.py
        reviewboard/webapi/resources/remote_repository.py
        reviewboard/scmtools/tests.py
        reviewboard/scmtools/admin.py
        reviewboard/webapi/tests/test_remote_repository.py
        reviewboard/hostingsvcs/service.py
        reviewboard/hostingsvcs/tests.py
    
    
  2. 
      
david
  1. In your testing done, you say "Unit tests have been written to make sure that functions from a Widget subclass have been called, and when an exception is thrown it gets logged." That doesn't seem correct for this change.

  2. reviewboard/scmtools/models.py (Diff revisions 5 - 6)
     
     
    Show all issues

    This is still making it fail silently (reporting zero branches). Before it would tell the user about an error occurring.

    These methods here should probably not be wrapped at all, because we expect them to throw exceptions and the proper error handling is already in place.

  3. 
      
justy777
Review request changed
Status:
Discarded