launchpad-reviewers team mailing list archive

[Johan Dahlin <johan.dahlin@xxxxxxxxxxxxx>]

Review: Approve

Two nitpicks, looks good either way.

Diff comments:

> === modified file 'lib/lp/services/librarianserver/tests/test_web.py'
> --- lib/lp/services/librarianserver/tests/test_web.py	2018-01-02 10:54:31 +0000
> +++ lib/lp/services/librarianserver/tests/test_web.py	2018-11-01 18:49:40 +0000
> @@ -108,29 +105,34 @@
>          # displaying Ubuntu build logs in the browser.  The mimetype should be
>          # "text/plain" for these files.
>          client = LibrarianClient()
> -        contents = 'Build log...'
> -        build_log = StringIO(contents)
> +        contents = b'Build log...'
> +        build_log = BytesIO()
> +        with GzipFile(mode='wb', fileobj=build_log) as f:

This seems like a logical change instead of just porting from urllib to requests.

> +            f.write(contents)
> +        build_log.seek(0)
>          alias_id = client.addFile(name="build_log.txt.gz",
> -                                  size=len(contents),
> +                                  size=len(build_log.getvalue()),
>                                    file=build_log,
>                                    contentType="text/plain")
>  
>          self.commit()
>  
>          url = client.getURLForAlias(alias_id)
> -        fileObj = urlopen(url)
> -        mimetype = fileObj.headers['content-type']
> -        encoding = fileObj.headers['content-encoding']
> +        response = requests.get(url)
> +        response.raise_for_status()
> +        mimetype = response.headers['content-type']
> +        encoding = response.headers['content-encoding']
>          self.assertTrue(mimetype == "text/plain; charset=utf-8",
>                          "Wrong mimetype. %s != 'text/plain'." % mimetype)
>          self.assertTrue(encoding == "gzip",
>                          "Wrong encoding. %s != 'gzip'." % encoding)
> +        self.assertEqual(contents, response.content)

You could keep using StringIO and compare with response.text I think?

>  
>      def test_checkNoEncoding(self):
>          # Other files should have no encoding.
>          client = LibrarianClient()
> -        contents = 'Build log...'
> -        build_log = StringIO(contents)
> +        contents = b'Build log...'
> +        build_log = BytesIO(contents)
>          alias_id = client.addFile(name="build_log.tgz",
>                                    size=len(contents),
>                                    file=build_log,


-- 
https://code.launchpad.net/~cjwatson/launchpad/librarianserver-test-web-requests/+merge/358189
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/librarianserver-test-web-requests into lp:launchpad.