Code review comment for lp:~submarine/ubuntu-scopes/colourlovers-set-as-wallpaper

Revision history for this message
James Henstridge (jamesh) wrote :

Looks better.

Two issues:

[1]
Rather than using a fixed temporary directory, create one with tempfile.mkdtemp() in the test's setUp() method, and delete it in tearDown() using shutil.rmtree(..., ignore_errors=True).

[2]
The test_preview() method looks a bit muddled. It appears to be calling unbound methods of the Preview class passing the Preview class itself (as opposed to an instance) as the first argument.

Isn't it possible to just call scope.create_previewer(result, metadata) to create the previewer instance? You could then call run() on it once, and then inspect the resulting preview.

With those two issues out of the way, I'd be happy for the branch to land.

review: Needs Fixing

« Back to merge proposal