1337517 - Write integration test for fake reviewboard service

Status: RESOLVED FIXED

(Conduit :: General, defect)

Description

[Māris Fogels [:mars] (please needinfo)]

Trello story card: https://trello.com/c/67hsrGO1

Comment 1

[Māris Fogels [:mars] (please needinfo)]

Attachment: autoland: add a configurable setting for the reviewboard service url (bug 1337517)

Add a configurable setting for the Reviewboard service URL so that application and test code can use it.

Review commit: https://reviewboard.mozilla.org/r/114236/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/114236/

Comment 2

[Māris Fogels [:mars] (please needinfo)]

Attachment: autoland: write integration test for fake reviewboard service (bug 1337517)

Add a fake reviewboard service to the test suite and use it to test repo URL handling.  The fake reviewboard is filled with junk data until we can get some real test data to work with.

Review commit: https://reviewboard.mozilla.org/r/114238/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/114238/

Comment 3

[Māris Fogels [:mars] (please needinfo)]

Attachment: autoland: remove pycharm testrunner PYTHONPATH hack (bug 1337517)

Removed the pycharm testrunner PYTHONPATH hack in our test suite.  The tests now pass without it.

Review commit: https://reviewboard.mozilla.org/r/114240/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/114240/

Comment 4

[Steven MacLeod [:smacleod]]

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):
> [e-mail]('--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?

Comment 5

[Steven MacLeod [:smacleod]]

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.

Comment 6

[Steven MacLeod [:smacleod]]

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`

Comment 7

[Māris Fogels [:mars] (please needinfo)]

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 8

[Māris Fogels [:mars] (please needinfo)]

Comment on attachment 8839659 [details]
autoland: add a configurable setting for the reviewboard service url (bug 1337517)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114236/diff/1-2/

Comment 9

[Māris Fogels [:mars] (please needinfo)]

Comment on attachment 8839660 [details]
autoland: write integration test for fake reviewboard service (bug 1337517)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114238/diff/1-2/

Comment 10

[Māris Fogels [:mars] (please needinfo)]

Comment on attachment 8839661 [details]
autoland: remove pycharm testrunner PYTHONPATH hack (bug 1337517)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114240/diff/1-2/

Comment 11

[Māris Fogels [:mars] (please needinfo)]

Comment on attachment 8839659 [details]
autoland: add a configurable setting for the reviewboard service url (bug 1337517)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114236/diff/2-3/

Comment 12

[Māris Fogels [:mars] (please needinfo)]

Comment on attachment 8839660 [details]
autoland: write integration test for fake reviewboard service (bug 1337517)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114238/diff/2-3/

Comment 13

[Māris Fogels [:mars] (please needinfo)]

Comment on attachment 8839661 [details]
autoland: remove pycharm testrunner PYTHONPATH hack (bug 1337517)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114240/diff/2-3/

Comment 14

[Steven MacLeod [:smacleod]]

Comment on attachment 8839659 [details]
autoland: add a configurable setting for the reviewboard service url (bug 1337517)

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

Comment 15

[Steven MacLeod [:smacleod]]

Comment on attachment 8839660 [details]
autoland: write integration test for fake reviewboard service (bug 1337517)

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

Comment 16

[Steven MacLeod [:smacleod]]

Comment on attachment 8839661 [details]
autoland: remove pycharm testrunner PYTHONPATH hack (bug 1337517)

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

Comment 17

[Pulsebot]

Pushed by [e-mail]:
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