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

[BUG] Diff between screenshots is not present in the report #15802

Closed
AnatoliiHanziuk opened this issue Jul 20, 2022 · 11 comments · Fixed by #20208
Closed

[BUG] Diff between screenshots is not present in the report #15802

AnatoliiHanziuk opened this issue Jul 20, 2022 · 11 comments · Fixed by #20208

Comments

@AnatoliiHanziuk
Copy link

AnatoliiHanziuk commented Jul 20, 2022

Context:

  • Playwright Version: 1.23.0
  • Operating System: Mac
  • Node.js version: 16.14.0
  • Browser: Chromium

Code Snippet

Playwright config:

const config: PlaywrightTestConfig = {
  testDir: 'tests',
  fullyParallel: true,
  reporter: process.env.CI ? [['github'], ['html']] : [['list'], ['html', { open: 'on-failure' }]],
  forbidOnly: !!process.env.CI,
  retries: process.env.CI ? 1 : 0,
  use: {
    headless: true,
    viewport: { width: 1280, height: 720 },
    screenshot: 'only-on-failure',
    video: 'retain-on-failure',
    trace: 'retain-on-failure',
  },
  expect: {
    toHaveScreenshot: { 
      threshold: 0.2,
      maxDiffPixelRatio: 0.05
     },
  },
  projects: [
    {
      name: 'chromium',
      use: { ...devices['Desktop Chrome'] },
    },
  ],
};
export default config;

Method that compares screenshots:

async matchesScreenshot(
    locator: Locator,
    options?: LocatorScreenshotOptions,
  ) {
    expect(locator).toHaveScreenshot({timeout: 5000, ...options});
  }

Describe the bug

I'm doing visual regression testing. I want to see the difference between screenshots in the report. However, the difference is not present there. Also, in the test-results folder the difference is missing too.

@dgozman
Copy link
Contributor

dgozman commented Jul 20, 2022

@AnatoliiHanziuk Are you missing an await before the expect?

async matchesScreenshot(
    locator: Locator,
    options?: LocatorScreenshotOptions,
  ) {
    // Note the await.
    await expect(locator).toHaveScreenshot({timeout: 5000, ...options});
  }

@AnatoliiHanziuk
Copy link
Author

Good catch. However, the addition of the await doesn't solve the problem.
Here you can find the repository with the problem I described.

@dgozman
Copy link
Contributor

dgozman commented Jul 22, 2022

Thank you for the example repository. I just ran the tests, and html report shows me the images. Since the images are of a different size, the diff image is not available, but it will be once the sizes match.

html report

Also, you should await the page.setViewportSize() call.

@dgozman
Copy link
Contributor

dgozman commented Jul 25, 2022

We have to reproduce this issue locally to be able to debug it. If you can provide a full repro, please file a new issue with it, and link to this one.

@dgozman dgozman closed this as completed Jul 25, 2022
@muhammad-saleh
Copy link

Is it possible to do something here instead of just removing the diff tab?
Maybe show the diff between the two images and fill the shorter one with solid color?
Sometimes we update the UI and we could increase/decrease a margin that could affect the page's height.

Maybe at least keep the diff slider while aligning the actual and the expected screenshots to the top?

@dgozman
Copy link
Contributor

dgozman commented Jul 30, 2022

@muhammad-saleh Could you please share more details about your usecase?

Sometimes we update the UI and we could increase/decrease a margin that could affect the page's height.

Playwright has a fixed viewport by default, so changing page layout should not affect the size of page.screenshot(). Are you doing something special here?

@muhammad-saleh
Copy link

@dgozman My use case is that in our org we have a staging website and each PR gets its own environment and url. What I need is to do a visual regression test between the changes in the PR vs. the staging env. using full page screenshots.
When in the PR we change the styles and reduce for example overall margins or font size, this result that the actual and the expected screenshots have different heights. When this happens we get an error message saying that the diffing won’t happen because the actual screenshot is 1280x2000 for example and the expected is 1280x2200 and that removes the diff tab in the html report. does that make sense?

@dgozman
Copy link
Contributor

dgozman commented Aug 1, 2022

@muhammad-saleh Thank you for the explanation, this makes sense. So, this is happening because you use full-page screenshots. I'll reopen this issue for now.

@DanielStoica85
Copy link

DanielStoica85 commented Nov 7, 2022

I am experiencing the same issue, "diff" is missing in the generated report. I am also using full page screenshots: await expect(page).toHaveScreenshot({ fullPage: true });

Error: Screenshot comparison failed:
Expected an image 1280px by 879px, received 1280px by 873px. 
Call log:
  - expect.toHaveScreenshot with timeout 5000ms
  -   verifying given screenshot expectation
  - taking page screenshot
  -   disabled all CSS animations
  - Expected an image 1280px by 879px, received 1280px by 873px. 
  - waiting 100ms before taking screenshot
  - taking page screenshot
  -   disabled all CSS animations
  - 814714 pixels (ratio 0.73 of all image pixels) are different
  - waiting 250ms before taking screenshot
  - taking page screenshot
  -   disabled all CSS animations
  - captured a stable screenshot
  - Expected an image 1280px by 879px, received 1280px by 873px. 

Based on the changes that have been done on the test pages, we expected a different screenshot size. But this shouldn't disable the "diff" tab/screenshot, in my opinion.

Anything that can be done about this? Comparing pages manually by constantly switching from "actual" to "expected" and trying to catch those differences with your eyes is far from ideal and I would say it even defeats the purpose of using such a framework.

