1358312 - Support testing of GitHub API requests

Status: RESOLVED FIXED

(Developer Services :: Servo VCS Sync, enhancement)

Description

[Gregory Szorc [:gps]]

We currently don't have good testing of functionality that touches the GitHub API. I have a trick up my sleeve to facilitate testing of GitHub API requests.

Comment 1

[Gregory Szorc [:gps]]

Attachment: vcssync: stop defining GitHub username (bug 1358312);

As part of refactoring code using github3 in a subsequent commit,
I realized that a username is not needed when using API tokens. The
code requires a username+password OR a token. Unless the password
is defined, the username argument is ignored.

So, stop defining and using a GitHub username value in the code and
in automation configs.

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

Comment 2

[Gregory Szorc [:gps]]

Attachment: vcssync: add utility function for obtaining a github3 client (bug 1358312);

We currently don't have test coverage of GitHub API requests. I'd like
to change that.

The first step is to create a function for obtaining a github3 client.
This will give us a central point from which we can inject special
behavior when running from tests.

While there is only one place where we obtain a github3 client today,
this will change in the near future. And the code will be sufficiently
complex to warrant a standalone function.

As part of this, I also snuck in a change to how the client is
constructed. ``github3.login()`` is equivalent to the new code,
at least for the usage we currently care about.

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

Comment 3

[Gregory Szorc [:gps]]

Attachment: vcssync: don't write tailing JSON (bug 1358312);

Python's built-in json serializer adds trailing whitespace after
commas by default. This makes sense for the default serialization
mode. But when putting each entry on its own line, this results in
trailing whitespace on lines.

This commit sets explicit separators when serializing so we don't
produce trailing whitespace. FWIW, I'm pretty sure Python 3 does
this nicer behavior by default.

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

Comment 4

[Gregory Szorc [:gps]]

Attachment: vcssync: use betamax for testing GitHub API interactions (bug 1358312);

The betamax Python package collaborates with the requests Python package
to record HTTP interactions so they can later be reused. Essentially, you
define a directory containing "cassettes," which are JSON files
holding recorded HTTP requests and responses. Using Betamax, you can
attach a cassette to a requests.Session. Depending on how Betamax is
configured, HTTP requests will actually be made and saved to the
cassette, will be immediately "served" from that cassette, or will
be rejected outright.

I see a number of compelling reasons to use Betamax over other
approaches. This is especially true when you can't host the service
you are talking to (such as the case with the GitHub API).

This commit establishes a framework for using Betamax to test GitHub
API interactions in the vcssync tests.

The functionality is explained more in the added documentation.

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

Comment 5

[:glob ✱]

Comment on attachment 8860224 [details]
vcssync: stop defining GitHub username (bug 1358312);

https://reviewboard.mozilla.org/r/132224/#review135588

Comment 6

[:glob ✱]

Comment on attachment 8860225 [details]
vcssync: add utility function for obtaining a github3 client (bug 1358312);

https://reviewboard.mozilla.org/r/132226/#review135590

Comment 7

[:glob ✱]

Comment on attachment 8860226 [details]
vcssync: don't write tailing JSON (bug 1358312);

https://reviewboard.mozilla.org/r/132228/#review135592

Comment 8

[:glob ✱]

Comment on attachment 8860227 [details]
vcssync: use betamax for testing GitHub API interactions (bug 1358312);

https://reviewboard.mozilla.org/r/132230/#review135596

::: vcssync/mozvcssync/util.py:57
(Diff revision 1)
> +            config.cassette_library_dir = betamax_library_dir
> +
> +            # We don't want requests hitting the network at all.
> +            config.default_cassette_options['record_mode'] = 'none'
> +
> +        recorder = betamax.Betamax(gh._session)

not a fan of accessing the private _session var, however until github3 1.0.0 is released we're stuck with it i guess.

::: vcssync/tests/record_cassettes.py:52
(Diff revision 1)
> +    with recorder.use_cassette('linearize-github-pull-request-messages'):
> +        # This test simply looks up a pull request and resolves more info about
> +        # the user who created it.
> +        client.login(token=args.token)
> +        client.pull_request('servo', 'servo', 16549)
> +        client.user('bholley')

in its current form it isn't obvious which file needs to be touched if a cassette needs to be updated due to the code performing additional requests.

the cassette recording actions for each test should be in their own file which shares the same name as the test/cassette (eg. tests/cassettes/linearize-github-pull-request-messages.py)

record_cassettes.py then becomes a wrapping harness for setting up betamax (and the github client?), then runs each cassette generator.

Comment 9

[Gregory Szorc [:gps]]

Comment on attachment 8860227 [details]
vcssync: use betamax for testing GitHub API interactions (bug 1358312);

https://reviewboard.mozilla.org/r/132230/#review135596

> not a fan of accessing the private _session var, however until github3 1.0.0 is released we're stuck with it i guess.

I was going to upgrade to github3 1.0.0. I wanted to have some testing in place before I did to prove certain things didn't break.

> in its current form it isn't obvious which file needs to be touched if a cassette needs to be updated due to the code performing additional requests.
> 
> the cassette recording actions for each test should be in their own file which shares the same name as the test/cassette (eg. tests/cassettes/linearize-github-pull-request-messages.py)
> 
> record_cassettes.py then becomes a wrapping harness for setting up betamax (and the github client?), then runs each cassette generator.

Yeah, this is how many projects do it. I figured it wasn't worth the initial complexity to stand up the infrastructure: I was very much going for MVP here.

Another alternative which I considered is not having a dedicated cassette recorder at all - at least for simple cases. We could have environment variables trigger a "recording mode" and then when you run a test in recording mode, it will write a cassette. This would prevent us from having to explicitly maintain cassette recordings that are shallow copies of functionality exercised in tests. Again, I thought it would be too difficult to initially implement.

Comment 10

[:glob ✱]

Comment on attachment 8860227 [details]
vcssync: use betamax for testing GitHub API interactions (bug 1358312);

https://reviewboard.mozilla.org/r/132230/#review135596

> Yeah, this is how many projects do it. I figured it wasn't worth the initial complexity to stand up the infrastructure: I was very much going for MVP here.
> 
> Another alternative which I considered is not having a dedicated cassette recorder at all - at least for simple cases. We could have environment variables trigger a "recording mode" and then when you run a test in recording mode, it will write a cassette. This would prevent us from having to explicitly maintain cassette recordings that are shallow copies of functionality exercised in tests. Again, I thought it would be too difficult to initially implement.

fair enough; i'll drop this issue with a MVP in mind.
once we have more than one test and/or project using betamax we should revisit.

Comment 11

[:glob ✱]

Comment on attachment 8860227 [details]
vcssync: use betamax for testing GitHub API interactions (bug 1358312);

https://reviewboard.mozilla.org/r/132230/#review137126

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/1c31dc81d838
vcssync: stop defining GitHub username ; r=glob
https://hg.mozilla.org/hgcustom/version-control-tools/rev/ff21afb2cbe6
vcssync: add utility function for obtaining a github3 client ; r=glob
https://hg.mozilla.org/hgcustom/version-control-tools/rev/862b38792a69
vcssync: don't write tailing JSON ; r=glob
https://hg.mozilla.org/hgcustom/version-control-tools/rev/f7f0fb021395
vcssync: use betamax for testing GitHub API interactions ; r=glob