launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Needs Fixing

Style nit throughout which I haven't commented in every location: when you have a multi-line list or dict, please put a trailing comma on every item rather than on all but the last, in order that future changes can have smaller diffs.

Diff comments:

> 
> === modified file 'lib/lp/snappy/tests/test_snapstoreclient.py'
> --- lib/lp/snappy/tests/test_snapstoreclient.py	2018-05-31 10:23:03 +0000
> +++ lib/lp/snappy/tests/test_snapstoreclient.py	2019-06-14 13:33:05 +0000
> @@ -9,6 +9,7 @@
>  
>  import base64
>  from cgi import FieldStorage
> +from datetime import datetime, timedelta

LP style is:

from datetime import (
    datetime,
    timedelta,
    )

>  import hashlib
>  import io
>  import json
> @@ -398,6 +402,44 @@
>              ]))
>  
>      @responses.activate
> +    def test_upload_with_built_at(self):

Should this really be a separate test?  A build can't get to the point where we need to upload it without having been started, so I would say that makeUploadableSnapBuild should unconditionally put the build in a state where it has all the attributes we need, and then the relevant bits of this test should just be folded into other existing tests.

> +        timestamp = datetime(2019, 06, 14, 02, 00, tzinfo=pytz.UTC)

Write 6 rather than 06, etc. - 06 is a SyntaxError in Python 3.

> +        build_kwargs = {
> +            'date_created': timestamp,
> +            'duration': timedelta(seconds=60)
> +        }

Style: trailing comma on the second dict item, and indent the closing brace.

But rather than setting these attributes manually, and taking into account my comment above, I'd prefer makeUploadableSnapBuild to do:

    snapbuild.updateStatus(BuildStatus.BUILDING)

(Technically a build will only ever be uploaded to the store once it's in FULLYBUILT, so perhaps you should add a second updateStatus call to do that as well for verisimilitude; but setting it to BUILDING sets date_started, which is all you need for this purpose.)

You can either mock datetime.datetime.now for this, or perhaps more simply just assert that built_at is set to snapbuild.date_started.isoformat(), as there's nothing particular to be gained from picking a fixed time here.

> +        snapbuild = self.makeUploadableSnapBuild(build_kwargs=build_kwargs)
> +        transaction.commit()
> +        self._addUnscannedUploadResponse()
> +        self._addSnapPushResponse()
> +        with dbuser(config.ISnapStoreUploadJobSource.dbuser):
> +            self.assertEqual(
> +                "http://sca.example/dev/api/snaps/1/builds/1/status";,
> +                self.client.upload(snapbuild))
> +        requests = [call.request for call in responses.calls]
> +        self.assertThat(requests, MatchesListwise([
> +            RequestMatches(
> +                url=Equals("http://updown.example/unscanned-upload/";),
> +                method=Equals("POST"),
> +                form_data={
> +                    "binary": MatchesStructure.byEquality(
> +                        name="binary", filename="test-snap.snap",
> +                        value="dummy snap content",
> +                        type="application/octet-stream",
> +                        )}),
> +            RequestMatches(
> +                url=Equals("http://sca.example/dev/api/snap-push/";),
> +                method=Equals("POST"),
> +                headers=ContainsDict(
> +                    {"Content-Type": Equals("application/json")}),
> +                auth=("Macaroon", MacaroonsVerify(self.root_key)),
> +                json_data={
> +                    "name": "test-snap", "updown_id": 1, "series": "rolling",
> +                    "built_at": timestamp.isoformat(), "only_if_newer": True
> +                    }),
> +            ]))
> +
> +    @responses.activate
>      def test_upload_no_discharge(self):
>          root_key = hashlib.sha256(self.factory.getUniqueString()).hexdigest()
>          root_macaroon = Macaroon(key=root_key)


-- 
https://code.launchpad.net/~twom/launchpad/snap-release-ordering/+merge/368832
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.