Closed Bug 1337517 Opened 7 years ago Closed 7 years ago

Write integration test for fake reviewboard service

Categories

(Conduit :: General, defect)

Product:
Conduit
Component:
General
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: mars, Assigned: mars)

References

(
URL
)

Details

Attachments

(3 files)

autoland: add a configurable setting for the reviewboard service url (bug 1337517)
59 bytes, text/x-review-board-request
smacleod
: review+
Details
autoland: write integration test for fake reviewboard service (bug 1337517)
59 bytes, text/x-review-board-request
smacleod
: review+
Details
autoland: remove pycharm testrunner PYTHONPATH hack (bug 1337517)
59 bytes, text/x-review-board-request
smacleod
: review+
Details 
Comment on attachment 8839659 [details]
autoland: add a configurable setting for the reviewboard service url (bug 1337517)

https://reviewboard.mozilla.org/r/114236/#review116034

::: autoland/webapi/autolandweb/server.py:20
(Diff revision 1)
>  from autolandweb.routes import ROUTES
>  
>  logger = logging.getLogger(__name__)
>  
>  
> -def make_app(debug, version_data):
> +def make_app(debug, version_data, reviewboard_url):

Ya, I think kwargs probably make sense for all of these. Mind changing that up while you're here?

::: autoland/webapi/autolandweb/server.py:40
(Diff revision 1)
>  @click.option(
>      '--version-path',
>      envvar='AUTOLANDWEB_VERSION_PATH',
>      default='/app/version.json'
>  )
> -def autolandweb(debug, port, pretty_log, version_path):
> +@click.option('--reviewboard-url', envvar='REVIEWBOARD_URL', default='')

I've been keeping these in alphabetical order.

::: autoland/webapi/tests/test_dockerflow_container_api.py:23
(Diff revision 1)
> -    return make_app(
> -        False, {
> +    debug = False
> +    version_data = {
>              'commit': None,
>              'version': 'test',
>              'source': 'https://hg.mozilla.org/automation/conduit',
>              'build': 'test',
>          }
> -    )
> +    reviewboard_url = None
> +
> +    return make_app(debug, version_data, reviewboard_url)

How about we turn these things into kwargs, that way it'll make the parameters they fill obvious, which is what I *think* you were trying to do here?
Attachment #8839659 - Flags: review?(smacleod) → review-
Comment on attachment 8839660 [details]
autoland: write integration test for fake reviewboard service (bug 1337517)

https://reviewboard.mozilla.org/r/114238/#review116036

::: autoland/webapi/autolandweb/routes.py:27
(Diff revision 1)
> -    def get(self, repo=None):
> -        pass
> +    async def get(self, repo=None):
> +        if repo is None:
> +            return
> +
> +        http = tornado.httpclient.AsyncHTTPClient()
> +        repo_url = self.settings['reviewboard_url'] + '/repos/' + repo

repo is completely passed by the user and isn't validated *at all*, we need to make sure we're not opening a security whole by just appending to the url here.

::: autoland/webapi/autolandweb/routes.py:40
(Diff revision 1)
> +            self.set_status(200)
> +        elif response.code == 404:
> +            self.set_status(404)
> +            self.write({'error': 'Repo not found'})
> +        else:
> +            # Everything not 2XX, 3XX or 404 is an exception.

You're not checking for 3XX above, I guess it followed the redirect automatically? If so this comment in combination with the conditionals *looks* like a bug. Is there some way we can be more clear about this?

::: autoland/webapi/autolandweb/routes.py:43
(Diff revision 1)
> +            self.write({'error': 'Repo not found'})
> +        else:
> +            # Everything not 2XX, 3XX or 404 is an exception.
> +            response.rethrow()
> +
> +        self.finish()

You don't need to call finish if you're just running off the end of `get()`, it will finish automatically.

::: autoland/webapi/autolandweb/testing.py:14
(Diff revision 1)
> +from collections import namedtuple
> +
> +import requests
> +
> +
> +class MountebankClient:

This made my Python 2 brain hurt, heh.

::: autoland/webapi/tests/test_repo_check.py:19
(Diff revision 1)
> +
> +from autolandweb.server import make_app
> +from autolandweb.testing import MountebankClient
> +
> +
> +class FakeReviewBoard:

Part of me thinks we should factor this into its own module which we could build over time... I guess that can come in the change that needs it in another test though.
Attachment #8839660 - Flags: review?(smacleod) → review-
Comment on attachment 8839661 [details]
autoland: remove pycharm testrunner PYTHONPATH hack (bug 1337517)

https://reviewboard.mozilla.org/r/114240/#review116080

::: autoland/webapi/tests/test_dockerflow_container_api.py:13
(Diff revision 1)
> -# FIXME: need to get pycharm test runner to add /app to the container
> -# PYTHONPATH
> -import sys
> -sys.path.insert(0, '/app')
> -
>  from autolandweb.server import make_app  # noqa

now that this doesn't have code between it and the other imports we can get rid of `# noqa`
Attachment #8839661 - Flags: review?(smacleod) → review-
Comment on attachment 8839660 [details]
autoland: write integration test for fake reviewboard service (bug 1337517)

https://reviewboard.mozilla.org/r/114238/#review116036

> repo is completely passed by the user and isn't validated *at all*, we need to make sure we're not opening a security whole by just appending to the url here.

I don't know the best way to validate this yet.  I'll add a bug card and FIXME in the code and we can look at it next sprint.
Comment on attachment 8839659 [details]
autoland: add a configurable setting for the reviewboard service url (bug 1337517)

https://reviewboard.mozilla.org/r/114236/#review116420
Attachment #8839659 - Flags: review?(smacleod) → review+
Comment on attachment 8839660 [details]
autoland: write integration test for fake reviewboard service (bug 1337517)

https://reviewboard.mozilla.org/r/114238/#review116422
Attachment #8839660 - Flags: review?(smacleod) → review+
Comment on attachment 8839661 [details]
autoland: remove pycharm testrunner PYTHONPATH hack (bug 1337517)

https://reviewboard.mozilla.org/r/114240/#review116424
Attachment #8839661 - Flags: review?(smacleod) → review+
Pushed by smacleod@mozilla.com:
https://hg.mozilla.org/automation/conduit/rev/b3c719a269bb
autoland: add a configurable setting for the reviewboard service url r=smacleod
https://hg.mozilla.org/automation/conduit/rev/1dbee02d0b54
autoland: write integration test for fake reviewboard service r=smacleod
https://hg.mozilla.org/automation/conduit/rev/a7d58007c6c0
autoland: remove pycharm testrunner PYTHONPATH hack r=smacleod
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: