Copy dashboard should skip downloading test data if response is not 200

RESOLVED FIXED

Status

Testing Graveyard
Eideticker
RESOLVED FIXED
4 years ago
5 months ago

People

(Reporter: davehunt, Assigned: wlach)

Tracking

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
It would appear that we have previously synced a tests.json file but no test data for the Flame device. When we copy the dashboard we're blindly writing the response content for the test data to disk, even if the response is a 404. We should only store the content if the response is a successful status code.

See http://docs.python-requests.org/en/latest/user/quickstart/#response-status-codes we can use `r.status_code == requests.codes.ok`
Created attachment 8437842 [details] [diff] [review]
Patch

This checks for invalid json as well as non-200 response codes.
Assignee: nobody → wlachance
Attachment #8437842 - Flags: review?(dave.hunt)
(Reporter)

Comment 2

4 years ago
Comment on attachment 8437842 [details] [diff] [review]
Patch

Review of attachment 8437842 [details] [diff] [review]:
-----------------------------------------------------------------

Could we also do the same check when getting tests.json here: https://github.com/mozilla/eideticker/blob/684e42e355b54033241b364b3bf753f383d62dbf/bin/copy-dashboard.py#L111 r=me with that addressed.
Attachment #8437842 - Flags: review?(dave.hunt) → review+
(Reporter)

Updated

4 years ago
Blocks: 1020215
Created attachment 8440084 [details] [diff] [review]
Expanded patch

I expanded the checks to test everything we download, as well as to return a non-zero return code if something wasn't there. I think these changes are worth it, but the result is a substantially different patch that probably needs re-review.
Attachment #8437842 - Attachment is obsolete: true
Attachment #8440084 - Flags: review?(dave.hunt)
(Reporter)

Comment 4

4 years ago
Comment on attachment 8440084 [details] [diff] [review]
Expanded patch

Review of attachment 8440084 [details] [diff] [review]:
-----------------------------------------------------------------

A couple of comments, but this looks good to me. I also tested this against the issues we're having with Flame, and it worked well.

::: bin/copy-dashboard.py
@@ +20,5 @@
> +def validate_response(r):
> +    global exit_status
> +
> +    if r.status_code != requests.codes.ok:
> +        print "WARNING: Problem downloading testdata at URL %s (HTTP " \

As this applies to all downloads now, maybe we should remove the reference to 'testdata'.

@@ +36,5 @@
> +    try:
> +        json.loads(r.text)
> +    except ValueError:
> +        exit_status = 1
> +        print "WARNING: Testdata at URL %s is not valid json" % r.url

As above, we can probably remove the reference to 'Testdata' as this applies to all JSON downloads now.
Attachment #8440084 - Flags: review?(dave.hunt) → review+
Pushed with suggested changes: https://github.com/mozilla/eideticker/commit/6742247f3df94a93d52c5da1efb059576619e190
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED

Updated

5 months ago
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.