Remove a TODO to convert bool failed, bool failure_ignored to an enum.

chrome/test/out_of_proc_test_runner.cc

-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <string>
 #include <vector>
 
 #include "base/command_line.h"
 #include "base/file_util.h"
 #include "base/hash_tables.h"
 #include "base/linked_ptr.h"
@@ skipping 66 matching lines @@
 // Note: as currently XML is the only supported format by gtest, we don't
 // check output format (e.g. "xml:" prefix) here and output an XML file
 // unconditionally.
 // Note: we don't output per-test-case or total summary info like
 // total failed_test_count, disabled_test_count, elapsed_time and so on.
 // Only each test (testcase element in the XML) will have the correct
 // failed/disabled/elapsed_time information. Each test won't include
 // detailed failure messages either.
 class ResultsPrinter {
  public:
+  enum ResultMask {

[Paweł Hajdan Jr., 2011/01/11 16:33:11]

Why do we need a mask? It should be possible to just use an enum:

RESULT_SUCCESS,
RESULT_FAILURE,
RESULT_IGNORED_FAILURE.

We don't need to OR together RESULT_FAILURE and RESULT_IGNORED_FAILURE.

In fact, I think we should add |run| to the mix, resulting in

RESULT_NOT_RUN,
RESULT_SUCCESS,
RESULT_FAILURE,
RESULT_IGNORED_FAILURE

[tfarina, 2011/01/11 16:36:49]

See line 419.
Initially I went to this way, but because of the possibility of having true and
false, I had to use a mask.
 
I needed the OR because of the condition at line 419.

+    RESULT_NONE            = 0,
+    RESULT_FAILED          = 1 << 0,
+    RESULT_FAILURE_IGNORED = 1 << 1,
+  };
+
   explicit ResultsPrinter(const CommandLine& command_line);
   ~ResultsPrinter();
   void OnTestCaseStart(const char* name, int test_count) const;
   void OnTestCaseEnd() const;
 
-  // TODO(phajdan.jr): Convert bool failed, bool failure_ignored to an enum.
   void OnTestEnd(const char* name, const char* case_name, bool run,
-                 bool failed, bool failure_ignored, double elapsed_time) const;
+                 int result_mask, double elapsed_time) const;
  private:
   FILE* out_;
 
   DISALLOW_COPY_AND_ASSIGN(ResultsPrinter);
 };
 
 ResultsPrinter::ResultsPrinter(const CommandLine& command_line) : out_(NULL) {
   if (!command_line.HasSwitch(kGTestOutputFlag))
     return;
   std::string flag = command_line.GetSwitchValueASCII(kGTestOutputFlag);
@@ skipping 48 matching lines @@
 
 void ResultsPrinter::OnTestCaseEnd() const {
   if (!out_)
     return;
   fprintf(out_, "  </testsuite>\n");
 }
 
 void ResultsPrinter::OnTestEnd(const char* name,
                                const char* case_name,
                                bool run,
-                               bool failed,
-                               bool failure_ignored,
+                               int result_mask,
                                double elapsed_time) const {
   if (!out_)
     return;
   fprintf(out_, "    <testcase name=\"%s\" status=\"%s\" time=\"%.3f\""
           " classname=\"%s\"",
           name, run ? "run" : "notrun", elapsed_time / 1000.0, case_name);
-  if (!failed) {
+  if (!(result_mask & FAILED)) {
     fprintf(out_, " />\n");
     return;
   }
   fprintf(out_, ">\n");
   fprintf(out_, "      <failure message=\"\" type=\"\"%s></failure>\n",
-          failure_ignored ? " ignored=\"true\"" : "");
+          (result_mask & FAILURE_IGNORED) ? " ignored=\"true\"" : "");
   fprintf(out_, "    </testcase>\n");
 }
 
 class TestCasePrinterHelper {
  public:
   TestCasePrinterHelper(const ResultsPrinter& printer,
                         const char* name,
                         int total_test_count)
       : printer_(printer) {
     printer_.OnTestCaseStart(name, total_test_count);
@@ skipping 183 matching lines @@
   ResultsPrinter printer(*command_line);
   for (int i = 0; i < unit_test->total_test_case_count(); ++i) {
     const testing::TestCase* test_case = unit_test->GetTestCase(i);
     TestCasePrinterHelper helper(printer, test_case->name(),
                                  test_case->total_test_count());
     for (int j = 0; j < test_case->total_test_count(); ++j) {
       const testing::TestInfo* test_info = test_case->GetTestInfo(j);
       // Skip disabled tests.
       if (std::string(test_info->name()).find("DISABLED") == 0 &&
           !command_line->HasSwitch(kGTestRunDisabledTestsFlag)) {
-        printer.OnTestEnd(test_info->name(), test_case->name(),
-                          false, false, false, 0);
+        printer.OnTestEnd(test_info->name(), test_case->name(), false,
+                          ResultsPrinter::RESULT_NONE, 0);
         continue;
       }
       std::string test_name = test_info->test_case_name();
       test_name.append(".");
       test_name.append(test_info->name());
       // Skip the test that doesn't match the filter string (if given).
       if ((!positive_filter.empty() &&
            !MatchesFilter(test_name, positive_filter)) ||
           MatchesFilter(test_name, negative_filter)) {
-        printer.OnTestEnd(test_info->name(), test_case->name(),
-                          false, false, false, 0);
+        printer.OnTestEnd(test_info->name(), test_case->name(), false,
+                          ResultsPrinter::RESULT_NONE, 0);
         continue;
       }
       base::Time start_time = base::Time::Now();
       ++test_run_count;
       int exit_code = RunTest(test_name);
       if (exit_code == 0) {
         // Test passed.
-        printer.OnTestEnd(test_info->name(), test_case->name(), true, false,
-                          false,
+        printer.OnTestEnd(test_info->name(), test_case->name(), true,
+                          ResultsPrinter::RESULT_NONE,
                           (base::Time::Now() - start_time).InMillisecondsF());
       } else {
         failed_tests.push_back(test_name);
 
         bool ignore_failure = false;
 
         // -1 exit code means a crash or hang. Never ignore those failures,
         // they are serious and should always be visible.
         if (exit_code != -1)
           ignore_failure = base::TestSuite::ShouldIgnoreFailure(*test_info);
 
-        printer.OnTestEnd(test_info->name(), test_case->name(), true, true,
-                          ignore_failure,
+        int results = ResultsPrinter::RESULT_FAILED;
+        if (ignore_failure)
+          results |= ResultsPrinter::RESULT_FAILURE_IGNORED;
+
+        printer.OnTestEnd(test_info->name(), test_case->name(), true, results,

[tfarina, 2011/01/11 16:54:04]

If it was an enum, how you would write this line?

[Paweł Hajdan Jr., 2011/01/11 16:55:18]

Exactly the same.

                           (base::Time::Now() - start_time).InMillisecondsF());
         if (ignore_failure)
           ++ignored_failure_count;
       }
     }
   }
 
   printf("%d test%s run\n", test_run_count, test_run_count > 1 ? "s" : "");
   printf("%d test%s failed (%d ignored)\n",
          static_cast<int>(failed_tests.size()),
@@ skipping 110 matching lines @@
       exit_code = 1;
       break;
     }
 
     // Special value "-1" means "repeat indefinitely".
     if (cycles != -1)
       cycles--;
   }
   return exit_code;
 }