@anetrebskii
Copy link

If it helps someone, I have prepared an NPM package patch using https://www.npmjs.com/package/patch-package, that ignores difference in height but still requires the same width of snapshots.

diff --git a/node_modules/@playwright/test/node_modules/playwright-core/lib/utils/comparators.js b/node_modules/@playwright/test/node_modules/playwright-core/lib/utils/comparators.js
index a0a36b1..eaad391 100644
--- a/node_modules/@playwright/test/node_modules/playwright-core/lib/utils/comparators.js
+++ b/node_modules/@playwright/test/node_modules/playwright-core/lib/utils/comparators.js
@@ -10,6 +10,7 @@ var _utilsBundle = require("../utilsBundle");
 var _pixelmatch = _interopRequireDefault(require("../third_party/pixelmatch"));
 
 var _diff_match_patch = require("../third_party/diff_match_patch");
+const exp = require("constants");
 
 function _interopRequireDefault(obj) { return obj && obj.__esModule ? obj : { default: obj }; }
 
@@ -55,31 +56,48 @@ function compareImages(mimeType, actualBuffer, expectedBuffer, options = {}) {
   if (!actualBuffer || !(actualBuffer instanceof Buffer)) return {
     errorMessage: 'Actual result should be a Buffer.'
   };
-  const actual = mimeType === 'image/png' ? _utilsBundle.PNG.sync.read(actualBuffer) : _utilsBundle.jpegjs.decode(actualBuffer, {
+  var actual = mimeType === 'image/png' ? _utilsBundle.PNG.sync.read(actualBuffer) : _utilsBundle.jpegjs.decode(actualBuffer, {
     maxMemoryUsageInMB: JPEG_JS_MAX_BUFFER_SIZE_IN_MB
   });
-  const expected = mimeType === 'image/png' ? _utilsBundle.PNG.sync.read(expectedBuffer) : _utilsBundle.jpegjs.decode(expectedBuffer, {
+  var expected = mimeType === 'image/png' ? _utilsBundle.PNG.sync.read(expectedBuffer) : _utilsBundle.jpegjs.decode(expectedBuffer, {
     maxMemoryUsageInMB: JPEG_JS_MAX_BUFFER_SIZE_IN_MB
   });
 
-  if (expected.width !== actual.width || expected.height !== actual.height) {
-    return {
-      errorMessage: `Expected an image ${expected.width}px by ${expected.height}px, received ${actual.width}px by ${actual.height}px. `
-    };
+  if (expected.width !== actual.width) {
+     return {
+       errorMessage: `Expected an image ${expected.width}px by ${expected.height}px, received ${actual.width}px by ${actual.height}px. `
+     };
+   }
+
+  var expectedImageData = expected.data;
+  var actualImageData = actual.data;
+  var imageWidth = expected.width;
+  var imageHeight = Math.max(expected.height, actual.height);
+
+  if (actualImageData.length < expectedImageData.length) {
+    var tmp = Buffer.alloc(expectedImageData.length);
+    actualImageData.copy(tmp)
+    actualImageData = tmp;
+  }
+  else if (expectedImageData.length < actualImageData.length) {
+    var tmp = Buffer.alloc(actualImageData.length);
+    expectedImageData.copy(tmp)
+    expectedImageData = tmp;
   }
 
   const diff = new _utilsBundle.PNG({
-    width: expected.width,
-    height: expected.height
+    width: imageWidth,
+    height: imageHeight
   });
-  const count = (0, _pixelmatch.default)(expected.data, actual.data, diff.data, expected.width, expected.height, {
+  
+  const count = (0, _pixelmatch.default)(expectedImageData, actualImageData, diff.data, imageWidth, imageHeight, {
     threshold: (_options$threshold = options.threshold) !== null && _options$threshold !== void 0 ? _options$threshold : 0.2
   });
   const maxDiffPixels1 = options.maxDiffPixels;
-  const maxDiffPixels2 = options.maxDiffPixelRatio !== undefined ? expected.width * expected.height * options.maxDiffPixelRatio : undefined;
+  const maxDiffPixels2 = options.maxDiffPixelRatio !== undefined ? imageWidth * imageHeight * options.maxDiffPixelRatio : undefined;
   let maxDiffPixels;
   if (maxDiffPixels1 !== undefined && maxDiffPixels2 !== undefined) maxDiffPixels = Math.min(maxDiffPixels1, maxDiffPixels2);else maxDiffPixels = (_ref = maxDiffPixels1 !== null && maxDiffPixels1 !== void 0 ? maxDiffPixels1 : maxDiffPixels2) !== null && _ref !== void 0 ? _ref : 0;
-  const ratio = Math.ceil(count / (expected.width * expected.height) * 100) / 100;
+  const ratio = Math.ceil(count / (imageWidth * imageHeight) * 100) / 100;
   return count > maxDiffPixels ? {
     errorMessage: `${count} pixels (ratio ${ratio.toFixed(2)} of all image pixels) are different`,
     diff: _utilsBundle.PNG.sync.write(diff)

dgozman added a commit that referenced this issue Jan 21, 2023
… and produce the diff image (#20208)

Also show sizes in the html report to easier spot the size mismatch
issue.

<img width="1030" alt="diff"
src="https://user-images.githubusercontent.com/9881434/213327632-b8fcd69c-8d08-460c-9de1-b5f4f8c56359.png">

Fixes #15802.
@DanielStoica85
Copy link

When will 1.31 be released? Looking forward to this bug fix.

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