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..6aa156fc3564cb41f756cad5492d4291301695f2 100644
--- a/webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc
+++ b/webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc
@@ -8,17 +8,49 @@
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
+#include <cstdio>
+#include <memory>
+
 #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;

[kwiberg-webrtc, 2017/03/16 14:10:08]

I don't think there's ever a reason to use typedef instead of using in new code.

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

Just wanted to make an alias.
Should I remove both? Or can I leave the first?

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

Sorry, I should probably have put quotes around "using" to make that easier to
parse. You can do exactly what you do, but use "using" instead of "typedef".

+
 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;
+Turn const turns[kNumberOfTurns] = {

[kwiberg-webrtc, 2017/03/16 14:10:08]

Don't put a number between the []. That way, it can't be wrong...

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

This was intentional even if it may look strange.
The constant is used in TEST(ConversationalSpeechTest, TimingInstanceRead) to
check the expected size. In this way, if an entry is added in turns without
updating kNumberOfTurns, a compilation error arises. I find this better than
having a test that fails.

But please, just tell me what is better according to the internal standards.

Third option, using sizeof(turns) / sizeof(*turns).

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

Third option. Use [] without a number, and use arraysize(turns) when you need
the size. But often you don't, because range for loops and std::begin/std::end
work with arrays.

+    {"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 turn_ : turns) {

[kwiberg-webrtc, 2017/03/16 14:10:08]

Please don't use trailing _ in anything except member variable names.

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

Done.

+    auto turn = std::unique_ptr<Turn>(
+        new Turn(turn_.speaker_name, turn_.audiotrack_file_name, turn_.offset));
+    timing->AppendTurn(std::move(turn));

[kwiberg-webrtc, 2017/03/16 14:10:08]

You don't need to bind the std::unique_ptr<Turn> to a local variable---just pass
it directly to AppendTurn.

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

Done.

+  }
+
+  return timing;
+}
+
 }  // namespace
 
 TEST(ConversationalSpeechTest, Settings) {
@@ -31,5 +63,51 @@ TEST(ConversationalSpeechTest, Settings) {
   EXPECT_EQ(config.output_path(), output_path);
 }
 
+TEST(ConversationalSpeechTest, TimingInstanceRead) {
+  std::unique_ptr<Timing> timing = CreateTiming();
+
+  // Check size.
+  EXPECT_EQ(timing->Size(), kNumberOfTurns);
+
+  // Test move semantic while iterating over turns.
+  std::size_t index = 0;
+  for (const auto& turn : timing->turns()) {
+    Turn expected_turn = turns[index++];
+    EXPECT_EQ(expected_turn.speaker_name, turn->speaker_name);
+    EXPECT_EQ(expected_turn.audiotrack_file_name, turn->audiotrack_file_name);
+    EXPECT_EQ(expected_turn.offset, turn->offset);
+  }

[kwiberg-webrtc, 2017/03/16 14:10:08]

To minimize the risk of human error, consider incrementing index in a separate
statement at the end.

Actually, maybe this would be even better:

  auto actual_turns = timing->turns();
  EXPECT_EQ(turns.size(), actual_turns.size());
  for (size_t i = 0; i < turns.size(); ++i) {
    ...all the EXPECT_EQs go here...
  }

One should try to avoid creative array indexing whenever possible.

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

Done.

+}
+
+TEST(ConversationalSpeechTest, SaveLoadSettings) {
+  std::string temporary_filepath = webrtc::test::TempFilename(

[kwiberg-webrtc, 2017/03/16 14:10:08]

const std::string. It's a good habit to use const whenever possible---it helps
the reader, since she doesn't have to consider the possibility that the variable
is modified below.

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

Done.

+      webrtc::test::OutputPath(), "TempTimingTestFile");
+
+  // Create a populated Timing instance and save it to a temporary file.
+  std::unique_ptr<Timing> expected_timing = CreateTiming();
+  expected_timing->Save(temporary_filepath);
+
+  // New Timing instance populated by loading the temporary file.
+  Timing timing;
+  timing.Load(temporary_filepath);
+  std::remove(temporary_filepath.c_str());
+
+  // Check size.
+  EXPECT_EQ(expected_timing->Size(), timing.Size());
+
+  // Check turns.
+  const std::vector<std::unique_ptr<Turn>>& expected_turns =
+      expected_timing->turns();
+  const std::vector<std::unique_ptr<Turn>>& turns = timing.turns();
+  for (std::size_t index = 0; index < timing.Size(); index++) {
+    // TODO(alessiob): is this a good practice? I need that the client code
+    // iterates the turns without owning the pointers.
+    Turn* expected_turn = expected_turns[index].get();
+    Turn* turn = turns[index].get();

[kwiberg-webrtc, 2017/03/16 14:10:08]

You don't need to bind these to local variables. Also, are they both
unique_ptrs? In that case, don't use .get()---unique_ptr has a suitable
operator==.

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

Done.

+    EXPECT_TRUE(*expected_turn == *turn)
+        << "turn #" << index << " not matching";
+  }
+}
+
 }  // namespace test
 }  // namespace webrtc