Skip to content

core(devtools-model): fix missing Runtime.experiments object#3514

Merged
paulirish merged 1 commit intomasterfrom
dtmfailures
Oct 9, 2017
Merged

core(devtools-model): fix missing Runtime.experiments object#3514
paulirish merged 1 commit intomasterfrom
dtmfailures

Conversation

@paulirish
Copy link
Member

@paulirish paulirish commented Oct 9, 2017 •
edited by brendankenny
Loading

  1. The changes in web-inspector fix Perform 'yarn test' abnormal #3509 and the current yarn test breakage on master due to changes in web-inspector: fall back to page's Runtime and queryParam() #3497
  2. The DTM changes handle the silent failure coming from mocha. One alternative to this fix is removing consoleQuieter entirely, which I'm open to. It was introduced to handle cases like TracingStartedInPage event not found. paulirish/pwmetrics#3. @brendankenny thoughts?

@paulirish paulirish changed the title core(devtools-model): fix missing Runtime.experiments core(devtools-model): fix devtool's missing Runtime.experiments Oct 9, 2017
@paulirish paulirish changed the title core(devtools-model): fix devtool's missing Runtime.experiments core(devtools-model): fix devtools' missing Runtime.experiments Oct 9, 2017
@paulirish paulirish changed the title core(devtools-model): fix devtools' missing Runtime.experiments core(devtools-model): fix the missing Runtime.experiments Oct 9, 2017
@paulirish paulirish changed the title core(devtools-model): fix the missing Runtime.experiments core(devtools-model): fix missing Runtime.experiments object Oct 9, 2017
@paulirish paulirish changed the title core(devtools-model): fix missing Runtime.experiments object core(devtools-model): fix missing Runtime.experiments obj Oct 9, 2017
@paulirish paulirish changed the title core(devtools-model): fix missing Runtime.experiments obj core(devtools-model): fix missing Runtime.experiments object Oct 9, 2017
@paulirish paulirish requested a review from brendankenny October 9, 2017 19:30
@paulirish paulirish added the P0 label Oct 9, 2017
patrickhulce
patrickhulce reviewed Oct 9, 2017
View reviewed changes
lighthouse-core/lib/traces/devtools-timeline-model.js
this._timelineModel.setEvents(this._tracingModel);
ConsoleQuieter.unmuteAndFlush();
try {
ConsoleQuieter.mute({prefix: 'timelineModel'});
Copy link
Collaborator

@patrickhulce patrickhulce Oct 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does this do?

Copy link
Contributor

@brendankenny brendankenny Oct 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does this do?

any console messages that are absorbed are later logged with that as the logger prefix/title

Copy link
Collaborator

@patrickhulce patrickhulce Oct 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh I didn't mean this particular line, just why is this try/finally necessary now? we're not catching anything the connection to the mocha failure isn't clear to me, so here to learn :)

Copy link
Member Author

@paulirish paulirish Oct 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previous to this patch landing, when you run mocha lighthouse-core/test/lib/traces/devtools-timeline-model-test.js it just exits silently. That's because console-quieter was also muting the logs from mocha. They never got flushed (the finally) because of an exception.

Copy link
Collaborator

@patrickhulce patrickhulce Oct 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh gotchaaaa, mocha really just calls console.log? I see

Copy link
Member Author

@paulirish paulirish Oct 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yah it does. found that this morning. was kinda surprised they aren't doing process.std*.write(), but yeah. :)

@patrickhulce
Copy link
Collaborator

patrickhulce commented Oct 9, 2017

we should really make it a P0 to fix or disable appveyor so we can trust checks and require green before merging again

@brendankenny
Copy link
Contributor

brendankenny commented Oct 9, 2017

One alternative to this fix is removing consoleQuieter entirely, which I'm open to

having all of Lighthouse follow --quiet is really nice. I don't think we should go back to that old world (though at least that particular case is to stderr like lighthouse-logger)

@brendankenny
Copy link
Contributor

brendankenny commented Oct 9, 2017 •
edited
Loading

we should really make it a P0 to fix or disable appveyor so we can trust checks and require green before merging again

isn't appveyor fixed with #3498 now? It would be nice if the appveyor badge in the readme linked to the latest master build and not just the latest build, though...

brendankenny
brendankenny approved these changes Oct 9, 2017
View reviewed changes
Copy link
Contributor

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@paulirish paulirish merged commit b98e8a3 into master Oct 9, 2017
@paulirish paulirish deleted the dtmfailures branch October 9, 2017 21:05
@patrickhulce
Copy link
Collaborator

patrickhulce commented Oct 9, 2017

isn't appveyor fixed with #3498 now? It would be nice if the appveyor badge in the readme linked to the latest master build and not just the latest build, though...

it is, but I'm talking about appveyor breakages in general. I'm betting Paul fell into my similar camp of "since all the builds are always red anyhow, this red build telling me I shouldn't merge is meaningless, so I'm just gonna merge now"

if red builds weren't pretty regularly crying wolf, then we'd be more likely to investigate and postpone a merge because of a red build

@brendankenny
Copy link
Contributor

brendankenny commented Oct 9, 2017

if red builds weren't pretty regularly crying wolf, then we'd be more likely to investigate and postpone a merge because of a red build

ah, got it, yes definitely agree

@wardpeet wardpeet mentioned this pull request Oct 24, 2017
Merged
@ghost ghost mentioned this pull request Dec 17, 2017
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@patrickhulce patrickhulce patrickhulce left review comments

@brendankenny brendankenny brendankenny approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Perform 'yarn test' abnormal

3 participants

@paulirish @patrickhulce @brendankenny