Successful translation erroneously reported on page with too little text

chrome/browser/ui/views/translate/translate_bubble_view_interactive_uitest.cc

Index: chrome/browser/ui/views/translate/translate_bubble_view_interactive_uitest.cc
diff --git a/chrome/browser/ui/views/translate/translate_bubble_view_interactive_uitest.cc b/chrome/browser/ui/views/translate/translate_bubble_view_interactive_uitest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..dd12eda7afee7a3a4558102b5323c754e51527b8
--- /dev/null
+++ b/chrome/browser/ui/views/translate/translate_bubble_view_interactive_uitest.cc
@@ -0,0 +1,188 @@
+// Copyright (c) 2017 The Chromium Authors. All rights reserved.

[groby-ooo-7-16, 2017/05/31 19:51:44]

Why did you name this interactive_uitest instead of browser_test? Intentional,
or based on the autofill test?

If the latter, please don't - there are very specific reasons why something
should be an interactive_ui_test, and we're trying to avoid them. (See
https://www.chromium.org/developers/testing/browser-tests for more details)

[Gaja, 2017/06/01 12:24:02]

I noticed that few bubble_view/icon_view related tests, especially with respect
to URL bar, were interactive_uitests. So decided to name it the same. Also
influenced by autofill test. If interactive_ui_tests are being avoided or not
preferred, then I will switch to browser_test.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <string>
+
+#include "base/macros.h"
+#include "chrome/app/chrome_command_ids.h"
+#include "chrome/browser/chrome_notification_types.h"
+#include "chrome/browser/renderer_context_menu/render_view_context_menu_test_util.h"
+#include "chrome/browser/translate/chrome_translate_client.h"
+#include "chrome/browser/translate/translate_service.h"
+#include "chrome/browser/ui/browser_command_controller.h"
+#include "chrome/browser/ui/browser_window.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/browser/ui/translate/translate_bubble_test_utils.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "chrome/test/base/interactive_test_utils.h"
+#include "chrome/test/base/ui_test_utils.h"
+#include "components/translate/core/browser/translate_manager.h"
+#include "content/public/browser/notification_service.h"
+#include "content/public/browser/web_contents.h"
+#include "net/test/embedded_test_server/embedded_test_server.h"
+#include "net/url_request/test_url_fetcher_factory.h"
+#include "net/url_request/url_request_status.h"
+
+namespace {
+
+static const char kDataURIPrefix[] = "data:text/html;charset=utf-8,";
+static const char kTestLanguageString[] =
+    "我々は重要な、興味深いものになるが、時折状況が発生するため苦労や痛みは"
+    "彼にいくつかの素晴らしいを調達することができます。それから、いくつかの利";
+}
+
+class TranslateBubbleViewInteractiveTest : public InProcessBrowserTest {
+ public:
+  TranslateBubbleViewInteractiveTest() {}
+  ~TranslateBubbleViewInteractiveTest() override {}
+  void SetUpOnMainThread() override {
+    InProcessBrowserTest::SetUpOnMainThread();
+  }
+
+  content::WebContents* GetWebContents() {
+    return browser()->tab_strip_model()->GetActiveWebContents();
+  }
+
+  void SimulateURLFetch(bool success) {

[groby-ooo-7-16, 2017/05/31 19:51:44]

It's probably worth collecting those test functions in a separate file - I
assume this is duplicated code from autofill_interactive_uitest?

(Sorry, can't verify a lot of things because code search is broken right now:( )

[Gaja, 2017/06/01 12:24:02]

OK, I will try moving it to separate file.
Yes, most of the code is duplicated from autofill_interactive_uitest, which I
thought that test was suitable for checking translate bubble states.

+    net::TestURLFetcher* fetcher = url_fetcher_factory_.GetFetcherByID(0);
+    ASSERT_TRUE(fetcher);
+    net::Error error = success ? net::OK : net::ERR_FAILED;
+
+    std::string script =
+        " var google = {};"
+        "google.translate = (function() {"
+        "  return {"
+        "    TranslateService: function() {"
+        "      return {"
+        "        isAvailable : function() {"
+        "          return true;"
+        "        },"
+        "        restore : function() {"
+        "          return;"
+        "        },"
+        "        getDetectedLanguage : function() {"
+        "          return \"ja\";"
+        "        },"
+        "        translatePage : function(originalLang, targetLang,"
+        "                                 onTranslateProgress) {"
+        "          document.getElementsByTagName(\"body\")[0].innerHTML = '" +
+        std::string(kTestLanguageString) +
+        "              ';"
+        "          onTranslateProgress(100, true, false);"
+        "        }"
+        "      };"
+        "    }"
+        "  };"
+        "})();"
+        "cr.googleTranslate.onTranslateElementLoad();";
+
+    fetcher->set_url(fetcher->GetOriginalURL());
+    fetcher->set_status(net::URLRequestStatus::FromError(error));
+    fetcher->set_response_code(success ? 200 : 500);
+    fetcher->SetResponseString(script);
+    fetcher->delegate()->OnURLFetchComplete(fetcher);
+  }
+
+ private:
+  net::TestURLFetcherFactory url_fetcher_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(TranslateBubbleViewInteractiveTest);
+};
+
+IN_PROC_BROWSER_TEST_F(TranslateBubbleViewInteractiveTest, TranslateSuccess) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+  ASSERT_TRUE(TranslateService::IsTranslateBubbleEnabled());
+
+  translate::TranslateManager::SetIgnoreMissingKeyForTesting(true);

[groby-ooo-7-16, 2017/05/31 19:51:44]

You probably want to do that in the SetUp code - it's likely you need this for
all test cases. (And this might possibly cause the failure)

[Gaja, 2017/06/01 12:24:02]

Acknowledged.

+
+  GURL url(
+      std::string(kDataURIPrefix) +
+      // Add Japanese characters to ensure the translate bar
+      // will appear.
+      "我々は重要な、興味深いものになるが、時折状況が発生するため苦労や痛みは"
+      "彼にいくつかの素晴らしいを調達することができます。それから、いくつかの"
+      "利");
+
+  // Set up an observer to be able to wait for the bubble to be shown.

[groby-ooo-7-16, 2017/05/31 19:51:44]

Since this is shared across tests, might belong on the fixture. 

[Gaja, 2017/06/01 12:24:02]

Acknowledged.

+  content::Source<content::WebContents> source(GetWebContents());
+  content::WindowedNotificationObserver language_detected_signal(
+      chrome::NOTIFICATION_TAB_LANGUAGE_DETERMINED, source);
+
+  ASSERT_NO_FATAL_FAILURE(ui_test_utils::NavigateToURL(browser(), url));
+
+  // Wait for the translate bubble to appear.
+  language_detected_signal.Wait();
+
+  // Verify current translate step.
+  const TranslateBubbleModel* model =
+      translate::test_utils::GetCurrentModel(browser());
+  ASSERT_NE(nullptr, model);
+  EXPECT_EQ(TranslateBubbleModel::VIEW_STATE_BEFORE_TRANSLATE,
+            model->GetViewState());
+
+  translate::test_utils::PressTranslate(browser());
+
+  // Verify current translate step after pressing translate button.
+  EXPECT_EQ(TranslateBubbleModel::VIEW_STATE_TRANSLATING,
+            model->GetViewState());
+
+  // Wait for translation.
+  content::WindowedNotificationObserver translation_observer(

[groby-ooo-7-16, 2017/05/31 19:51:44]

This is probably sharable too

[Gaja, 2017/06/01 12:24:02]

Acknowledged.

+      chrome::NOTIFICATION_PAGE_TRANSLATED,
+      content::NotificationService::AllSources());
+
+  // Simulate the translate script being retrieved.
+  // Pass fake google.translate lib as the translate script.
+  SimulateURLFetch(true);
+
+  // Simulate the render notifying the translation has been done.
+  translation_observer.Wait();
+
+  EXPECT_EQ(TranslateBubbleModel::VIEW_STATE_AFTER_TRANSLATE,
+            model->GetViewState());
+}
+
+IN_PROC_BROWSER_TEST_F(TranslateBubbleViewInteractiveTest, TranslateError) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+  ASSERT_TRUE(TranslateService::IsTranslateBubbleEnabled());
+
+  translate::TranslateManager::SetIgnoreMissingKeyForTesting(true);
+
+  // Set up an observer to be able to wait for the bubble to be shown.
+  content::Source<content::WebContents> source(GetWebContents());
+  content::WindowedNotificationObserver language_detected_signal(
+      chrome::NOTIFICATION_TAB_LANGUAGE_DETERMINED, source);
+
+  ASSERT_NO_FATAL_FAILURE(
+      ui_test_utils::NavigateToURL(browser(), GURL("about:blank")));
+
+  // Wait for the language detection.
+  language_detected_signal.Wait();
+
+  content::ContextMenuParams params;

