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

Current master tests fail with Chrome 56.0.2924.76 (stable) #1573

Closed
XhmikosR opened this issue Jan 29, 2017 · 12 comments
Closed

Current master tests fail with Chrome 56.0.2924.76 (stable) #1573

XhmikosR opened this issue Jan 29, 2017 · 12 comments

Comments

@XhmikosR
Copy link
Contributor

XhmikosR commented Jan 29, 2017

So, while trying to make AppVeyor to work, I noticed a failure when doing npm run smoke with Chrome 56.0.2924.76 from the stable channel.

xmr@xmr-PC ~
$ cd /c/Users/xmr/Desktop/lighthouse/

xmr@xmr-PC /c/Users/xmr/Desktop/lighthouse
$ npm run smoke

> lighthouse@1.4.1 smoke c:\Users\xmr\Desktop\lighthouse
> bash lighthouse-cli/test/smokehouse/offline-local/run-tests.sh && bash lighthouse-cli/test/smokehouse/dobetterweb/run-tests.sh

Checking 'http://localhost:10200/online-only.html'...
  √ final url: http://localhost:10200/online-only.html
  Correctly passed 26 audits

Checking 'http://localhost:10503/offline-ready.html'...
  √ final url: http://localhost:10503/offline-ready.html
  Correctly passed 26 audits

52 passing
Checking 'http://localhost:10200/dobetterweb/dbw_tester.html'...
  √ final url: http://localhost:10200/dobetterweb/dbw_tester.html
  × difference at uses-optimized-images.extendedInfo.value.results.length: found 2, expected 1
      full found result: {
        "score": false,
        "displayValue": "7KB (~30ms) potential savings",
        "rawValue": false,
        "extendedInfo": {
          "formatter": "table",
          "value": {
            "results": [
              {
                "url": "data:image/jpeg;base64,/9j/4AAQSkZJRgABAQEASABIAAD/2wBDAAMCAgMC…",
                "total": "5 KB",
                "webpSavings": "64%",
                "jpegSavings": "37%"
              },
              {
                "url": "data:image/jpeg;base64,/9j/4AAQSkZJRgABAQEASABIAAD/2wBDAAMCAgMC…",
                "total": "5 KB",
                "webpSavings": "64%",
                "jpegSavings": "37%"
              }
            ],
            "tableHeadings": {
              "url": "URL",
              "total": "Original (KB)",
              "webpSavings": "WebP Savings (%)",
              "jpegSavings": "JPEG Savings (%)"
            }
          }
        },
        "name": "uses-optimized-images",
        "category": "Images",
        "description": "Site uses optimized images",
        "helpText": "Images should be optimized to save network bytes. The following images could have smaller file sizes when compressed with [WebP](https://developers.google.com/speed/webp/) or JPEG at 80 quality. [Learn more about image optimization](https://developers.google.com/web/fundamentals/performance/optimizing-content-efficiency/image-optimization)."
      }
  Correctly passed 18 audits

Checking 'http://localhost:10200/online-only.html'...
  √ final url: http://localhost:10200/online-only.html
  Correctly passed 15 audits

33 passing
1 failing

This doesn't happen if I use the latest Chromium from https://download-chromium.appspot.com/dl/Win?type=snapshots so I doubt this is just a Windows issue.

My suggestion would be to also test with Chrome stable or beta so that such failures can be caught in CI in the future.

@patrickhulce
Copy link
Collaborator

patrickhulce commented Jan 30, 2017

I'll look into it thanks for finding! #1480 is relevant right about now :)

@XhmikosR
Copy link
Contributor Author

Yup, indeed, didn't noticed 1480 :)

@patrickhulce
Copy link
Collaborator

Yikes, and on Chrome 55 there are half a dozen more failures than that! All similar duplicate entries.

@XhmikosR
Copy link
Contributor Author

Yeah, on 55 I thought it was normal because it's missing the CSS usage stuff hence why I don't mention that.

All this I noticed with AppVeyor. I really believe we should merge #1280 ASAP.

@XhmikosR
Copy link
Contributor Author

@XhmikosR
Copy link
Contributor Author

So after #1579, there's 4 failing tests with v55, see the above link.

@XhmikosR
Copy link
Contributor Author

XhmikosR commented Feb 2, 2017

@patrickhulce: do you plan to fix the v55 issues too? If not we can close this after one updates the docs that v56 is required.

@XhmikosR
Copy link
Contributor Author

Bump...

You should either make v56 the minimum required version or fix tests with v55.

@patrickhulce
Copy link
Collaborator

patrickhulce commented Feb 10, 2017

Oh sorry, I was not planning on addressing the rest of v55 issues since v56 should already be out there for most folks by now. I can look into optimized image issue for 56 though.

@XhmikosR
Copy link
Contributor Author

XhmikosR commented Feb 10, 2017 via email

@XhmikosR
Copy link
Contributor Author

BTW I confirmed tests pass with current master and v56 stable.

So, this issue can be closed after someone mentions in the docs that v56 is required.

/CC @brendankenny

@XhmikosR
Copy link
Contributor Author

I'm gonna close this since readme.md mentions that v56 is required.

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

No branches or pull requests

2 participants