Loading screenshots and simple 1-to-1 comparison of screenshots

chrome/browser/chromeos/login/screenshot_tester.cc

Index: chrome/browser/chromeos/login/screenshot_tester.cc
diff --git a/chrome/browser/chromeos/login/screenshot_tester.cc b/chrome/browser/chromeos/login/screenshot_tester.cc
index 683f987d6d0f82c460edeaf1d1ef6841bf16722d..a90f105a77ec1d39c1fa117cc971c40d2e30cbae 100644
--- a/chrome/browser/chromeos/login/screenshot_tester.cc
+++ b/chrome/browser/chromeos/login/screenshot_tester.cc
@@ -4,10 +4,14 @@
 
 #include "chrome/browser/chromeos/login/screenshot_tester.h"
 
+#include <algorithm>
+#include <string>

[ygorshenin1, 2014/07/31 13:19:14]

nit: remove <string> inclusion since you already included it in a header file.

[Lisa Ignatyeva, 2014/07/31 16:37:39]

Done.

+
 #include "ash/shell.h"
 #include "base/base_export.h"
 #include "base/bind_internal.h"
 #include "base/command_line.h"
+#include "base/logging.h"
 #include "base/memory/weak_ptr.h"
 #include "base/prefs/pref_service.h"
 #include "base/run_loop.h"
@@ -15,13 +19,17 @@
 #include "chrome/common/pref_names.h"
 #include "chromeos/chromeos_switches.h"
 #include "content/public/browser/browser_thread.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/skia/include/core/SkBitmap.h"
+#include "third_party/skia/include/core/SkCanvas.h"
 #include "ui/compositor/compositor_switches.h"
