Skip to content
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

core: remove estimated-input-latency and first-cpu-idle #12553

Merged
merged 11 commits into from
May 26, 2021
Merged

Conversation

connorjclark
Copy link
Collaborator

ref #11866

@connorjclark connorjclark requested a review from a team as a code owner May 24, 2021 23:19
@connorjclark connorjclark requested review from patrickhulce and removed request for a team May 24, 2021 23:19
@google-cla google-cla bot added the cla: yes label May 24, 2021
'first-cpu-idle': {
numericValue: '>2000',
},
// TODO: ?
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should I replace these smoke expectations with something else?

Copy link
Member

Choose a reason for hiding this comment

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

nah. FCI is basically TTI and TTI is already there.

Copy link
Member

Choose a reason for hiding this comment

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

delete?

@brendankenny
Copy link
Member

what happened to debug.json?

@connorjclark
Copy link
Collaborator Author

I started just removing the EIL/FCI references then decided to just strip the entire thing but what is needed for treemap

@brendankenny
Copy link
Member

could you split that and we could just land that quickly independently of other changes?

e.g. I updated individual lines in debug.json in #12554 and I'd rather just have that over and done with rather than waiting for this PR to land :)

Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

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

a few more things you'll see if you search FCPUI.

  • There's a few errors that appear to be safe to delete. (they were never in the proto
  • And there's a block in lantern-baseline-accuracy.json that you'll need to delete.

(and the devtools tests need a rebaseline)

'first-cpu-idle': {
numericValue: '>2000',
},
// TODO: ?
Copy link
Member

Choose a reason for hiding this comment

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

nah. FCI is basically TTI and TTI is already there.

'first-cpu-idle': {
score: '>=0.90', // primarily just making sure it didn't fail/go crazy, specific value isn't that important
},
// TODO ?
Copy link
Member

Choose a reason for hiding this comment

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

flagging to remove

@@ -17,10 +17,11 @@ module.exports = [
requestedUrl: 'http://localhost:10200/tricky-tti.html',
finalUrl: 'http://localhost:10200/tricky-tti.html',
audits: {
'first-cpu-idle': {
Copy link
Member

Choose a reason for hiding this comment

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

on these you could add assertions for TBT if you want, but i don't think that's necessary or a great test for that metric.

so i'm +1 on removing in this file too

@patrickhulce
Copy link
Collaborator

@paulirish has got this one under control 👍

@patrickhulce patrickhulce removed their assignment May 25, 2021
Copy link
Member

@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.

one last TODO to remove and looks like need to run yarn update:test-devtools one more time, but otherwise looks good to ship

nice work in all of those (many) test files

'first-cpu-idle': {
numericValue: '>2000',
},
// TODO: ?
Copy link
Member

Choose a reason for hiding this comment

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

delete?

assert.equal(EstimatedInputLatency.calculateRollingWindowEIL(events), 4516);
});

it('handles continuous tasks', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

good stuff in here. Oh well :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants