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.93k stars 564 forks source link

chrome://inspect?tracing not working #1884

Open primiano opened 8 years ago

primiano commented 8 years ago

Recently the tracing UI seems to fall in a broken state on a weekly basis. This week it's the case for chrome://inspect/?tracing Tried on ToT canary (49.0.2618.0).

Repro step:

What happens: Nothing happens (not even a JS warning)

What I expect: A dialog that prompts me which categories to enable etc etc.

primiano commented 8 years ago

@petrcermak @natduca @skyostil FYI

I'm going to create a crbug and mark as beta blocker. We cannot ship android without chrome://inspect?tracing.

As a more general comment, should we make tracing part, at very least, of the set of manual QA tests that we do before releases? The branch point is soon, and we might have end up shipping an untraceable chrome.

primiano commented 8 years ago

Nevermind the "and we might have end up shipping an untraceable chrome." See http://crbug.com/576292 it seems to be broken even in M47, so I guess we already shipped a broken one.

petrcermak commented 8 years ago

I'll give it a go :hammer:

petrcermak commented 8 years ago

The problem seems to be that neither resolver function inside InspectorTracingControllerClient.prototype.getCategories is ever called:

getCategories: function() {
  var res = this.conn_.req('Tracing.getCategories', {});  // This does get called.
  return res.then(function(result) {
    return result.categories;                             // This does NOT get called.
  }, function(err) {
    return [];                                            // This does NOT get called.
  });
},

I will continue my investigation in the morning (GMT).

natduca commented 8 years ago

Should we stand up a browser test for this?

On Mon, Jan 11, 2016 at 10:43 AM, petrcermak notifications@github.com wrote:

The problem seems to be that neither resolver function inside InspectorTracingControllerClient.prototype.getCategories is ever called:

getCategories: function() { var res = this.conn_.req('Tracing.getCategories', {}); // This does get called. return res.then(function(result) { return result.categories; // This does NOT get called. }, function(err) { return []; // This does NOT get called. }); },

I will continue my investigation in the morning (GMT).

— Reply to this email directly or view it on GitHub https://github.com/catapult-project/catapult/issues/1884#issuecomment-170648681 .

petrcermak commented 8 years ago

I agree with @primiano that we should definitely have end-to-end tests for tracing in Chrome. My impression (given the finding described in my last comment) is that this is a Chrome issue (rather than a Trace Viewer one).

Investigation still pending...

primiano commented 8 years ago

browser test is going to be tricky: we have to mock the presence of a device over adb and then poke around with the UI. My fear is that it's going to be too fake (due to the mocking) or too flaky (due to UI changes). That's why I was suggesting to add to the QA manual test. At least would give us coverage on the releases.

skyostil commented 8 years ago

Do we know how devtools tests chrome://inspect? Perhaps we just extend those tests.

natduca commented 8 years ago

Maybe ask pfeldman?

On Tue, Jan 12, 2016 at 6:19 AM, Sami Kyöstilä notifications@github.com wrote:

Do we know how devtools tests chrome://inspect? Perhaps we just extend those tests.

— Reply to this email directly or view it on GitHub https://github.com/catapult-project/catapult/issues/1884#issuecomment-170924949 .

petrcermak commented 8 years ago

I've just reassigned this to @anniesullie, who has recently been working on a chrome://tracing manual testing plan.