launchpad-reviewers team mailing list archive

[Jürgen Gmach <mp+414392@xxxxxxxxxxxxxxxxxx>]

Diff comments:

> diff --git a/lib/lp/code/model/tests/test_revisionstatus.py b/lib/lp/code/model/tests/test_revisionstatus.py
> index 6ce84c5..05367de 100644
> --- a/lib/lp/code/model/tests/test_revisionstatus.py
> +++ b/lib/lp/code/model/tests/test_revisionstatus.py
> @@ -239,3 +241,64 @@ class TestRevisionStatusReportWebservice(TestCaseWithFactory):
>                  result_summary=Equals(initial_result_summary),
>                  result=Equals(RevisionStatusResult.SUCCEEDED),
>                  date_finished=GreaterThan(date_finished_before_update)))
> +
> +    def test_getArtifactsURLs(self):
> +        report = self.factory.makeRevisionStatusReport()
> +        artifact_log = self.factory.makeRevisionStatusArtifact(
> +            report=report, artifact_type=RevisionStatusArtifactType.LOG,
> +            content=b'log_data')
> +        log_url = artifact_log.library_file.http_url
> +        artifact_binary = self.factory.makeRevisionStatusArtifact(
> +            report=report, artifact_type=RevisionStatusArtifactType.BINARY,
> +            content=b'binary_data')
> +        binary_url = artifact_binary.library_file.http_url
> +        requester = report.creator
> +        repository = report.git_repository
> +        report_url = api_url(report)
> +        webservice = self.getWebservice(requester, repository)
> +        response = webservice.named_get(
> +            report_url, "getArtifactsURLs", artifact_type="Log")
> +        self.assertEqual(200, response.status)
> +        with person_logged_in(requester):
> +            self.assertIn(log_url, response.jsonBody())
> +            self.assertNotIn(binary_url, response.jsonBody())
> +            file = requests.get(response.jsonBody()[0])
> +            self.assertEqual(b'log_data', file.content)
> +        response = webservice.named_get(
> +            report_url, "getArtifactsURLs", artifact_type="Binary")
> +        self.assertEqual(200, response.status)
> +        with person_logged_in(requester):
> +            self.assertNotIn(log_url, response.jsonBody())
> +            self.assertIn(binary_url, response.jsonBody())
> +            file = requests.get(response.jsonBody()[0])
> +            self.assertEqual(b'binary_data', file.content)
> +        response = webservice.named_get(
> +            report_url, "getArtifactsURLs")
> +        self.assertEqual(200, response.status)
> +        with person_logged_in(requester):
> +            self.assertIn(log_url, response.jsonBody())
> +            self.assertIn(binary_url, response.jsonBody())
> +
> +    def test_getArtifactsURLs_restricted(self):
> +        requester = self.factory.makePerson()
> +        with person_logged_in(requester):
> +            kwargs = {"owner": requester}
> +            kwargs["information_type"] = InformationType.USERDATA
> +            repository = self.factory.makeGitRepository(**kwargs)
> +            report = self.factory.makeRevisionStatusReport(
> +                git_repository=repository)
> +            report_url = api_url(report)
> +            artifact = self.factory.makeRevisionStatusArtifact(
> +                report=report, artifact_type=RevisionStatusArtifactType.LOG,
> +                content=b'log_data', restricted=True)
> +            log_url = 'http://code.launchpad.test/%s/+status/%s/+files/%s' % (
> +                repository.unique_name, report.id,
> +                artifact.library_file.filename)
> +
> +        webservice = self.getWebservice(requester, repository)
> +        response = webservice.named_get(
> +            report_url, "getArtifactsURLs", artifact_type="Log")
> +        self.assertEqual(200, response.status)
> +
> +        with person_logged_in(requester):
> +            self.assertIn(log_url, response.jsonBody())

Just a minor nitpick or let's say a personal preference of mine...

I think tests are easier to read when they are structured as arrange, act and assert.

I would write the test as...

```
    def test_getArtifactsURLs_restricted(self):
        requester = self.factory.makePerson()
        with person_logged_in(requester):
            kwargs = {"owner": requester}
            kwargs["information_type"] = InformationType.USERDATA
            repository = self.factory.makeGitRepository(**kwargs)
            report = self.factory.makeRevisionStatusReport(
                git_repository=repository)
            report_url = api_url(report)
            artifact = self.factory.makeRevisionStatusArtifact(
                report=report, artifact_type=RevisionStatusArtifactType.LOG,
                content=b'log_data', restricted=True)
            log_url = 'http://code.launchpad.test/%s/+status/%s/+files/%s' % (
                repository.unique_name, report.id,
                artifact.library_file.filename)
        webservice = self.getWebservice(requester, repository)

        response = webservice.named_get(
            report_url, "getArtifactsURLs", artifact_type="Log")

        self.assertEqual(200, response.status)
        with person_logged_in(requester):
            self.assertIn(log_url, response.jsonBody())
```

Feel free to ignore this if you think otherwise.



-- 
https://code.launchpad.net/~ilasc/launchpad/+git/launchpad/+merge/414392
Your team Launchpad code reviewers is requested to review the proposed merge of ~ilasc/launchpad:add-get-status-artifacts-api into launchpad:master.