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

4.3.1 #8750

Merged
merged 14 commits into from
May 2, 2019
Merged

4.3.1 #8750

merged 14 commits into from
May 2, 2019

Conversation

connorjclark
Copy link
Collaborator

@connorjclark connorjclark commented Apr 30, 2019

v4 branch is currently identical to tag v4.3.0

This PR proposes adding these cherrypicks to the v4 branch:

#7122
#8294
#8338
#8354
#8387

No merge conflicts :)

Fixes #8447

@googlebot

This comment has been minimized.

@connorjclark connorjclark changed the title V4 431 backport 4.3.1 backports Apr 30, 2019
@connorjclark connorjclark changed the base branch from master to v4 April 30, 2019 18:19
@connorjclark
Copy link
Collaborator Author

should be g2g, if tests pass.

@paulirish
Copy link
Member

1 smoketest failure

node lighthouse-cli/index.js http://localhost:10200/byte-efficiency/tester.html --config-path=/home/travis/build/GoogleChrome/lighthouse/lighthouse-cli/test/smokehouse/byte-config.js --output-path=smokehouse-37174.report.json --output=json --quiet --port=0 
Asserting expected results match those found. (http://localhost:10200/byte-efficiency/tester.html)
  ✘ difference at unused-javascript.details.overallSavingsBytes
              expected: ">=25000"
                 found: 21018
          found result:
      {
        "id": "unused-javascript",
        "title": "Remove unused JavaScript",
        "description": "Remove unused JavaScript to reduce bytes consumed by network activity.",
        "score": 0.67,
        "scoreDisplayMode": "numeric",
        "rawValue": 450,
        "displayValue": "Potential savings of 21 KB",
        "details": {
          "type": "opportunity",
          "headings": [
            {
              "key": "url",
              "valueType": "url",
              "label": "URL"
            },
            {
              "key": "totalBytes",
              "valueType": "bytes",
              "label": "Size (KB)"
            },
            {
              "key": "wastedBytes",
              "valueType": "bytes",
              "label": "Potential Savings (KB)"
            }
          ],
          "items": [
            {
              "url": "http://localhost:10200/byte-efficiency/script.js",
              "totalBytes": 53181,
              "wastedBytes": 14415,
              "wastedPercent": 27.105307847614018
            },
            {
              "url": "http://localhost:10200/byte-efficiency/tester.html",
              "totalBytes": 15089,
              "wastedBytes": 6603,
              "wastedPercent": 43.76043266341724
            }
          ],
          "overallSavingsMs": 450,
          "overallSavingsBytes": 21018
        }
      }

@patrickhulce
Copy link
Collaborator

That failure looks suspiciously like that chrome bug from forever ago it didn't make it to stable, right?

the backports doesn't have our chrome version printing in travis so hard to tell :)

@connorjclark
Copy link
Collaborator Author

That failure looks suspiciously like that chrome bug from forever ago it didn't make it to stable, right?

#7065 (comment)
yup exactly the same

@connorjclark
Copy link
Collaborator Author

connorjclark commented Apr 30, 2019

but the repro doesnt happen for me in stable ( https://bugs.chromium.org/p/chromium/issues/detail?id=927464 )

so that must mean travis is NOT using stable chrome ..

@connorjclark
Copy link
Collaborator Author

connorjclark commented Apr 30, 2019

pushed a commit to print the chrome version in travis.

Google Chrome 74.0.3729.131. Which is indeed stable..

Could the smokes somehow be using a Chrome other than the one defined by:

export CHROME_PATH="$(which google-chrome-stable)"

?

@connorjclark
Copy link
Collaborator Author

connorjclark commented Apr 30, 2019

coverage (in stable) for the failing bytes tester.html fixture:

image

image

lgtm

@connorjclark
Copy link
Collaborator Author

I'm 99% convinced Travis is using an old, broken Canary.

@connorjclark
Copy link
Collaborator Author

connorjclark commented May 1, 2019


diff --git a/lighthouse-core/audits/byte-efficiency/unused-javascript.js b/lighthouse-core/audits/byte-efficiency/unused-javascript.js
index f13e0272..2d6e33a7 100644
--- a/lighthouse-core/audits/byte-efficiency/unused-javascript.js
+++ b/lighthouse-core/audits/byte-efficiency/unused-javascript.js
@@ -50,11 +50,14 @@ class UnusedJavaScript extends ByteEfficiencyAudit {
     const unusedByIndex = new Uint8Array(maximumEndOffset);
     for (const func of script.functions) {
       for (const range of func.ranges) {
+        let c = 0;
         if (range.count === 0) {
           for (let i = range.startOffset; i < range.endOffset; i++) {
             unusedByIndex[i] = 1;
+            c++;
           }
         }
+        console.log(func.functionName, range.count, c);
       }
     }

  status Evaluating: Remove unused JavaScript +3ms
 1 0
generateInlineStyleWithSize 1 0
longTask 1 0
 1 0
 1 0
 1 0
 1 0
 1 0
unusedFunction 0 6554
 1 0
anUnusedFunction 0 14365
aUsedFunction 1 0

notice aNestedUnusedFunction is missing, which accounts for 7875 missed unused bytes.

@connorjclark
Copy link
Collaborator Author

connorjclark commented May 1, 2019

running master LH:

 1 0
generateInlineStyleWithSize 1 0
longTask 1 0
 1 0
 1 0
 1 0
 1 0
 1 0
unusedFunction 0 6554
 1 0
anUnusedFunction 0 14365
aUsedFunction 1 0
aNestedUnusedFunction 0 7875 <<<<<<<<<<<<<

hmmm....

no diff in the script.js / audit between master/v430 .

@connorjclark
Copy link
Collaborator Author

We gonna just change the smoke test value to pass. The unused-javascript audit is not in default config and won't ever run in DT (which will be shipped with 4.3.1 for a while).

There may be some sort of big to file upstream but it won't affect LH since the async stacks PR does something to fix things.

As for why the errors smoke test fails ... It works locally, maybe it's just flaky. Let's try again.

@paulirish paulirish changed the title 4.3.1 backports 4.3.1 May 2, 2019
@paulirish paulirish merged commit 5c48bb8 into v4 May 2, 2019
@paulirish paulirish deleted the v4-431-backport branch May 2, 2019 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants