Migrate Android Java E2E tests from App Center to Browserstack #22117
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| args = parser.parse_args() | ||
|
|
||
|
|
||
| def post_response_to_json(response): |
There was a problem hiding this comment.
nit: the name is post_reponse_to_json but this function is also called with a GET response. maybe response_to_json would be a more general name.
| def upload_apk_parse_json(post_url, apk_path): | ||
| with open(apk_path, "rb") as apk_file: | ||
| response = requests.post(post_url, files={"file": apk_file}, auth=(args.id, args.token)) | ||
| return post_response_to_json(response.text) |
There was a problem hiding this comment.
should we also check response.status_code? e.g., this method might be useful:
https://docs.python-requests.org/en/latest/api/#requests.Response.raise_for_status
There was a problem hiding this comment.
there's also a response method for parsing json which may simplify things:
https://docs.python-requests.org/en/latest/api/#requests.Response.json
|
|
||
| import requests | ||
|
|
||
| parser = argparse.ArgumentParser("Upload and run BrowserStack tests") |
There was a problem hiding this comment.
we usually want to put the script logic to execute inside the check if __name__ == '__main__'. then, it only gets executed when the script is called directly and not in other contexts, like when the script is imported by something else.
more details here: https://docs.python.org/3/library/__main__.html
| parser = argparse.ArgumentParser("Upload and run BrowserStack tests") | ||
|
|
||
| parser.add_argument("--id", type=str, help="BrowserStack user ID") | ||
| parser.add_argument("--token", type=str, help="BrowserStack token") |
There was a problem hiding this comment.
is this value secret?
passing it on the command line might be fine in the context of a CI pipeline where it is redacted from logs, but command line arguments are generally not hidden. e.g., when running locally, the command line arguments will be visible to other processes and also in the command history.
some alternatives are to pass the secret value via stdin or in an environment variable.
if we don't expect to run this script much locally, this is less of a concern.
|
|
||
| parser.add_argument("--id", type=str, help="BrowserStack user ID") | ||
| parser.add_argument("--token", type=str, help="BrowserStack token") | ||
| parser.add_argument("--test_platform", type=str, help="Testing platform, i.e., XCUITest or Espresso") |
There was a problem hiding this comment.
if there are two valid options, can we just constrain them here to catch typos earlier and with an obvious error message?
e.g., choices=['espresso', ...]
carzh
force-pushed
the
carzh/browserstack-android-test
branch
from
2347315
to
1a2ae52
Compare
| --id $(browserstack_username) \ | ||
| --token $(browserstack_access_key) \ |
There was a problem hiding this comment.
Is there WIP documentation somewhere that provides the high level details of how this all works and where these values are defined so that we know how to update them in the future?
| if "error" in json_response: | ||
| raise Exception(json_response["error"]) |
There was a problem hiding this comment.
Are there specific json properties we can check for 'error' instead of matching across the entire response?
|
|
||
| def upload_apk_parse_json(post_url, apk_path): | ||
| with open(apk_path, "rb") as apk_file: | ||
| response = requests.post(post_url, files={"file": apk_file}, auth=(args.id, args.token)) |
There was a problem hiding this comment.
Should we configure specific timeouts based on the operation? I can imagine apk upload may take longer than what the default timeout (assuming there is one) allows for.
| while tests_status == "running": | ||
| time.sleep(30) |
There was a problem hiding this comment.
Should we have an overall timeout for this loop?
| ) | ||
| print("=" * 30) | ||
| if tests_status != "passed": | ||
| raise Exception("Tests failed. Go to the link above for more details.") |
There was a problem hiding this comment.
nit: include the link directly as there could (in theory) be other output when the CI runs that makes 'above' unclear.
Description
Motivation and Context
App Center is getting deprecated in the next year + we have upcoming Android work that depends on working E2E testing.