Conversational speech tool: timing model with data access.

webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc

Index: webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc
diff --git a/webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc b/webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc
index da923bc44817e6297571f39748501b218929249e..eddf7a41e946c0b29cb7ac79e6b7abf07eadbfad 100644
--- a/webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc
+++ b/webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc
@@ -8,21 +8,55 @@
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
+#include <cstdio>
+#include <memory>
+
+#include "webrtc/base/array_view.h"
 #include "webrtc/modules/audio_processing/test/conversational_speech/config.h"
+#include "webrtc/modules/audio_processing/test/conversational_speech/timing.h"
 #include "webrtc/test/gtest.h"
+#include "webrtc/test/testsupport/fileutils.h"
 
 namespace webrtc {
 namespace test {
 namespace {
 
+typedef conversational_speech::Timing Timing;
+typedef conversational_speech::Timing::Turn Turn;
+
 const char* const audiotracks_path = "/path/to/audiotracks";
 const char* const timing_filepath = "/path/to/timing_file.txt";
 const char* const output_path = "/path/to/output_dir";
 
+const std::size_t kNumberOfTurns = 6;
+const Turn turns[kNumberOfTurns] = {
+    {"A", "a1", 0},
+    {"B", "b1", 0},
+    {"A", "a2", 100},
+    {"B", "b2", -200},
+    {"A", "a3", 0},
+    {"A", "a4", 0},
+};
+
+// Create a Timing instance populated with turns.
+std::unique_ptr<Timing> CreateTiming() {
+  std::unique_ptr<Timing> timing = std::unique_ptr<Timing>(new Timing());
+
+  // Add turns.
+  for (Turn source_turn : turns) {

[kwiberg-webrtc, 2017/03/21 22:35:15]

Almost always use const& (or just &, if you need mutability) for the loop
variable in ranged for loops; otherwise you make a copy.

[AleBzk, 2017/03/22 10:07:22]

Done.

+    timing->AppendTurn(Turn(
+        source_turn.speaker_name,
+        source_turn.audiotrack_file_name,
+        source_turn.offset));

[kwiberg-webrtc, 2017/03/21 22:35:15]

Why not just

  timing->AppendTurn(source_turn);

?

[AleBzk, 2017/03/22 10:07:21]

Not applying anymore.

+  }

[kwiberg-webrtc, 2017/03/21 22:35:15]

Hmm. Use the new initializer_list constructor here? Or replace it with a
constructor that accepts an ArrayView<const Turn>?

[AleBzk, 2017/03/22 10:07:22]

Not applying anymore.

+
+  return timing;
+}
+
 }  // namespace
 
 TEST(ConversationalSpeechTest, Settings) {
-  conversational_speech::Config config(
+  const conversational_speech::Config config(
       audiotracks_path, timing_filepath, output_path);
 
   // Test getters.
@@ -31,5 +65,62 @@ TEST(ConversationalSpeechTest, Settings) {
   EXPECT_EQ(config.output_path(), output_path);
 }
 
+TEST(ConversationalSpeechTest, TimingInitializerList) {
+  // Test initializer list for the Timing class.
+  const std::size_t kNumberOfTurns = 6;
+  Timing actual_timing = {
+      Turn("A", "a1", 0),
+      Turn("B", "b1", 0),
+      Turn("A", "a2", 100),
+      Turn("B", "b2", -200),
+      Turn("A", "a3", 0),
+      Turn("A", "a4", 0),
+  };
+  EXPECT_EQ(actual_timing.turns().size(), kNumberOfTurns);

[kwiberg-webrtc, 2017/03/21 22:35:15]

"kNumberOfTurns" isn't much use here. Just use a literal 6.

Also, the convention is to put the expected value in the first argument position
when you use {EXPECT,ASSERT,CHECK,DCHECK}_EQ. I think you're consistently doing
the opposite (understandable, since that's the natural order...).

[AleBzk, 2017/03/22 10:07:21]

Done.

+}
+

[AleBzk, 2017/03/20 14:25:28]

I added a new constructor for Timing to familiarize with initializer lists.

[kwiberg-webrtc, 2017/03/21 22:35:15]

Hmm. Is it useful enough to pay for the extra complexity?

[AleBzk, 2017/03/22 10:07:21]

I don't need it anymore due to the refactoring.

+TEST(ConversationalSpeechTest, TimingInstanceRead) {
+  const std::unique_ptr<Timing> actual_timing = CreateTiming();
+  rtc::ArrayView<const Timing::Turn> actual_turns = actual_timing->turns();
+
+  // Check size.
+  EXPECT_EQ(actual_turns.size(), kNumberOfTurns);
+
+  // Check members for each turn.
+  for (size_t index = 0; index < actual_turns.size(); ++index) {
+    const auto& actual_turn = actual_turns[index];
+    const auto& expected_turn = turns[index];
+    EXPECT_EQ(actual_turn.speaker_name, expected_turn.speaker_name);
+    EXPECT_EQ(actual_turn.audiotrack_file_name,
+              expected_turn.audiotrack_file_name);
+    EXPECT_EQ(actual_turn.offset, expected_turn.offset);

[kwiberg-webrtc, 2017/03/21 22:35:15]

Should Turn have an operator==? Or does it already?---you seem to be using it
below.

[AleBzk, 2017/03/22 10:07:22]

Right, however I removed this test due to the refactoring.

+  }
+}
+
+TEST(ConversationalSpeechTest, SaveLoadSettings) {
+  const std::string temporary_filepath = webrtc::test::TempFilename(
+      webrtc::test::OutputPath(), "TempTimingTestFile");
+
+  // Create a populated Timing instance and save it to a temporary file.
+  const std::unique_ptr<Timing> expected_timing = CreateTiming();
+  expected_timing->Save(temporary_filepath);
+
+  // New Timing instance populated by loading the temporary file.
+  Timing actual_timing;
+  actual_timing.Load(temporary_filepath);
+  std::remove(temporary_filepath.c_str());
+
+  // Check size.
+  rtc::ArrayView<const Timing::Turn> expected_turns = expected_timing->turns();
+  rtc::ArrayView<const Timing::Turn> actual_turns = actual_timing.turns();
+  EXPECT_EQ(expected_turns.size(), actual_turns.size());
+
+  // Check Turn instances.
+  for (size_t index = 0; index < actual_turns.size(); ++index) {
+    EXPECT_TRUE(expected_turns[index] == actual_turns[index])

[kwiberg-webrtc, 2017/03/21 22:35:15]

EXPECT_EQ?

[AleBzk, 2017/03/22 10:07:21]

Done.

+        << "turn #" << index << " not matching";
+  }
+}
+
 }  // namespace test
 }  // namespace webrtc