Better comparison for frame analyzer

webrtc/tools/frame_analyzer/video_quality_analysis_unittest.cc

Index: webrtc/tools/frame_analyzer/video_quality_analysis_unittest.cc
diff --git a/webrtc/tools/frame_analyzer/video_quality_analysis_unittest.cc b/webrtc/tools/frame_analyzer/video_quality_analysis_unittest.cc
index 85d0b46e79f72879db7fa0f7053888d7fe19fc2a..304477ee39366bede7bf00df56cf2b5727eda0b3 100644
--- a/webrtc/tools/frame_analyzer/video_quality_analysis_unittest.cc
+++ b/webrtc/tools/frame_analyzer/video_quality_analysis_unittest.cc
@@ -27,8 +27,8 @@ namespace test {
 class VideoQualityAnalysisTest : public ::testing::Test {
  protected:
   static void SetUpTestCase() {
-    std::string log_filename = webrtc::test::OutputPath() +
-        "VideoQualityAnalysisTest.log";
+    std::string log_filename = TempFilename(webrtc::test::OutputPath(),

[kjellander_webrtc, 2017/02/15 12:16:58]

"Google Test automatically calls SetUpTestCase() before running the first test
in the FooTest test case". 
https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid...

This means the same log file will be shared between all the test cases, which
doesn't seem to be what we want. This should probably be moved into a SetUp()
method instead, which executes before each individual test case. Same for
TearDownTestCase().

[mandermo, 2017/02/15 12:52:54]

This code was there before I started to work on this. It should be fine, since
they don't care about the output to the log. I have changed it to SetUp() and
TearDown() anyway, since it also works.

[kjellander_webrtc, 2017/02/15 14:00:37]

OK, it feels like things could break if we write and read to the same log file
in multiple parallel tests.

[mandermo, 2017/02/15 14:50:29]

That log file is just for putting the data somewhere and is never read from, but
I agree it is nicer to put it into SetUp() instead of SetUpTestCase().

+                                            "VideoQualityAnalysisTest.log");
     logfile_ = fopen(log_filename.c_str(), "w");
     ASSERT_TRUE(logfile_ != NULL);
   }
@@ -95,8 +95,8 @@ TEST_F(VideoQualityAnalysisTest, PrintMaxRepeatedAndSkippedFramesInvalidFile) {
 
 TEST_F(VideoQualityAnalysisTest,
        PrintMaxRepeatedAndSkippedFramesEmptyStatsFile) {
-  std::string stats_filename_ref = OutputPath() + "empty-stats-1.txt";
-  std::string stats_filename = OutputPath() + "empty-stats-2.txt";
+  std::string stats_filename_ref = TempFilename(OutputPath(), "stats-1.txt");

[kjellander_webrtc, 2017/02/15 12:16:58]

The declaration pattern at line 98-100 is repeated a lot so I suggest you move
those variables into private members of the VideoQualityAnalysisTest class
(+suffix them with _) and initialize the first two in a SetUp() function. 

Then you can remove a lot of the boilerplate. I realize not all test cases will
use stats_filename_ref_ but it won't really matter (most do).

[mandermo, 2017/02/15 12:52:54]

Done.

+  std::string stats_filename = TempFilename(OutputPath(), "stats-2.txt");
   std::ofstream stats_file;
   stats_file.open(stats_filename_ref.c_str());
   stats_file.close();
@@ -107,8 +107,8 @@ TEST_F(VideoQualityAnalysisTest,
 }
 
 TEST_F(VideoQualityAnalysisTest, PrintMaxRepeatedAndSkippedFramesNormalFile) {
-  std::string stats_filename_ref = OutputPath() + "stats-1.txt";
-  std::string stats_filename = OutputPath() + "stats-2.txt";
+  std::string stats_filename_ref = TempFilename(OutputPath(), "stats-1.txt");
+  std::string stats_filename = TempFilename(OutputPath(), "stats-2.txt");
   std::ofstream stats_file;
 
   stats_file.open(stats_filename_ref.c_str());
@@ -132,6 +132,205 @@ TEST_F(VideoQualityAnalysisTest, PrintMaxRepeatedAndSkippedFramesNormalFile) {
                                    stats_filename_ref, stats_filename);
 }
 
+namespace {
+void VerifyLogOutput(const std::string& log_filename,
+                     const std::vector<std::string>& expected_out) {
+  std::ifstream logf(log_filename);
+  std::string line;
 
+  std::size_t i;
+  for (i = 0; i < expected_out.size() && getline(logf, line); ++i) {
+    ASSERT_EQ(expected_out.at(i), line);
+  }
+  ASSERT_TRUE(i == expected_out.size()) << "Not enough input data";
+}
+}  // unnamed namespace
+
+TEST_F(VideoQualityAnalysisTest,
+       PrintMaxRepeatedAndSkippedFramesSkippedFrames) {
+  std::string stats_filename_ref = TempFilename(OutputPath(), "stats-1.txt");
+  std::string stats_filename = TempFilename(OutputPath(), "stats-2.txt");
+  std::ofstream stats_file;
+
+  std::string log_filename =
+      TempFilename(webrtc::test::OutputPath(), "log.log");
+  FILE* logfile = fopen(log_filename.c_str(), "w");
+  ASSERT_TRUE(logfile != NULL);
+  stats_file.open(stats_filename_ref.c_str());
+  stats_file << "frame_0001 0100\n";
+  stats_file << "frame_0002 0101\n";
+  stats_file << "frame_0002 0101\n";
+  stats_file << "frame_0003 0103\n";
+  stats_file << "frame_0004 0103\n";
+  stats_file << "frame_0005 0106\n";
+  stats_file << "frame_0006 0106\n";
+  stats_file << "frame_0007 0108\n";
+  stats_file << "frame_0008 0110\n";
+  stats_file << "frame_0009 0112\n";
+  stats_file.close();
+
+  stats_file.open(stats_filename.c_str());
+  stats_file << "frame_0001 0101\n";
+  stats_file << "frame_0002 0101\n";
+  stats_file << "frame_0003 0101\n";
+  stats_file << "frame_0004 0108\n";
+  stats_file << "frame_0005 0108\n";
+  stats_file << "frame_0006 0112\n";
+  stats_file.close();
+
+  PrintMaxRepeatedAndSkippedFrames(logfile, "NormalStatsFile",
+                                   stats_filename_ref, stats_filename);
+  ASSERT_EQ(0, fclose(logfile));
+
+  std::vector<std::string> expected_out = {
+      "RESULT Max_repeated: NormalStatsFile= 2",
+      "RESULT Max_skipped: NormalStatsFile= 2",
+      "RESULT Total_skipped: NormalStatsFile= 3",
+      "RESULT Decode_errors_reference: NormalStatsFile= 0",
+      "RESULT Decode_errors_test: NormalStatsFile= 0"};
+  VerifyLogOutput(log_filename, expected_out);
+}
+
+TEST_F(VideoQualityAnalysisTest,
+       PrintMaxRepeatedAndSkippedFramesDecodeErrorInTest) {
+  std::string stats_filename_ref =
+      TempFilename(OutputPath(), "error-test-stats-1.txt");
+  std::string stats_filename =
+      TempFilename(OutputPath(), "error-test-stats-2.txt");
+  std::ofstream stats_file;
+
+  std::string log_filename =
+      TempFilename(webrtc::test::OutputPath(), "log.log");
+  FILE* logfile = fopen(log_filename.c_str(), "w");
+  ASSERT_TRUE(logfile != NULL);
+  stats_file.open(stats_filename_ref.c_str());
+  stats_file << "frame_0001 0100\n";
+  stats_file << "frame_0002 0100\n";
+  stats_file << "frame_0002 0101\n";
+  stats_file << "frame_0003 0103\n";
+  stats_file << "frame_0004 0103\n";
+  stats_file << "frame_0005 0106\n";
+  stats_file << "frame_0006 0107\n";
+  stats_file << "frame_0007 0107\n";
+  stats_file << "frame_0008 0110\n";
+  stats_file << "frame_0009 0112\n";
+  stats_file.close();
+
+  stats_file.open(stats_filename.c_str());
+  stats_file << "frame_0001 0101\n";
+  stats_file << "frame_0002 Barcode error\n";
+  stats_file << "frame_0003 Barcode error\n";
+  stats_file << "frame_0004 Barcode error\n";
+  stats_file << "frame_0005 0107\n";
+  stats_file << "frame_0006 0110\n";
+  stats_file.close();
+
+  PrintMaxRepeatedAndSkippedFrames(logfile, "NormalStatsFile",
+                                   stats_filename_ref, stats_filename);
+  ASSERT_EQ(0, fclose(logfile));
+
+  std::vector<std::string> expected_out = {
+      "RESULT Max_repeated: NormalStatsFile= 1",
+      "RESULT Max_skipped: NormalStatsFile= 0",
+      "RESULT Total_skipped: NormalStatsFile= 0",
+      "RESULT Decode_errors_reference: NormalStatsFile= 0",
+      "RESULT Decode_errors_test: NormalStatsFile= 3"};
+  VerifyLogOutput(log_filename, expected_out);
+}
+
+TEST_F(VideoQualityAnalysisTest, CalculateFrameClustersOneValue) {

[kjellander_webrtc, 2017/02/15 12:16:58]

Please add a test case for an empty stats file as well.

[mandermo, 2017/02/15 12:52:54]

Done.

+  std::string stats_filename = TempFilename(OutputPath(), "stats.txt");
+  std::ofstream stats_file;
+
+  stats_file.open(stats_filename.c_str());
+  stats_file << "frame_0001 0101\n";
+  stats_file.close();
+
+  FILE* stats_filef = fopen(stats_filename.c_str(), "r");
+  ASSERT_TRUE(stats_filef != NULL);
+
+  auto clusters = CalculateFrameClusters(stats_filef, nullptr);
+  decltype(clusters) expected = {std::make_pair(101, 1)};
+  ASSERT_EQ(expected, clusters);
+}
+
+TEST_F(VideoQualityAnalysisTest, CalculateFrameClustersOneOneTwo) {
+  std::string stats_filename = TempFilename(OutputPath(), "stats.txt");
+  std::ofstream stats_file;
+
+  stats_file.open(stats_filename.c_str());
+  stats_file << "frame_0001 0101\n";
+  stats_file << "frame_0002 0101\n";
+  stats_file << "frame_0003 0102\n";
+  stats_file.close();
+
+  FILE* stats_filef = fopen(stats_filename.c_str(), "r");
+  ASSERT_TRUE(stats_filef != NULL);
+
+  auto clusters = CalculateFrameClusters(stats_filef, nullptr);
+  decltype(clusters) expected = {std::make_pair(101, 2),
+                                 std::make_pair(102, 1)};
+  ASSERT_EQ(expected, clusters);
+}
+
+TEST_F(VideoQualityAnalysisTest, CalculateFrameClustersOneOneErrErrThree) {
+  std::string stats_filename = TempFilename(OutputPath(), "stats.txt");
+  std::ofstream stats_file;
+
+  stats_file.open(stats_filename.c_str());
+  stats_file << "frame_0001 0101\n";
+  stats_file << "frame_0002 0101\n";
+  stats_file << "frame_0003 Barcode error\n";
+  stats_file << "frame_0004 Barcode error\n";
+  stats_file << "frame_0005 0103\n";
+  stats_file.close();
+
+  FILE* stats_filef = fopen(stats_filename.c_str(), "r");
+  ASSERT_TRUE(stats_filef != NULL);
+
+  auto clusters = CalculateFrameClusters(stats_filef, nullptr);
+  decltype(clusters) expected = {std::make_pair(101, 2),
+                                 std::make_pair(DECODE_ERROR, 2),
+                                 std::make_pair(103, 1)};
+  ASSERT_EQ(expected, clusters);
+}
+
+TEST_F(VideoQualityAnalysisTest, CalculateFrameClustersErrErr) {
+  std::string stats_filename = TempFilename(OutputPath(), "stats.txt");
+  std::ofstream stats_file;
+
+  stats_file.open(stats_filename.c_str());
+  stats_file << "frame_0001 Barcode error\n";
+  stats_file << "frame_0002 Barcode error\n";
+  stats_file.close();
+
+  FILE* stats_filef = fopen(stats_filename.c_str(), "r");
+  ASSERT_TRUE(stats_filef != NULL);
+
+  auto clusters = CalculateFrameClusters(stats_filef, nullptr);
+  decltype(clusters) expected = {std::make_pair(DECODE_ERROR, 2)};
+  ASSERT_EQ(expected, clusters);
+}
+
+TEST_F(VideoQualityAnalysisTest, CalculateFrameClustersOneOneErrErrOne) {

[kjellander_webrtc, 2017/02/15 12:16:58]

Following your established naming convention this should be called
CalculateFrameClustersOneOneErrErrOneOne (or remove line 325 and change the size
to 5).

[mandermo, 2017/02/15 12:52:54]

Done.

+  std::string stats_filename = TempFilename(OutputPath(), "stats.txt");
+  std::ofstream stats_file;
+
+  stats_file.open(stats_filename.c_str());
+  stats_file << "frame_0001 0101\n";
+  stats_file << "frame_0002 0101\n";
+  stats_file << "frame_0003 Barcode error\n";
+  stats_file << "frame_0004 Barcode error\n";
+  stats_file << "frame_0005 0101\n";
+  stats_file << "frame_0006 0101\n";
+  stats_file.close();
+
+  FILE* stats_filef = fopen(stats_filename.c_str(), "r");

[mandermo, 2017/02/15 12:52:54]

I forgot to close the file. Doing it now.

+  ASSERT_TRUE(stats_filef != NULL);
+
+  auto clusters = CalculateFrameClusters(stats_filef, nullptr);
+  decltype(clusters) expected = {std::make_pair(101, 6)};
+  ASSERT_EQ(expected, clusters);
+}
 }  // namespace test
 }  // namespace webrtc