Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add tests and make them run on CI #3

Merged
merged 10 commits into from Nov 22, 2016
Merged

Add tests and make them run on CI #3

merged 10 commits into from Nov 22, 2016

Conversation

JohanLorenzo
Copy link
Contributor

No description provided.

@JohanLorenzo JohanLorenzo force-pushed the add-test branch 3 times, most recently from a32ed8a to 1b0b143 Compare October 27, 2016 16:46
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 1b0b143 on JohanLorenzo:add-test into * on mozilla-releng:master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling dc92664 on JohanLorenzo:add-test into * on mozilla-releng:master*.

@JohanLorenzo JohanLorenzo mentioned this pull request Nov 1, 2016
Merged
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 1e354e0 on JohanLorenzo:add-test into * on mozilla-releng:master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 73f4b93 on JohanLorenzo:add-test into * on mozilla-releng:master*.

@JohanLorenzo JohanLorenzo force-pushed the add-test branch 3 times, most recently from 30793fc to 845bef2 Compare November 7, 2016 17:24
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 845bef2 on JohanLorenzo:add-test into * on mozilla-releng:master*.

@coveralls
Copy link

Coverage Status

Coverage increased (+70.0%) to 80.564% when pulling 3d261c9 on JohanLorenzo:add-test into b35e0ea on mozilla-releng:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+71.4%) to 81.972% when pulling 0447923 on JohanLorenzo:add-test into b35e0ea on mozilla-releng:master.

@JohanLorenzo
Copy link
Contributor Author

JohanLorenzo commented Nov 15, 2016 •
edited

@sylvestre noticed the division by 100 doesn't work on Python 2. This PR is the perfect place to add a test for it.

@coveralls
Copy link

Coverage Status

Coverage increased (+74.6%) to 85.153% when pulling f74c630 on JohanLorenzo:add-test into cd9f759 on mozilla-releng:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+74.6%) to 85.153% when pulling f74c630 on JohanLorenzo:add-test into cd9f759 on mozilla-releng:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+78.5%) to 89.08% when pulling 7b521ac on JohanLorenzo:add-test into cd9f759 on mozilla-releng:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+78.5%) to 89.08% when pulling b75355c on JohanLorenzo:add-test into cd9f759 on mozilla-releng:master.

@JohanLorenzo
Copy link
Contributor Author

JohanLorenzo commented Nov 21, 2016 •
edited

Follow up of #6 => We now have unit tests across the whole project.

The few bits missing are related to get_apk.py and googleplay.py. In the first case, things related to IO (as in "downloads and cleanups") are not tested yet. I don't believe this is a high priority as pushapkworker relies on scriptworker to perform downloads (including chain of trust, in the near future).
In the second case, I don't think unit tests are a smart way to proceed. The functions not unit tested in googleplay.py are one-liners (if we don't take logs into accounts) that directly call the googleplay client. Hence, I think it'd be better to have an integration test that performs a big dry-run. I'd like to handle that in a follow up. The current PR is already big enough (and I apologize for that)

Finally, I decided to go with python 2 for the coveralls reports. We sometimes have try/except imports, to be python 2.7 compatible. As a consequence, the python 3 report shows some missing lines, here and there.

r? @lundjordan

lundjordan
lundjordan approved these changes Nov 21, 2016
View reviewed changes
Copy link
Contributor

@lundjordan lundjordan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still new to all this code so from an eyeball check, everything looks good. huge improvements! And bonus, lot's of tests ++

If you want more confidence we are doing things logically correctly, we might want a 2nd reviewer. Otherwise, feel free to merge :)

mozapkpublisher/push_apk.py
https://github.com/mozilla-l10n/stores_l10n/issues/71). Skipping what\'s new.')
else:
self._push_whats_new(package_code, service, edit_id, apk_response)
apk_response = edit_service.upload_apk(apk_file_name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens if we fail to upload apk? does .execute() fatal out for us or should we catch key errors like 'versionCode' missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the documentation, it's unclear. I tried locally to upload an APK that was already uploaded. .execute() failed this way:

2016-11-22 10:29:16,844 - http.py - WARNING - Encountered 403 Forbidden with reason "apkUpgradeVersionConflict"
Traceback (most recent call last):
  File "../mozapkpublisher/push_apk.py", line 119, in <module>
    main(__name__)
  File "../mozapkpublisher/push_apk.py", line 112, in main
    PushAPK().run()
  File "../mozapkpublisher/push_apk.py", line 84, in run
    self.upload_apks(apks)
  File "../mozapkpublisher/push_apk.py", line 65, in upload_apks
    apk_response = edit_service.upload_apk(apk_file_name)
  File "/home/jlorenzo/git/mozilla-releng/mozapkpublisher/mozapkpublisher/googleplay.py", line 68, in _transaction_required
    return method(*args, **kwargs)
  File "/home/jlorenzo/git/mozilla-releng/mozapkpublisher/mozapkpublisher/googleplay.py", line 88, in upload_apk
    media_body=apk_path
  File "/home/jlorenzo/.virtualenvs/mozapkpublisher/lib/python3.5/site-packages/oauth2client-3.0.0-py3.5.egg/oauth2client/util.py", line 137, in positional_wrapper
  File "/home/jlorenzo/.virtualenvs/mozapkpublisher/lib/python3.5/site-packages/google_api_python_client-1.5.3-py3.5.egg/googleapiclient/http.py", line 838, in execute
googleapiclient.errors.HttpError: <HttpError 403 when requesting https://www.googleapis.com/upload/androidpublisher/v2/applications/org.mozilla.fennec_aurora/edits/16704049670753158544/apks?alt=json&uploadType=media returned "APK specifies a version code that has already been used.">

I believe it's safe enough, so we won't get non obvious errors like KeyError: 'versionCode'

mozapkpublisher/push_apk.py
editId=edit_id, packageName=self.config.package_name
).execute()
logger.info('Changes committed')
upload_body[u'userFraction'] = self.config.rollout_percentage / 100.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hehe, I remember this!

@JohanLorenzo
Copy link
Contributor Author

In addition to unit tests, I tested the refactored scripts locally. I believe 1 reviewer is okay, based on the amount of code changed in the non-test code.

@JohanLorenzo JohanLorenzo merged commit 18c2581 into mozilla-releng:master Nov 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lundjordan lundjordan lundjordan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@JohanLorenzo @coveralls @lundjordan