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.

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

Handle the case when MANIFEST.json is invalid JSON #10323

Merged
merged 1 commit into from Apr 5, 2018

Conversation

Projects
None yet
5 participants
@fergald
Contributor

fergald commented Apr 4, 2018

I don't know how I got into this situation (probably hit ctrl-C at the wrong time) but I did and it was unclear how to get out. It presents as a ValueError with no useful info.

Update the code to handle ValueError to log a warning and cause a full regen of the manifest.

Also reported in https://crbug.com/822041


This change is Reviewable

Handle the case when MANIFEST.json is empty
I don't know how I got into this situation (probably hit ctrl-C at the wrong time) but I did and it was unclear how to get out. It presents as a ValueError with no useful info.

Update the code to handle ValueError to log a warning and cause a full regen of the manifest.

Also reported in https://crbug.com/822041

@wpt-pr-bot wpt-pr-bot added the infra label Apr 4, 2018

@wpt-pr-bot wpt-pr-bot requested review from gsnedders and jgraham Apr 4, 2018

@@ -225,6 +225,9 @@ def load(tests_root, manifest):
rv = Manifest.from_json(tests_root, json.load(f))
except IOError:
return None
except ValueError:

This comment has been minimized.

@gsnedders

gsnedders Apr 4, 2018

Contributor

IMO, this should probably just capture Exception? @jgraham will probably disagree, so don't bother trying to be too prompt with addressing my comment, @fergald. 🙂

This comment has been minimized.

@fergald

fergald Apr 4, 2018

Contributor

I created another branch base on that suggestion.

https://github.com/fergald/web-platform-tests/tree/fergald-patch-2

Let me know what you think.

@fergald fergald changed the title from Handle the case when MANIFEST.json is empty to Handle the case when MANIFEST.json is invalid JSON Apr 4, 2018

@w3c-bots

This comment has been minimized.

w3c-bots commented Apr 5, 2018

Build PASSED

Started: 2018-04-05 00:51:04
Finished: 2018-04-05 01:06:54

View more information about this build on:

@jgraham

jgraham approved these changes Apr 5, 2018

@jgraham jgraham merged commit 384edc9 into web-platform-tests:master Apr 5, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

aarongable pushed a commit to chromium/chromium that referenced this pull request Apr 5, 2018

Roll in new version of WPT tools
This roll is mainly to include
web-platform-tests/wpt#10323, which fixes a bug
that `wpt manifest` would fail when the local manifest is invalid.

wpt.config.json is changed to accomodate:
web-platform-tests/wpt#9998
web-platform-tests/wpt#10078

Bug: 822041
Change-Id: I074afd90e32db299fc6f58b870f9b72576fab225
Reviewed-on: https://chromium-review.googlesource.com/997764
Reviewed-by: Quinten Yearsley <qyearsley@chromium.org>
Commit-Queue: Robert Ma <robertma@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548584}

aarongable pushed a commit to chromium/chromium that referenced this pull request Apr 6, 2018

[webkitpy] Improve some error handling and logging
This CL contains some code health improvements as a by product of fixing
issue 822041. Though the root cause of that issue was eventually fixed
from upstream (web-platform-tests/wpt#10323),
these improvements are still good to have.

* WPTManifest raises an exception when `wpt manifest` fails instead of
  directy exiting the process. Subprocess invocation is simplified.
* lint-test-expectations now has a --verbose flag to print debug logs,
  including error messages from subprocesses (i.e. `wpt manifest`).
* Use the canonical configure_logging in lint-test-expectations and
  LoggingTestCase in its unit test. Also make the unit test stricter as
  PRESUBMIT.py relies on the output.

Bug: 822041
Change-Id: I4724904c61b3c5b6ed3239584ed03089f2b203ce
Reviewed-on: https://chromium-review.googlesource.com/998341
Reviewed-by: Quinten Yearsley <qyearsley@chromium.org>
Commit-Queue: Robert Ma <robertma@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548632}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment