Offspring

Code review comment for lp:~timrchavez/offspring/offspring-sync-milestones-gracefully-handle-buggy-launchpad-api

offspring-sync-milestones-gracefully-handle-buggy-launchpad-api
Merge into trunk

Revision history for this message

Timothy R. Chavez (timrchavez) wrote on 2012-06-13:

> #1
>
> + def test_sync_project_milestones(self):
> + """
> + Test milestone syncing does not abort with bad milestone objects.
> + """
> +
> + def test_sync_project_milestones_handles_bad_milestone(self):
> + """
> + Test milestone syncing does not abort with bad milestone objects.
> + """
>
> Something's not quite right, they both have the same docstring ;-)

Oops. Good catch.

>
> #2
>
> I appreciate that this is a hangover from the previous implementation...but...
>
> + try:
> + credentials.load(open(config.get("master", "launchpad_oauth")))
> + launchpad = Launchpad(credentials, LPNET_SERVICE_ROOT)
> + except:
> + launchpad = Launchpad.get_token_and_login(
> + 'Offspring Image Build System', LPNET_SERVICE_ROOT, cachedir)
> + launchpad.credentials.save(
> + file(config.get("master", "launchpad_oauth"), "w"))
>
> the bare except should be fixed with something a bit more appropriate...
>

I agree with you that bare excepts are trouble, but I'm addressing a specific problem with this MP. I've filed a separate bug to address them: https://bugs.launchpad.net/offspring/+bug/1012487

> #3
>
> I'd like to see a test with a successful run of the script...that doesn't have
> to hit Launchpad, but there should be a test with sync_milestones returning
> something other than None

Not sure I follow here. I re-arranged the script at little as possible to write tests that were specifically designed to validate the fix contained in this MP (though one could argue that only the second test is needed to accomplish this). The point is, an invalid milestone (retrieved from Launchpad and simulated in the second test) should not cause the script to abort. By returning 'None', we prove that we did not abort, and the function returned. I think any other tests fall outside the scope of this MP an will require additional changes to the script (if I understand your request correctly).

I pushed a new branch which makes the first test more solid (actually tests a valid milestone scenario).

> #1
> 
> +    def test_sync_project_milestones(self):
> +        """
> +        Test milestone syncing does not abort with bad milestone objects.
> +        """
> +
> +    def test_sync_project_milestones_handles_bad_milestone(self):
> +        """
> +        Test milestone syncing does not abort with bad milestone objects.
> +        """
> 
> Something's not quite right, they both have the same docstring ;-)

Oops.  Good catch.

> 
> #2
> 
> I appreciate that this is a hangover from the previous implementation...but...
> 
> +    try:
> +        credentials.load(open(config.get("master", "launchpad_oauth")))
> +        launchpad = Launchpad(credentials, LPNET_SERVICE_ROOT)
> +    except:
> +        launchpad = Launchpad.get_token_and_login(
> +            'Offspring Image Build System', LPNET_SERVICE_ROOT, cachedir)
> +        launchpad.credentials.save(
> +            file(config.get("master", "launchpad_oauth"), "w"))
> 
> the bare except should be fixed with something a bit more appropriate...
>

I agree with you that bare excepts are trouble, but I'm addressing a specific problem with this MP.  I've filed a separate bug to address them: https://bugs.launchpad.net/offspring/+bug/1012487
 
> #3
> 
> I'd like to see a test with a successful run of the script...that doesn't have
> to hit Launchpad, but there should be a test with sync_milestones returning
> something other than None

Not sure I follow here.  I re-arranged the script at little as possible to write tests that were specifically designed to validate the fix contained in this MP (though one could argue that only the second test is needed to accomplish this).  The point is, an invalid milestone (retrieved from Launchpad and simulated in the second test) should not cause the script to abort.  By returning 'None', we prove that we did not abort, and the function returned.  I think any other tests fall outside the scope of this MP an will require additional changes to the script (if I understand your request correctly).

I pushed a new branch which makes the first test more solid (actually tests a valid milestone scenario).

« Back to merge proposal