+#include "ui/gfx/codec/png_codec.h"
 #include "ui/gfx/image/image.h"
 #include "ui/snapshot/snapshot.h"
 
 namespace chromeos {
 
-ScreenshotTester::ScreenshotTester() : weak_factory_(this) {
+ScreenshotTester::ScreenshotTester() : test_mode_(false), weak_factory_(this) {
 }
 
 ScreenshotTester::~ScreenshotTester() {
@@ -29,7 +37,7 @@ ScreenshotTester::~ScreenshotTester() {
 
 bool ScreenshotTester::TryInitialize() {
   CommandLine& command_line = *CommandLine::ForCurrentProcess();
-  if (!command_line.HasSwitch(switches::kEnableScreenshotTesting))
+  if (!command_line.HasSwitch(switches::kEnableScreenshotTestingWithMode))
     return false;
   if (!command_line.HasSwitch(::switches::kEnablePixelOutputInTests) ||
       !command_line.HasSwitch(::switches::kUIEnableImplSidePainting)) {
@@ -40,64 +48,172 @@ bool ScreenshotTester::TryInitialize() {
                << "screenshots";
     return false;
   }
-  if (!command_line.HasSwitch(switches::kScreenshotDestinationDir)) {
-    LOG(ERROR) << "No destination for screenshots specified";
-    return false;
+
+  std::string mode = command_line.GetSwitchValueASCII(
+      switches::kEnableScreenshotTestingWithMode);
+  if (mode != "update" && mode != "test") {
+    LOG(ERROR) << "Invalid mode for screenshot testing: " << mode;
+    DCHECK(false);

[ygorshenin1, 2014/07/31 13:19:14]

I thought it's better to return false instead of DCHECK(false). The same for
line 61.

[Lisa Ignatyeva, 2014/07/31 16:37:39]

That's actually a question of strictness. I believe that wrong usage of these
switches (==usage that makes screenshot testing impossible to work) should cause
an immediate fail of the whole process. Returning false will change that to just
turning screenshot testing off if something went wrong during initialization,
and by now I think that makes less sense.

[ygorshenin1, 2014/07/31 16:50:17]

So you should replace DCHECK() by CHECK(), because DCHECK's are triggered only
in debug builds. Moreover, you could merge two lines to one: CHECK(false) << "No
directory ...";

[Lisa Ignatyeva, 2014/07/31 16:58:39]

Done.

+  }
+
+  if (!command_line.HasSwitch(chromeos::switches::kGoldenScreenshotsDir)) {
+    LOG(ERROR) << "No directory for golden screenshots specified";
+    DCHECK(false);
+  }
+
+  golden_screenshots_dir_ =
+      command_line.GetSwitchValuePath(switches::kGoldenScreenshotsDir);
+
+  if (mode == "test") {

[ygorshenin1, 2014/07/31 13:19:14]

Could you please define a couple of string constants in an anonymous namespace
for "update" and "test", like, for instance:

namespace {
const char kUpdateMode[] = "update";
const char kTestMode[] = "test";
}  // namespace

and compare |mode| with them?

[Lisa Ignatyeva, 2014/07/31 16:37:39]

Done.

+    test_mode_ = true;
+    if (!command_line.HasSwitch(switches::kArtifactsDir)) {
+      artifacts_dir_ = golden_screenshots_dir_;
+      LOG(WARNING)
+          << "No directory for artifact storing specified. Artifacts will be"
+          << "saved at golden screenshots directory.";
+    } else {
+      artifacts_dir_ = command_line.GetSwitchValuePath(switches::kArtifactsDir);
+    }
   }
-  screenshot_dest_ = command_line.GetSwitchValuePath(
-      chromeos::switches::kScreenshotDestinationDir);
   return true;
 }
 
-void ScreenshotTester::Run(const std::string& file_name) {
-  TakeScreenshot();
-  PNGFile current_screenshot = screenshot_;
-  UpdateGoldenScreenshot(file_name, current_screenshot);
+void ScreenshotTester::Run(const std::string& test_name) {
+  test_name_ = test_name;
+  PNGFile current_screenshot = TakeScreenshot();
+  if (test_mode_) {
+    PNGFile golden_screenshot = LoadGoldenScreenshot();
+    VLOG(0) << "Loaded golden screenshot";
+    CompareScreenshots(golden_screenshot, current_screenshot);
+  } else {
+    UpdateGoldenScreenshot(current_screenshot);
+    return;
+  }
+}
+
+void ScreenshotTester::UpdateGoldenScreenshot(PNGFile png_data) {
+  SaveImage("golden_screenshot", golden_screenshots_dir_, png_data);
 }
 
-void ScreenshotTester::UpdateGoldenScreenshot(const std::string& file_name,
-                                              PNGFile png_data) {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+void ScreenshotTester::SaveImage(const std::string& file_name,
+                                 const base::FilePath& screenshot_dir,
+                                 PNGFile png_data) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  base::FilePath screenshot_path =
+      screenshot_dir.AppendASCII(test_name_ + "_" + file_name + ".png");
   if (!png_data) {
     LOG(ERROR) << "Can't take a screenshot";
-    return;
+    DCHECK(false);
   }
-  base::FilePath golden_screenshot_path =
-      screenshot_dest_.AppendASCII(file_name + ".png");
-  if (!base::CreateDirectory(golden_screenshot_path.DirName())) {
-    LOG(ERROR) << "Can't create a directory ";
-    return;
+  if (!base::CreateDirectory(screenshot_path.DirName())) {
+    LOG(ERROR) << "Can't create a directory "
+               << screenshot_path.DirName().value();
+    DCHECK(false);

[ygorshenin1, 2014/07/31 13:19:14]

It's better add return value for SaveImage() and return false from the method
instead of DCHECK(). The same for line #118.

[Lisa Ignatyeva, 2014/07/31 16:37:39]

Done.

   }
   if (static_cast<size_t>(
-          base::WriteFile(golden_screenshot_path,
+          base::WriteFile(screenshot_path,
                           reinterpret_cast<char*>(&(png_data->data()[0])),
                           png_data->size())) != png_data->size()) {
-    LOG(ERROR) << "Can't save screenshot";
+    LOG(ERROR) << "Can't save screenshot " << file_name;
+    DCHECK(false);
   }
-  VLOG(0) << "Golden screenshot updated successfully";
+  VLOG(0) << "Screenshot " << file_name << ".png saved successfully to "
+          << screenshot_dir.value();
 }
 
-void ScreenshotTester::ReturnScreenshot(PNGFile png_data) {
-  // TODO(elizavetai): rewrite this function so that TakeScreenshot
-  // could return a |PNGFile| -- current screenshot.
-  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  screenshot_ = png_data;
+void ScreenshotTester::ReturnScreenshot(const PNGFile& screenshot,
+                                        PNGFile png_data) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));

[ygorshenin1, 2014/07/31 13:19:14]

nit: replace this DCHECK() by DCHECK_CURRENTLY_ON(). The same for lines #150 and
#173.

[Lisa Ignatyeva, 2014/07/31 16:37:39]

Done.

+  screenshot->data() = png_data->data();
   content::BrowserThread::PostTask(
       content::BrowserThread::UI, FROM_HERE, run_loop_quitter_);
 }
 
-void ScreenshotTester::TakeScreenshot() {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+ScreenshotTester::PNGFile ScreenshotTester::TakeScreenshot() {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
   aura::Window* primary_window = ash::Shell::GetPrimaryRootWindow();
   gfx::Rect rect = primary_window->bounds();
+  PNGFile screenshot = new base::RefCountedBytes;
   ui::GrabWindowSnapshotAsync(primary_window,
                               rect,
                               content::BrowserThread::GetBlockingPool(),
                               base::Bind(&ScreenshotTester::ReturnScreenshot,
-                                         weak_factory_.GetWeakPtr()));
+                                         weak_factory_.GetWeakPtr(),
+                                         screenshot));
   base::RunLoop run_loop;
   run_loop_quitter_ = run_loop.QuitClosure();
   run_loop.Run();
+  return screenshot;
+}
+
+ScreenshotTester::PNGFile ScreenshotTester::LoadGoldenScreenshot() {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+  base::FilePath screenshot_path = golden_screenshots_dir_.AppendASCII(
+      test_name_ + "_golden_screenshot.png");
+  if (!base::PathExists(screenshot_path)) {
+    LOG(ERROR) << "Can't find a golden screenshot for this test";
+    DCHECK(false);

[ygorshenin1, 2014/07/31 13:19:14]

No need in DCHECK(), since path does not exists and following call to
GetFileSize will return incorrect value. It's better to return from the method.

[Lisa Ignatyeva, 2014/07/31 16:37:38]

I can return NULL instead of loaded file if something goes wrong, but that seems
strange to me.  

[ygorshenin1, 2014/07/31 16:50:17]

I suggest you to replace DCHECK by CHECK, as I explained above.

On 2014/07/31 16:37:38, Lisa Ignatyeva wrote:

[Lisa Ignatyeva, 2014/07/31 16:58:39]

Done.

+  }
+
+  size_t golden_screenshot_size;
+  base::GetFileSize(screenshot_path,
+                    reinterpret_cast<int64*>(&golden_screenshot_size));
+
+  PNGFile png_data = new base::RefCountedBytes;
+  png_data->data().resize(golden_screenshot_size);
+  base::ReadFile(screenshot_path,
+                 reinterpret_cast<char*>(&(png_data->data()[0])),
+                 golden_screenshot_size);
+
+  return png_data;
+}
+
+void ScreenshotTester::CompareScreenshots(PNGFile model, PNGFile sample) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+  ASSERT_FALSE(model == NULL);

[ygorshenin1, 2014/07/31 13:19:14]

nit: ASSERT_TRUE(model.get());
     ASSERT_TRUE(sample.get());

[Lisa Ignatyeva, 2014/07/31 16:37:39]

Done.

+  ASSERT_FALSE(sample == NULL);
+
+  SkBitmap model_bitmap;
+  SkBitmap sample_bitmap;
+  gfx::PNGCodec::Decode(reinterpret_cast<unsigned char*>(&(model->data()[0])),
+                        model->data().size(),
+                        &model_bitmap);
+  gfx::PNGCodec::Decode(reinterpret_cast<unsigned char*>(&(sample->data()[0])),
+                        sample->data().size(),
+                        &sample_bitmap);
+
+  ASSERT_EQ(model_bitmap.config(), sample_bitmap.config());
+  ASSERT_EQ(model_bitmap.width(), sample_bitmap.width());
+  ASSERT_EQ(model_bitmap.height(), sample_bitmap.height());
+
+  bool screenshots_match = true;
+
+  SkCanvas diff_canvas(sample_bitmap);
+  for (int i = 0; i < model_bitmap.width(); i++)
+    for (int j = 0; j < model_bitmap.height(); j++) {
+      if (model_bitmap.getColor(i, j) == sample_bitmap.getColor(i, j)) {
+        diff_canvas.drawPoint(i, j, SK_ColorWHITE);
+      } else {
+        screenshots_match = false;
+        diff_canvas.drawPoint(i, j, SK_ColorRED);
+      }
+    }
+
+  if (!screenshots_match) {
+    SaveImage("failed_screenshot", artifacts_dir_, sample);
+    gfx::PNGCodec::EncodeBGRASkBitmap(sample_bitmap, false, &sample->data());
+    SaveImage("difference", artifacts_dir_, sample);
+    LOG(ERROR)
+        << "Screenshots testing failed. Screenshots differ in some pixels";
+    VLOG(0) << "Current screenshot and diff picture saved to "
+            << artifacts_dir_.value();
+    ASSERT_TRUE(screenshots_match);

[ygorshenin1, 2014/07/31 13:19:14]

This assertion contradicts with if's condition.

[Lisa Ignatyeva, 2014/07/31 16:37:39]

Yes, that was done intentionally. The test is failed when |screenshot_match| is
false, but we have to do some logging if the test is failed. So, here is
actually written something like: "if we found a mistake, log errors and then
fail". I've moved the assertion, now it should look less weird.

+  } else {
+    VLOG(0) << "Current screenshot matches the golden screenshot. Screenshot "
+               "testing passed.";
+  }
 }
 
 }  // namespace chromeos