More clearly point out XR JavaScript failures

Makes the first (and thus fatal) failure from JavaScript more clear in
the XR browser tests. This is achieved by adding logging to point the
user towards the failure and stack that are actually useful for
debugging.

This is a workaround for a limitation of browser tests in general.
FAIL() and other fatal macros provided by gtest only abort the current
function. Since all the JavaScript execution is handled by helper
functions, this means that even if we catch a failure in JavaScript and
report it as a failure, the browser test will continue to run. This can
then cause debugging issues because errors/stack traces from later on in
the test end up getting printed as well since the test is continuing to
run in a bad state.

Bug: 961378
Change-Id: Iafd4892dc7d5307fa35848a855d5f1c76bd27095
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1605340
Auto-Submit: Brian Sheedy <bsheedy@chromium.org>
Commit-Queue: Bill Orr <billorr@chromium.org>
Reviewed-by: Bill Orr <billorr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#658773}

chrome/browser/vr/test/xr_browser_test.cc

@@ -195,6 +195,11 @@ void XrBrowserTestBase::LoadUrlAndAwaitInitialization(const GURL& url) {
 void XrBrowserTestBase::RunJavaScriptOrFail(
     const std::string& js_expression,
     content::WebContents* web_contents) {
+  if (javascript_failed_) {
+    LogJavaScriptFailure();
+    return;
+  }
+
   ASSERT_TRUE(content::ExecuteScript(web_contents, js_expression))
       << "Failed to run given JavaScript: " << js_expression;
 }
@@ -202,6 +207,11 @@ void XrBrowserTestBase::RunJavaScriptOrFail(
 bool XrBrowserTestBase::RunJavaScriptAndExtractBoolOrFail(
     const std::string& js_expression,
     content::WebContents* web_contents) {
+  if (javascript_failed_) {
+    LogJavaScriptFailure();
+    return false;
+  }
+
   bool result;
   DLOG(ERROR) << "Run JavaScript: " << js_expression;
   EXPECT_TRUE(content::ExecuteScriptAndExtractBool(
@@ -214,6 +224,11 @@ bool XrBrowserTestBase::RunJavaScriptAndExtractBoolOrFail(
 std::string XrBrowserTestBase::RunJavaScriptAndExtractStringOrFail(
     const std::string& js_expression,
     content::WebContents* web_contents) {
+  if (javascript_failed_) {
+    LogJavaScriptFailure();
+    return "";
+  }
+
   std::string result;
   EXPECT_TRUE(content::ExecuteScriptAndExtractString(
       web_contents,
@@ -332,6 +347,18 @@ void XrBrowserTestBase::WaitOnJavaScriptStep(
       reason +=
           " JavaScript testharness reported failure reason: " + result_string;
     }
+    // Store that we've failed waiting for a JavaScript step so we can abort
+    // further attempts to run JavaScript, which has the potential to do weird
+    // things and produce non-useful output due to JavaScript code continuing
+    // to run when it's in a known bad state.
+    // This is a workaround for the fact that FAIL() and other gtest macros that
+    // cause test failures only abort the current function. Thus, a failure here
+    // will show up as a test failure, but there's nothing that actually stops
+    // the test from continuing to run since FAIL() is not being called in the
+    // main test body.
+    javascript_failed_ = true;
+    // Newlines to help the failure reason stick out.
+    LOG(ERROR) << "\n\n\nvvvvvvvvvvvvvvvvv Useful Stack vvvvvvvvvvvvvvvvv\n\n";
     FAIL() << reason;
   }
 
@@ -432,4 +459,10 @@ void XrBrowserTestBase::AssertNoJavaScriptErrors() {
   AssertNoJavaScriptErrors(GetCurrentWebContents());
 }
 
+void XrBrowserTestBase::LogJavaScriptFailure() {
+  LOG(ERROR) << "HEY! LISTEN! Not running requested JavaScript due to previous "
+                "failure. Failures below this are likely garbage. Look for the "
+                "useful stack above.";
+}
+
 }  // namespace vr

chrome/browser/vr/test/xr_browser_test.h

@@ -210,9 +210,11 @@ class XrBrowserTestBase : public InProcessBrowserTest {
   std::unordered_set<std::string> ignored_requirements_;
 
  private:
+  void LogJavaScriptFailure();
   std::unique_ptr<net::EmbeddedTestServer> server_;
   base::test::ScopedFeatureList scoped_feature_list_;
   bool test_skipped_at_startup_ = false;
+  bool javascript_failed_ = false;
   DISALLOW_COPY_AND_ASSIGN(XrBrowserTestBase);
 };