catapult-project / catapult

Deprecated Catapult GitHub. Please instead use http://crbug.com "Speed>Benchmarks" component for bugs and https://chromium.googlesource.com/catapult for downloading and editing source code..
https://chromium.googlesource.com/catapult
BSD 3-Clause "New" or "Revised" License
1.91k stars 563 forks source link

[📍] Better message when user runs a benchmark that doesn't exist. #4555

Open dave-2 opened 5 years ago

dave-2 commented 5 years ago

It's pretty common for users to either misspell benchmarks, or try to run a benchmark that no longer exists.

In this case, Pinpoint says, IOError: [Errno 2] No such file or directory: '/b/s/w/itt_QIq4/tmpLOfCV9telemetry/histograms.json' We should parse Telemetry's logs and pull out the No benchmark named "abc.xyz".

Example: https://pinpoint-dot-chromeperf.appspot.com/job/14bb6286a40000

amyqiu commented 5 years ago

@sadrulhc

Now that the smoothness/thread times benchmarks have been replaced by rendering.desktop and rendering.mobile, some users are confused when they try to run the old benchmarks. It would be great to have an error message that lets them know to use the rendering benchmarks instead, with "--story-tag-filter=___" to specify the page set. This could also include a link to documentation on the rendering benchmarks.

dave-2 commented 5 years ago

@nedn Sounds like that would be useful in Telemetry. If Telemetry produces the message, Pinpoint can plumb that message through to the user.

nedn commented 5 years ago

I would avoid parsing the log. Maybe something we can add to the json file? hmhh..

@dpranke what do we do for other test suite when the filtering flag doesn't match any test?

dpranke commented 5 years ago

I don't know that we have any consistent behavior here. I'd probably argue that it should be an immediate failure (as if it was an invalid command line flag).

dave-2 commented 5 years ago

If we want to avoid parsing the log, then we'd need the failure message in the JSON file. Pinpoint also parses the logs to pull out exception strings, so those would need to be in there too.

nedn commented 5 years ago

Maybe easiest to add s.t like --pinpoint-error-report=<foo.json> to harden the contract between benchmark & pinpoint. The key point is I think parsing the log can be very fragile