[groby-ooo-7-16, 2017/05/31 19:51:44]

You might want to use translate::test_utils::PressTranslate(browser());

[Gaja, 2017/06/01 12:24:02]

As I observed, for about:blank case, the translate icon and bubble does not
appear in location bar. But the "Translate to English" option will be available
in Context menu. As per my understanding, the above util function does press
action on "Translate" button in bubble. Since the bubble isn't shown, I was
sceptical to use PressTranslate at this point. Please let me know your opinion.

+  TestRenderViewContextMenu menu(GetWebContents()->GetMainFrame(), params);
+  menu.Init();
+  menu.ExecuteCommand(IDC_CONTENT_CONTEXT_TRANSLATE, 0);
+
+  // Verify current translate step.
+  const TranslateBubbleModel* model =
+      translate::test_utils::GetCurrentModel(browser());
+  ASSERT_NE(nullptr, model);
+  EXPECT_EQ(TranslateBubbleModel::VIEW_STATE_TRANSLATING,
+            model->GetViewState());
+
+  // Wait for translation.
+  content::WindowedNotificationObserver translation_observer(
+      chrome::NOTIFICATION_PAGE_TRANSLATED,
+      content::NotificationService::AllSources());
+
+  // Simulate the translate script being retrieved.
+  // Pass fake google.translate lib as the translate script.
+  SimulateURLFetch(true);
+
+  // Simulate the render notifying the translation has been done.
+  translation_observer.Wait();
+
+  EXPECT_EQ(TranslateBubbleModel::VIEW_STATE_ERROR, model->GetViewState());
+}