[BattOr] Retry battor_agent commands when they fail.

tools/battor_agent/battor_agent_unittest.cc

 // Copyright 2015 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 <memory>
 
 #include "tools/battor_agent/battor_agent.h"
 
 #include "base/test/test_simple_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
@@ skipping 146 matching lines @@
     SET_GAIN_SENT,
     GAIN_ACKED,
     START_TRACING_SENT,
     START_TRACING_COMPLETE,
 
     // States required to StopTracing.
     EEPROM_REQUEST_SENT,
     EEPROM_RECEIVED,
     SAMPLES_REQUEST_SENT,
     CALIBRATION_FRAME_RECEIVED,
+    SAMPLES_END_FRAME_RECEIVED,
 
     // States required to RecordClockSyncMarker.
     CURRENT_SAMPLE_REQUEST_SENT,
     RECORD_CLOCK_SYNC_MARKER_COMPLETE,
 
     // States required to GetFirmwareGitHash.
     GIT_FIRMWARE_HASH_REQUEST_SENT,
     READ_GIT_HASH_RECEIVED,
   };
 
   // Runs BattOrAgent::StartTracing until it reaches the specified state by
   // feeding it the callbacks it needs to progress.
   void RunStartTracingTo(BattOrAgentState end_state) {
-    is_command_complete_ = false;
-
-    GetAgent()->StartTracing();

[rnephew (Reviews Here), 2017/05/09 15:04:06]

Charlie is there any reason you put these calls here as opposed to where Aaron
moved them to?

[charliea (OOO until 10-5), 2017/05/09 19:58:08]

Yeah, there was: before, there was the assumption that we'd only ever "run
through" the command once. A specific step in that command might be retried if
something went amiss, but things never really went backwards. 

Aaron's CL changes this and, instead of retrying individual steps, retries the
whole command over from the start when something goes wrong. As such, it makes
sense to be able to run through the command multiple times.

It seems like it's probably also safe to get rid of is_command_complete = false:
it's being set to false in the SetUp() method, where we're already setting
command_error. In the unlikely instance that either needs to be reused within a
single test case, we could always just manually set them.

[aschulman, 2017/05/11 13:13:00]

Acknowledged.

     GetTaskRunner()->RunUntilIdle();
 
     GetAgent()->OnConnectionOpened(true);
     GetTaskRunner()->RunUntilIdle();
 
     if (end_state == BattOrAgentState::CONNECTED)
       return;
 
     OnBytesSent(true);
     if (end_state == BattOrAgentState::INIT_SENT)
@@ skipping 21 matching lines @@
     // state machine.
     DCHECK(end_state == BattOrAgentState::START_TRACING_COMPLETE);
 
     OnMessageRead(true, BATTOR_MESSAGE_TYPE_CONTROL_ACK,
                   ToCharVector(kStartTracingAck));
   }
 
   // Runs BattOrAgent::StopTracing until it reaches the specified state by
   // feeding it the callbacks it needs to progress.
   void RunStopTracingTo(BattOrAgentState end_state) {
-    is_command_complete_ = false;

[rnephew (Reviews Here), 2017/05/09 15:04:05]

Why is this no longer needed to be set to false? 

[charliea (OOO until 10-5), 2017/05/09 19:58:08]

(See above comment)

[aschulman, 2017/05/11 13:13:00]

Acknowledged.

-
-    GetAgent()->StopTracing();
     GetTaskRunner()->RunUntilIdle();
 
     GetAgent()->OnConnectionOpened(true);
     GetTaskRunner()->RunUntilIdle();
 
     if (end_state == BattOrAgentState::CONNECTED)
       return;
 
     OnBytesSent(true);
     if (end_state == BattOrAgentState::EEPROM_REQUEST_SENT)
       return;
 
     BattOrEEPROM eeprom;
     eeprom.r1 = 1;
     eeprom.r2 = 1;
     eeprom.r3 = 1;
     eeprom.low_gain = 1;
     eeprom.low_gain_correction_offset = 0;
     eeprom.low_gain_correction_factor = 1;
     eeprom.sd_sample_rate = 1000;
 
     OnMessageRead(true, BATTOR_MESSAGE_TYPE_CONTROL_ACK, ToCharVector(eeprom));
     if (end_state == BattOrAgentState::EEPROM_RECEIVED)
       return;
 
     OnBytesSent(true);
     if (end_state == BattOrAgentState::SAMPLES_REQUEST_SENT)
       return;
 
-    DCHECK(end_state == BattOrAgentState::CALIBRATION_FRAME_RECEIVED);
-
     BattOrFrameHeader cal_frame_header{0, sizeof(RawBattOrSample)};
     RawBattOrSample cal_frame[] = {RawBattOrSample{1, 1}};
     OnMessageRead(true, BATTOR_MESSAGE_TYPE_SAMPLES,
                   CreateFrame(cal_frame_header, cal_frame, 1));
+
+    if (end_state == BattOrAgentState::CALIBRATION_FRAME_RECEIVED)
+      return;
+
+    DCHECK(end_state == BattOrAgentState::SAMPLES_END_FRAME_RECEIVED);
+
+    BattOrFrameHeader frame_header{1, 0};
+    OnMessageRead(true, BATTOR_MESSAGE_TYPE_SAMPLES,
+                  CreateFrame(frame_header, nullptr, 0));
   }
 
   // Runs BattOrAgent::RecordClockSyncMarker until it reaches the specified
   // state by feeding it the callbacks it needs to progress.
   void RunRecordClockSyncMarkerTo(BattOrAgentState end_state) {
-    is_command_complete_ = false;
-
-    GetAgent()->RecordClockSyncMarker(kClockSyncId);
     GetTaskRunner()->RunUntilIdle();
 
     GetAgent()->OnConnectionOpened(true);
     GetTaskRunner()->RunUntilIdle();
 
     if (end_state == BattOrAgentState::CONNECTED)
       return;
 
     OnBytesSent(true);
     if (end_state == BattOrAgentState::CURRENT_SAMPLE_REQUEST_SENT)
       return;
 
     DCHECK(end_state == BattOrAgentState::RECORD_CLOCK_SYNC_MARKER_COMPLETE);
 
     uint32_t current_sample = 1;
     OnMessageRead(true, BATTOR_MESSAGE_TYPE_CONTROL_ACK,
                   ToCharVector(current_sample));
   }
 
   // Runs BattOrAgent::GetFirmwareGitHash until it reaches the specified
   // state by feeding it the callbacks it needs to progress.
   void RunGetFirmwareGitHashTo(BattOrAgentState end_state) {
-    is_command_complete_ = false;
-
-    GetAgent()->GetFirmwareGitHash();
     GetTaskRunner()->RunUntilIdle();
 
     GetAgent()->OnConnectionOpened(true);
     GetTaskRunner()->RunUntilIdle();
 
     if (end_state == BattOrAgentState::CONNECTED)
       return;
 
     OnBytesSent(true);
     if (end_state == BattOrAgentState::INIT_SENT)
@@ skipping 36 matching lines @@
   bool is_command_complete_;
   BattOrError command_error_;
   std::string trace_;
   std::string firmware_git_hash_;
 };
 
 TEST_F(BattOrAgentTest, StartTracing) {
   testing::InSequence s;
   EXPECT_CALL(*GetAgent()->GetConnection(), Open());
 
-  EXPECT_CALL(*GetAgent()->GetConnection(), Flush());
   BattOrControlMessage init_msg{BATTOR_CONTROL_MESSAGE_TYPE_INIT, 0, 0};
   EXPECT_CALL(
       *GetAgent()->GetConnection(),
       SendBytes(BATTOR_MESSAGE_TYPE_CONTROL,
                 BufferEq(&init_msg, sizeof(init_msg)), sizeof(init_msg)));
 
   EXPECT_CALL(*GetAgent()->GetConnection(),
               ReadMessage(BATTOR_MESSAGE_TYPE_CONTROL_ACK));
 
   BattOrControlMessage set_gain_msg{BATTOR_CONTROL_MESSAGE_TYPE_SET_GAIN,
                                     BATTOR_GAIN_LOW, 0};
   EXPECT_CALL(*GetAgent()->GetConnection(),
               SendBytes(BATTOR_MESSAGE_TYPE_CONTROL,
                         BufferEq(&set_gain_msg, sizeof(set_gain_msg)),
                         sizeof(set_gain_msg)));
 
   EXPECT_CALL(*GetAgent()->GetConnection(),
               ReadMessage(BATTOR_MESSAGE_TYPE_CONTROL_ACK));
 
   BattOrControlMessage start_tracing_msg{
       BATTOR_CONTROL_MESSAGE_TYPE_START_SAMPLING_SD, 0, 0};
   EXPECT_CALL(*GetAgent()->GetConnection(),
               SendBytes(BATTOR_MESSAGE_TYPE_CONTROL,
                         BufferEq(&start_tracing_msg, sizeof(start_tracing_msg)),
                         sizeof(start_tracing_msg)));
 
   EXPECT_CALL(*GetAgent()->GetConnection(),
               ReadMessage(BATTOR_MESSAGE_TYPE_CONTROL_ACK));
 
+  GetAgent()->StartTracing();
   RunStartTracingTo(BattOrAgentState::START_TRACING_COMPLETE);
   EXPECT_TRUE(IsCommandComplete());
   EXPECT_EQ(BATTOR_ERROR_NONE, GetCommandError());
 }
 
 TEST_F(BattOrAgentTest, StartTracingFailsWithoutConnection) {
   GetAgent()->StartTracing();
   GetTaskRunner()->RunUntilIdle();
 
   GetAgent()->OnConnectionOpened(false);
   GetTaskRunner()->RunUntilIdle();
 
   EXPECT_TRUE(IsCommandComplete());
   EXPECT_EQ(BATTOR_ERROR_CONNECTION_FAILED, GetCommandError());
 }
 
 TEST_F(BattOrAgentTest, StartTracingFailsIfInitSendFails) {
+  GetAgent()->StartTracing();
   RunStartTracingTo(BattOrAgentState::CONNECTED);
   OnBytesSent(false);
 
   EXPECT_TRUE(IsCommandComplete());
   EXPECT_EQ(BATTOR_ERROR_SEND_ERROR, GetCommandError());
 }
 
-TEST_F(BattOrAgentTest, StartTracingFailsIfInitAckReadFails) {
+TEST_F(BattOrAgentTest, StartTracingSucceedsAfterInitAckReadFails) {
+  GetAgent()->StartTracing();
+
   RunStartTracingTo(BattOrAgentState::INIT_SENT);
+  OnMessageRead(false, BATTOR_MESSAGE_TYPE_CONTROL_ACK, nullptr);
 
-  for (int i = 0; i < 21; i++) {
-    OnMessageRead(false, BATTOR_MESSAGE_TYPE_CONTROL_ACK, nullptr);
+  EXPECT_FALSE(IsCommandComplete());
 
-    // Bytes will be sent because INIT will be retried.
-    OnBytesSent(true);
-  }
+  RunStartTracingTo(BattOrAgentState::START_TRACING_COMPLETE);
 
   EXPECT_TRUE(IsCommandComplete());
-  EXPECT_EQ(BATTOR_ERROR_TOO_MANY_INIT_RETRIES, GetCommandError());
+  EXPECT_EQ(BATTOR_ERROR_NONE, GetCommandError());
 }
 
-TEST_F(BattOrAgentTest, StartTracingFailsIfInitWrongAckRead) {
+TEST_F(BattOrAgentTest, StartTracingSucceedsAfterInitWrongAckRead) {
+  GetAgent()->StartTracing();
+
   RunStartTracingTo(BattOrAgentState::INIT_SENT);
+  OnMessageRead(true, BATTOR_MESSAGE_TYPE_CONTROL_ACK,
+                ToCharVector(kStartTracingAck));
 
-  for (int i = 0; i < 21; i++) {
-    OnMessageRead(true, BATTOR_MESSAGE_TYPE_CONTROL_ACK,
-                  ToCharVector(kStartTracingAck));
+  EXPECT_FALSE(IsCommandComplete());
 
-    // Bytes will be sent because INIT will be retried.
-    OnBytesSent(true);
-  }
+  RunStartTracingTo(BattOrAgentState::START_TRACING_COMPLETE);
 
   EXPECT_TRUE(IsCommandComplete());
-  EXPECT_EQ(BATTOR_ERROR_TOO_MANY_INIT_RETRIES, GetCommandError());
+  EXPECT_EQ(BATTOR_ERROR_NONE, GetCommandError());
 }
 
-TEST_F(BattOrAgentTest, StartTracingFailsIfSetGainSendFails) {
+TEST_F(BattOrAgentTest, StartTracingSucceedsAfterSetGainSendFails) {

[charliea (OOO until 10-5), 2017/05/12 22:03:42]

o_O is this passing right now? The name of the test is
"StartTracingSucceedsAfter...", but the test asserts that StartTracing fails
after an init send fails.

It seems like EXPECT_TRUE(IsCommandComplete()); should fail because the
BattOrAgent is waiting for more data for its retry attempt.

[aschulman, 2017/05/15 18:49:17]

right, this should say "StartTracingFails..." because the send failed. we fail
immediately if a send fails because there is no other reason for that than a
completely dead serial connection to the BattOr. there is no benefit to
retrying.

[charliea (OOO until 10-5), 2017/05/15 19:03:10]

Ah, okay. So we only retry the command if a receive fails, not if a send fails?

[aschulman, 2017/05/15 22:26:25]

correct

+  GetAgent()->StartTracing();
+
   RunStartTracingTo(BattOrAgentState::INIT_SENT);
   OnBytesSent(false);
 
   EXPECT_TRUE(IsCommandComplete());
   EXPECT_EQ(BATTOR_ERROR_SEND_ERROR, GetCommandError());
 }
 
-TEST_F(BattOrAgentTest, StartTracingFailsIfSetGainAckReadFails) {
+TEST_F(BattOrAgentTest, StartTracingSucceedsAfterSetGainAckReadFails) {
+  GetAgent()->StartTracing();
+
   RunStartTracingTo(BattOrAgentState::SET_GAIN_SENT);
   OnMessageRead(false, BATTOR_MESSAGE_TYPE_CONTROL_ACK, nullptr);
 
+  EXPECT_FALSE(IsCommandComplete());
+
+  RunStartTracingTo(BattOrAgentState::START_TRACING_COMPLETE);
+
   EXPECT_TRUE(IsCommandComplete());
-  EXPECT_EQ(BATTOR_ERROR_RECEIVE_ERROR, GetCommandError());
+  EXPECT_EQ(BATTOR_ERROR_NONE, GetCommandError());
 }
 
-TEST_F(BattOrAgentTest, StartTracingFailsIfSetGainWrongAckRead) {
+TEST_F(BattOrAgentTest, StartTracingSucceedsAfterSetGainWrongAckRead) {
+  GetAgent()->StartTracing();
+
   RunStartTracingTo(BattOrAgentState::SET_GAIN_SENT);
   OnMessageRead(true, BATTOR_MESSAGE_TYPE_CONTROL_ACK,
                 ToCharVector(kStartTracingAck));
 
+  EXPECT_FALSE(IsCommandComplete());
+
+  RunStartTracingTo(BattOrAgentState::START_TRACING_COMPLETE);
+
   EXPECT_TRUE(IsCommandComplete());
-  EXPECT_EQ(BATTOR_ERROR_UNEXPECTED_MESSAGE, GetCommandError());
+  EXPECT_EQ(BATTOR_ERROR_NONE, GetCommandError());
 }
 
 TEST_F(BattOrAgentTest, StartTracingFailsIfStartTracingSendFails) {
+  GetAgent()->StartTracing();
   RunStartTracingTo(BattOrAgentState::INIT_SENT);
   OnBytesSent(false);
 
   EXPECT_TRUE(IsCommandComplete());
   EXPECT_EQ(BATTOR_ERROR_SEND_ERROR, GetCommandError());
 }
 
-TEST_F(BattOrAgentTest, StartTracingSucceedsAfterRetriesIfWrongAckRead) {
-  RunStartTracingTo(BattOrAgentState::CONNECTED);
+TEST_F(BattOrAgentTest, StartTracingSucceedsAfterWrongAckRead) {
+  GetAgent()->StartTracing();
 
-  for (int i = 0; i < 4; i++) {
-    // Go through the correct init sequence, but give the wrong ack to
-    // START_TRACING.
-    OnBytesSent(true);
-    OnMessageRead(true, BATTOR_MESSAGE_TYPE_CONTROL_ACK,
-                  ToCharVector(kInitAck));
-    OnBytesSent(true);
-    OnMessageRead(true, BATTOR_MESSAGE_TYPE_CONTROL_ACK,
-                  ToCharVector(kSetGainAck));
-    OnBytesSent(true);
-    OnMessageRead(true, BATTOR_MESSAGE_TYPE_CONTROL_ACK,
-                  ToCharVector(kInitAck));
-  }
+  // Go through the correct init sequence, but give the wrong ack to
+  // START_TRACING.
+  RunStartTracingTo(BattOrAgentState::START_TRACING_SENT);
+  OnMessageRead(true, BATTOR_MESSAGE_TYPE_CONTROL_ACK, ToCharVector(kInitAck));
 
-  // On the last attempt, give the correct ack to START_TRACING.
-  OnBytesSent(true);
-  OnMessageRead(true, BATTOR_MESSAGE_TYPE_CONTROL_ACK, ToCharVector(kInitAck));
-  OnBytesSent(true);
-  OnMessageRead(true, BATTOR_MESSAGE_TYPE_CONTROL_ACK,
-                ToCharVector(kSetGainAck));
-  OnBytesSent(true);
-  OnMessageRead(true, BATTOR_MESSAGE_TYPE_CONTROL_ACK,
-                ToCharVector(kStartTracingAck));
+  EXPECT_FALSE(IsCommandComplete());
+
+  RunStartTracingTo(BattOrAgentState::START_TRACING_COMPLETE);
 
   EXPECT_TRUE(IsCommandComplete());
   EXPECT_EQ(BATTOR_ERROR_NONE, GetCommandError());
 }
 
-TEST_F(BattOrAgentTest, StartTracingSucceedsAfterRetriesWithReadFailure) {
-  RunStartTracingTo(BattOrAgentState::CONNECTED);
+TEST_F(BattOrAgentTest, StartTracingSucceedsAfterReadFails) {
+  GetAgent()->StartTracing();
 
-  for (int i = 0; i < 4; i++) {
-    // Go through the correct init sequence, but indicate that we failed to read
-    // the START_TRACING ack.
-    OnBytesSent(true);
-    OnMessageRead(true, BATTOR_MESSAGE_TYPE_CONTROL_ACK,
-                  ToCharVector(kInitAck));
-    OnBytesSent(true);
-    OnMessageRead(true, BATTOR_MESSAGE_TYPE_CONTROL_ACK,
-                  ToCharVector(kSetGainAck));
-    OnBytesSent(true);
-    OnMessageRead(false, BATTOR_MESSAGE_TYPE_CONTROL_ACK, nullptr);
-  }
+  // Go through the correct init sequence, but indicate that we failed to read
+  // the START_TRACING ack.
+  RunStartTracingTo(BattOrAgentState::START_TRACING_SENT);
+  OnMessageRead(false, BATTOR_MESSAGE_TYPE_CONTROL_ACK, nullptr);
+
+  EXPECT_FALSE(IsCommandComplete());
 
   // On the last attempt, give the correct ack to START_TRACING.
-  OnBytesSent(true);
-  OnMessageRead(true, BATTOR_MESSAGE_TYPE_CONTROL_ACK, ToCharVector(kInitAck));
-  OnBytesSent(true);
-  OnMessageRead(true, BATTOR_MESSAGE_TYPE_CONTROL_ACK,
-                ToCharVector(kSetGainAck));
-  OnBytesSent(true);
+  RunStartTracingTo(BattOrAgentState::START_TRACING_SENT);
   OnMessageRead(true, BATTOR_MESSAGE_TYPE_CONTROL_ACK,
                 ToCharVector(kStartTracingAck));
 
   EXPECT_TRUE(IsCommandComplete());
   EXPECT_EQ(BATTOR_ERROR_NONE, GetCommandError());
 }
 
-TEST_F(BattOrAgentTest, StartTracingFailsIfStartTracingWrongAckReadTooMuch) {
-  RunStartTracingTo(BattOrAgentState::CONNECTED);
+TEST_F(BattOrAgentTest, StartTracingSucceedsAfterSamplesReadDuringInit) {
+  GetAgent()->StartTracing();
 
-  for (int i = 0; i < 5; i++) {
-    // Go through the correct init sequence, but give the wrong ack to
-    // START_TRACING.
-    OnBytesSent(true);
-    OnMessageRead(true, BATTOR_MESSAGE_TYPE_CONTROL_ACK,
-                  ToCharVector(kInitAck));
-    OnBytesSent(true);
-    OnMessageRead(true, BATTOR_MESSAGE_TYPE_CONTROL_ACK,
-                  ToCharVector(kSetGainAck));
-    OnBytesSent(true);
-    OnMessageRead(true, BATTOR_MESSAGE_TYPE_CONTROL_ACK,
-                  ToCharVector(kInitAck));
-  }
-
-  EXPECT_TRUE(IsCommandComplete());
-  EXPECT_EQ(BATTOR_ERROR_TOO_MANY_START_TRACING_RETRIES, GetCommandError());
-}
-
-TEST_F(BattOrAgentTest, StartTracingSucceedsWithOneInitFailure) {
   RunStartTracingTo(BattOrAgentState::INIT_SENT);
 
-  // Send some samples instead of an INIT ACK. This will force an INIT retry.
+  // Send some samples instead of an INIT ACK. This will force an command retry.

[charliea (OOO until 10-5), 2017/05/12 22:03:42]

s/an command/a command

[aschulman, 2017/05/15 18:49:16]

Done.

   BattOrFrameHeader frame_header{1, 3 * sizeof(RawBattOrSample)};
   RawBattOrSample frame[] = {
       RawBattOrSample{1, 1}, RawBattOrSample{2, 2}, RawBattOrSample{3, 3},
   };
   OnMessageRead(true, BATTOR_MESSAGE_TYPE_SAMPLES,
                 CreateFrame(frame_header, frame, 3));
 
-  OnBytesSent(true);
-  OnMessageRead(true, BATTOR_MESSAGE_TYPE_CONTROL_ACK, ToCharVector(kInitAck));
+  EXPECT_FALSE(IsCommandComplete());
 
-  OnBytesSent(true);
-  OnMessageRead(true, BATTOR_MESSAGE_TYPE_CONTROL_ACK,
-                ToCharVector(kSetGainAck));
-
-  OnBytesSent(true);
-  OnMessageRead(true, BATTOR_MESSAGE_TYPE_CONTROL_ACK,
-                ToCharVector(kStartTracingAck));
+  RunStartTracingTo(BattOrAgentState::START_TRACING_COMPLETE);
 
   EXPECT_TRUE(IsCommandComplete());
   EXPECT_EQ(BATTOR_ERROR_NONE, GetCommandError());
 }
 
+TEST_F(BattOrAgentTest, StartTracingFailsAfterTooManyFails) {

[charliea (OOO until 10-5), 2017/05/12 22:03:43]

I think that "StartTracingFailsAfterTooManyCumulativeFailures" would be a better
indicator of what this test is actually testing (both that there's a failure
limit, and that the failure limit works between different stages of the same
command)

[aschulman, 2017/05/15 18:49:16]

Done.

+  GetAgent()->StartTracing();
+
+  for (int i = 0; i < 9; i++) {
+    RunStartTracingTo(BattOrAgentState::SET_GAIN_SENT);
+    OnMessageRead(false, BATTOR_MESSAGE_TYPE_CONTROL_ACK, nullptr);
+
+    EXPECT_FALSE(IsCommandComplete());
+  }
+
+  RunStartTracingTo(BattOrAgentState::SET_GAIN_SENT);
+  OnMessageRead(false, BATTOR_MESSAGE_TYPE_CONTROL_ACK, nullptr);
+
+  EXPECT_TRUE(IsCommandComplete());
+  EXPECT_EQ(BATTOR_ERROR_TOO_MANY_COMMAND_RETRIES, GetCommandError());
+}
+
 TEST_F(BattOrAgentTest, StopTracing) {
   testing::InSequence s;
   EXPECT_CALL(*GetAgent()->GetConnection(), Open());
 
   BattOrControlMessage request_eeprom_msg{
       BATTOR_CONTROL_MESSAGE_TYPE_READ_EEPROM, sizeof(BattOrEEPROM), 0};
   EXPECT_CALL(
       *GetAgent()->GetConnection(),
       SendBytes(BATTOR_MESSAGE_TYPE_CONTROL,
                 BufferEq(&request_eeprom_msg, sizeof(request_eeprom_msg)),
                 sizeof(request_eeprom_msg)));
 
   EXPECT_CALL(*GetAgent()->GetConnection(),
               ReadMessage(BATTOR_MESSAGE_TYPE_CONTROL_ACK));
 
   BattOrControlMessage request_samples_msg{
       BATTOR_CONTROL_MESSAGE_TYPE_READ_SD_UART, 0, 0};
   EXPECT_CALL(
       *GetAgent()->GetConnection(),
       SendBytes(BATTOR_MESSAGE_TYPE_CONTROL,
                 BufferEq(&request_samples_msg, sizeof(request_samples_msg)),
                 sizeof(request_samples_msg)));
 
   // We send the agent four frames: a calibration frame, and two real frames,
   // and one zero-length frame to indicate that we're done.
   EXPECT_CALL(*GetAgent()->GetConnection(),
               ReadMessage(BATTOR_MESSAGE_TYPE_SAMPLES))
       .Times(4);
 
+  GetAgent()->StopTracing();
   RunStopTracingTo(BattOrAgentState::SAMPLES_REQUEST_SENT);
 
   // Send the calibration frame.
   BattOrFrameHeader cal_frame_header{0, 2 * sizeof(RawBattOrSample)};
   RawBattOrSample cal_frame[] = {
       RawBattOrSample{1, 1}, RawBattOrSample{2, 2},
   };
   OnMessageRead(true, BATTOR_MESSAGE_TYPE_SAMPLES,
                 CreateFrame(cal_frame_header, cal_frame, 2));
 
@@ skipping 30 matching lines @@
   GetTaskRunner()->RunUntilIdle();
 
   GetAgent()->OnConnectionOpened(false);
   GetTaskRunner()->RunUntilIdle();
 
   EXPECT_TRUE(IsCommandComplete());
   EXPECT_EQ(BATTOR_ERROR_CONNECTION_FAILED, GetCommandError());
 }
 
 TEST_F(BattOrAgentTest, StopTracingFailsIfEEPROMRequestSendFails) {
+  GetAgent()->StopTracing();
   RunStopTracingTo(BattOrAgentState::CONNECTED);
   OnBytesSent(false);
 
   EXPECT_TRUE(IsCommandComplete());
   EXPECT_EQ(BATTOR_ERROR_SEND_ERROR, GetCommandError());
 }
 
-TEST_F(BattOrAgentTest, StopTracingRetriesEEPROMRead) {
+TEST_F(BattOrAgentTest, StopTracingSucceedsAfterEEPROMReadFails) {
+  GetAgent()->StopTracing();
+
   RunStopTracingTo(BattOrAgentState::EEPROM_REQUEST_SENT);
-
   OnMessageRead(false, BATTOR_MESSAGE_TYPE_CONTROL_ACK, nullptr);
 
-  OnMessageRead(true, BATTOR_MESSAGE_TYPE_CONTROL_ACK,
-                ToCharVector(BattOrEEPROM()));
+  EXPECT_FALSE(IsCommandComplete());
 
-  // Give confirmation that the samples request was sent.
-  OnBytesSent(true);
-
-  BattOrFrameHeader cal_frame_header{0, sizeof(RawBattOrSample)};
-  RawBattOrSample cal_frame[] = {RawBattOrSample{1, 1}};
-  OnMessageRead(true, BATTOR_MESSAGE_TYPE_SAMPLES,
-                CreateFrame(cal_frame_header, cal_frame, 1));
-
-  BattOrFrameHeader frame_header{1, 0};
-  OnMessageRead(true, BATTOR_MESSAGE_TYPE_SAMPLES,
-                CreateFrame(frame_header, nullptr, 0));
+  RunStopTracingTo(BattOrAgentState::SAMPLES_END_FRAME_RECEIVED);
 
   EXPECT_TRUE(IsCommandComplete());
   EXPECT_EQ(BATTOR_ERROR_NONE, GetCommandError());
 }
 
-TEST_F(BattOrAgentTest, StopTracingFailsIfEEPROMReadFails) {
-  RunStopTracingTo(BattOrAgentState::EEPROM_REQUEST_SENT);
+TEST_F(BattOrAgentTest, StopTracingSucceedsAfterEEPROMWrongAckRead) {
+  GetAgent()->StopTracing();
 
-  for (int i = 0; i < 21; i++) {
-    OnMessageRead(false, BATTOR_MESSAGE_TYPE_CONTROL_ACK, nullptr);
-  }
-
-  EXPECT_TRUE(IsCommandComplete());
-  EXPECT_EQ(BATTOR_ERROR_RECEIVE_ERROR, GetCommandError());
-}
-
-TEST_F(BattOrAgentTest, StopTracingFailsIfEEPROMWrongAckRead) {
   RunStopTracingTo(BattOrAgentState::EEPROM_REQUEST_SENT);
   OnMessageRead(true, BATTOR_MESSAGE_TYPE_CONTROL_ACK, ToCharVector(kInitAck));
 
+  EXPECT_FALSE(IsCommandComplete());
+
+  RunStopTracingTo(BattOrAgentState::SAMPLES_END_FRAME_RECEIVED);
+
   EXPECT_TRUE(IsCommandComplete());
-  EXPECT_EQ(BATTOR_ERROR_UNEXPECTED_MESSAGE, GetCommandError());
+  EXPECT_EQ(BATTOR_ERROR_NONE, GetCommandError());
 }
 
-TEST_F(BattOrAgentTest, StopTracingFailsIfRequestSamplesFails) {
+TEST_F(BattOrAgentTest, StopTracingFailsIfSendamplesRequestFails) {
+  GetAgent()->StopTracing();
+
   RunStopTracingTo(BattOrAgentState::EEPROM_RECEIVED);
   OnBytesSent(false);
 
   EXPECT_TRUE(IsCommandComplete());
   EXPECT_EQ(BATTOR_ERROR_SEND_ERROR, GetCommandError());
 }
 
-TEST_F(BattOrAgentTest, StopTracingSucceedsWithOneCalibrationFrameReadFailure) {
+TEST_F(BattOrAgentTest, StopTracingSucceedsAfterCalibrationFrameReadFailure) {
+  GetAgent()->StopTracing();
+
   RunStopTracingTo(BattOrAgentState::SAMPLES_REQUEST_SENT);
+  // Make a read fail in order to make sure that the agent will retry.
+  OnMessageRead(false, BATTOR_MESSAGE_TYPE_SAMPLES, nullptr);
+
+  RunStopTracingTo(BattOrAgentState::SAMPLES_REQUEST_SENT);
+
+  EXPECT_FALSE(IsCommandComplete());
+
+  RunStopTracingTo(BattOrAgentState::SAMPLES_END_FRAME_RECEIVED);
+
+  EXPECT_TRUE(IsCommandComplete());
+  EXPECT_EQ(BATTOR_ERROR_NONE, GetCommandError());
+}
+
+TEST_F(BattOrAgentTest, StopTracingSucceedsAfterDataFrameReadFailure) {
+  GetAgent()->StopTracing();
+
+  RunStopTracingTo(BattOrAgentState::CALIBRATION_FRAME_RECEIVED);
 
   // Make a read fail in order to make sure that the agent will retry.
   OnMessageRead(false, BATTOR_MESSAGE_TYPE_SAMPLES, nullptr);
 
-  BattOrFrameHeader cal_frame_header{0, sizeof(RawBattOrSample)};
-  RawBattOrSample cal_frame[] = {RawBattOrSample{1, 1}};
-  OnMessageRead(true, BATTOR_MESSAGE_TYPE_SAMPLES,
-                CreateFrame(cal_frame_header, cal_frame, 1));
+  EXPECT_FALSE(IsCommandComplete());
 
-  BattOrFrameHeader frame_header{1, 0};
-  OnMessageRead(true, BATTOR_MESSAGE_TYPE_SAMPLES,
-                CreateFrame(frame_header, nullptr, 0));
+  RunStopTracingTo(BattOrAgentState::SAMPLES_END_FRAME_RECEIVED);
 
   EXPECT_TRUE(IsCommandComplete());
   EXPECT_EQ(BATTOR_ERROR_NONE, GetCommandError());
 }
 
 TEST_F(BattOrAgentTest, StopTracingFailsWithManyCalibrationFrameReadFailures) {
-  RunStopTracingTo(BattOrAgentState::SAMPLES_REQUEST_SENT);
+  GetAgent()->StopTracing();
 
-  // We attempt the read a max of 20 times: send that many failures.
-  for (int i = 0; i < 21; i++) {
+  for (int i = 0; i < 9; i++) {
+    RunStopTracingTo(BattOrAgentState::SAMPLES_REQUEST_SENT);
     OnMessageRead(false, BATTOR_MESSAGE_TYPE_SAMPLES, nullptr);
+
+    EXPECT_FALSE(IsCommandComplete());
   }
 
+  RunStopTracingTo(BattOrAgentState::SAMPLES_REQUEST_SENT);
+  OnMessageRead(false, BATTOR_MESSAGE_TYPE_SAMPLES, nullptr);
+
   EXPECT_TRUE(IsCommandComplete());
-  EXPECT_EQ(BATTOR_ERROR_RECEIVE_ERROR, GetCommandError());
+  EXPECT_EQ(BATTOR_ERROR_TOO_MANY_COMMAND_RETRIES, GetCommandError());
 }
 
-TEST_F(BattOrAgentTest, StopTracingSucceedsWithOneDataFrameReadFailure) {
+TEST_F(BattOrAgentTest, StopTracingFailsWithManyDataFrameReadFailures) {
+  GetAgent()->StopTracing();
+
+  for (int i = 0; i < 9; i++) {
+    RunStopTracingTo(BattOrAgentState::CALIBRATION_FRAME_RECEIVED);
+    OnMessageRead(false, BATTOR_MESSAGE_TYPE_SAMPLES, nullptr);
+
+    EXPECT_FALSE(IsCommandComplete());
+  }
+
   RunStopTracingTo(BattOrAgentState::CALIBRATION_FRAME_RECEIVED);
-
-  // Make a read fail in order to make sure that the agent will retry.
   OnMessageRead(false, BATTOR_MESSAGE_TYPE_SAMPLES, nullptr);
 
-  BattOrFrameHeader frame_header{1, 0};
+  EXPECT_TRUE(IsCommandComplete());
+  EXPECT_EQ(BATTOR_ERROR_TOO_MANY_COMMAND_RETRIES, GetCommandError());
+}
+
+TEST_F(BattOrAgentTest, StopTracingSucceedsWithFewDataFrameReadFailures) {
+  BattOrFrameHeader frame_header1{1, 1 * sizeof(RawBattOrSample)};
+  RawBattOrSample frame1[] = {RawBattOrSample{1, 1}};

[charliea (OOO until 10-5), 2017/05/12 22:03:42]

nit: if there's no frame2, just call it "frame" instead of "frame1"

[aschulman, 2017/05/15 18:49:17]

Done.

+
+  GetAgent()->StopTracing();
+
+  RunStopTracingTo(BattOrAgentState::CALIBRATION_FRAME_RECEIVED);
+  OnMessageRead(false, BATTOR_MESSAGE_TYPE_SAMPLES, nullptr);
+
+  EXPECT_FALSE(IsCommandComplete());
+
+  RunStopTracingTo(BattOrAgentState::CALIBRATION_FRAME_RECEIVED);
   OnMessageRead(true, BATTOR_MESSAGE_TYPE_SAMPLES,
-                CreateFrame(frame_header, nullptr, 0));
+                CreateFrame(frame_header1, frame1, 1));
+  OnMessageRead(false, BATTOR_MESSAGE_TYPE_SAMPLES, nullptr);
+
+  EXPECT_FALSE(IsCommandComplete());
+
+  RunStopTracingTo(BattOrAgentState::SAMPLES_END_FRAME_RECEIVED);
 
   EXPECT_TRUE(IsCommandComplete());
   EXPECT_EQ(BATTOR_ERROR_NONE, GetCommandError());
 }
 
-TEST_F(BattOrAgentTest, StopTracingFailsWithManyDataFrameReadFailures) {
-  RunStopTracingTo(BattOrAgentState::CALIBRATION_FRAME_RECEIVED);
+TEST_F(BattOrAgentTest, StopTracingSucceedsAfterSamplesReadHasWrongType) {
+  GetAgent()->StopTracing();
 
-  // We attempt the read a max of 20 times: send that many failures.
-  for (int i = 0; i < 21; i++) {
-    OnMessageRead(false, BATTOR_MESSAGE_TYPE_SAMPLES, nullptr);
-  }
+  RunStopTracingTo(BattOrAgentState::SAMPLES_REQUEST_SENT);
+  OnMessageRead(true, BATTOR_MESSAGE_TYPE_CONTROL_ACK, ToCharVector(kInitAck));
 
-  EXPECT_TRUE(IsCommandComplete());
-  EXPECT_EQ(BATTOR_ERROR_RECEIVE_ERROR, GetCommandError());
-}
+  EXPECT_FALSE(IsCommandComplete());
 
-TEST_F(BattOrAgentTest, StopTracingRetriesResetEachFrame) {
-  RunStopTracingTo(BattOrAgentState::CALIBRATION_FRAME_RECEIVED);
-
-  // Send 11 failures on two different reads: because the retry count should
-  // reset after a successful read, this should still be okay.
-  for (int i = 0; i < 11; i++) {
-    OnMessageRead(false, BATTOR_MESSAGE_TYPE_SAMPLES, nullptr);
-  }
-
-  BattOrFrameHeader frame_header1{1, 1 * sizeof(RawBattOrSample)};
-  RawBattOrSample frame1[] = {RawBattOrSample{1, 1}};
-  OnMessageRead(true, BATTOR_MESSAGE_TYPE_SAMPLES,
-                CreateFrame(frame_header1, frame1, 1));
-
-  for (int i = 0; i < 11; i++) {
-    OnMessageRead(false, BATTOR_MESSAGE_TYPE_SAMPLES, nullptr);
-  }
-
-  BattOrFrameHeader frame_header2{2, 0};
-  OnMessageRead(true, BATTOR_MESSAGE_TYPE_SAMPLES,
-                CreateFrame(frame_header2, nullptr, 0));
+  RunStopTracingTo(BattOrAgentState::SAMPLES_END_FRAME_RECEIVED);
 
   EXPECT_TRUE(IsCommandComplete());
   EXPECT_EQ(BATTOR_ERROR_NONE, GetCommandError());
 }
 
-TEST_F(BattOrAgentTest, StopTracingFailsIfSamplesReadHasWrongType) {
-  RunStopTracingTo(BattOrAgentState::SAMPLES_REQUEST_SENT);
-  OnMessageRead(true, BATTOR_MESSAGE_TYPE_CONTROL_ACK, ToCharVector(kInitAck));
-
-  EXPECT_TRUE(IsCommandComplete());
-  EXPECT_EQ(BATTOR_ERROR_UNEXPECTED_MESSAGE, GetCommandError());
-}
-
-TEST_F(BattOrAgentTest, StopTracingFailsIfCalibrationFrameHasWrongLength) {
-  RunStopTracingTo(BattOrAgentState::SAMPLES_REQUEST_SENT);
+TEST_F(BattOrAgentTest, StopTracingSucceedsAfterCalibrationFrameWrongLength) {
+  GetAgent()->StopTracing();
 
   // Send a calibration frame with a mismatch between the frame length in the
   // header and the actual frame length.
   BattOrFrameHeader cal_frame_header{0, 1 * sizeof(RawBattOrSample)};
   RawBattOrSample cal_frame[] = {
       RawBattOrSample{1, 1}, RawBattOrSample{2, 2},
   };
+
+  RunStopTracingTo(BattOrAgentState::SAMPLES_REQUEST_SENT);
   OnMessageRead(true, BATTOR_MESSAGE_TYPE_SAMPLES,
                 CreateFrame(cal_frame_header, cal_frame, 2));
 
+  EXPECT_FALSE(IsCommandComplete());
+
+  RunStopTracingTo(BattOrAgentState::SAMPLES_END_FRAME_RECEIVED);
+
   EXPECT_TRUE(IsCommandComplete());
-  EXPECT_EQ(BATTOR_ERROR_UNEXPECTED_MESSAGE, GetCommandError());
+  EXPECT_EQ(BATTOR_ERROR_NONE, GetCommandError());
 }
 
-TEST_F(BattOrAgentTest, StopTracingFailsIfDataFrameHasWrongLength) {
-  RunStopTracingTo(BattOrAgentState::SAMPLES_REQUEST_SENT);
-
-  BattOrFrameHeader cal_frame_header{0, 1 * sizeof(RawBattOrSample)};
-  RawBattOrSample cal_frame[] = {
-      RawBattOrSample{1, 1},
-  };
-  OnMessageRead(true, BATTOR_MESSAGE_TYPE_SAMPLES,
-                CreateFrame(cal_frame_header, cal_frame, 1));
+TEST_F(BattOrAgentTest, StopTracingSucceedsAfterDataFrameHasWrongLength) {
+  GetAgent()->StopTracing();
 
   // Send a data frame with a mismatch between the frame length in the
   // header and the actual frame length.
   BattOrFrameHeader frame_header{1, 2 * sizeof(RawBattOrSample)};
   RawBattOrSample frame[] = {RawBattOrSample{1, 1}};
+
+  RunStopTracingTo(BattOrAgentState::CALIBRATION_FRAME_RECEIVED);
   OnMessageRead(true, BATTOR_MESSAGE_TYPE_SAMPLES,
                 CreateFrame(frame_header, frame, 1));
 
+  EXPECT_FALSE(IsCommandComplete());
+
+  RunStopTracingTo(BattOrAgentState::SAMPLES_END_FRAME_RECEIVED);
+
   EXPECT_TRUE(IsCommandComplete());
-  EXPECT_EQ(BATTOR_ERROR_UNEXPECTED_MESSAGE, GetCommandError());
+  EXPECT_EQ(BATTOR_ERROR_NONE, GetCommandError());
 }
 
-TEST_F(BattOrAgentTest, StopTracingFailsIfCalibrationFrameMissingByte) {
-  RunStopTracingTo(BattOrAgentState::SAMPLES_REQUEST_SENT);
+TEST_F(BattOrAgentTest, StopTracingSucceedsAfterCalibrationFrameMissingByte) {
+  GetAgent()->StopTracing();
 
   BattOrFrameHeader cal_frame_header{0, 2 * sizeof(RawBattOrSample)};
   RawBattOrSample cal_frame[] = {
       RawBattOrSample{1, 1}, RawBattOrSample{2, 2},
   };
 
   // Remove the last byte from the frame to make it invalid.
   std::unique_ptr<vector<char>> cal_frame_bytes =
       CreateFrame(cal_frame_header, cal_frame, 2);
   cal_frame_bytes->pop_back();
 
+  RunStopTracingTo(BattOrAgentState::SAMPLES_REQUEST_SENT);
   OnMessageRead(true, BATTOR_MESSAGE_TYPE_SAMPLES, std::move(cal_frame_bytes));
 
+  EXPECT_FALSE(IsCommandComplete());
+
+  RunStopTracingTo(BattOrAgentState::SAMPLES_END_FRAME_RECEIVED);
+
   EXPECT_TRUE(IsCommandComplete());
-  EXPECT_EQ(BATTOR_ERROR_UNEXPECTED_MESSAGE, GetCommandError());
+  EXPECT_EQ(BATTOR_ERROR_NONE, GetCommandError());
 }
 
-TEST_F(BattOrAgentTest, StopTracingFailsIfDataFrameMissingByte) {
-  RunStopTracingTo(BattOrAgentState::SAMPLES_REQUEST_SENT);
-
-  BattOrFrameHeader cal_frame_header{0, 1 * sizeof(RawBattOrSample)};
-  RawBattOrSample cal_frame[] = {
-      RawBattOrSample{1, 1},
-  };
-  OnMessageRead(true, BATTOR_MESSAGE_TYPE_SAMPLES,
-                CreateFrame(cal_frame_header, cal_frame, 1));
+TEST_F(BattOrAgentTest, StopTracingSucceedsAfterDataFrameMissingByte) {
+  GetAgent()->StopTracing();
 
   BattOrFrameHeader frame_header{1, 1 * sizeof(RawBattOrSample)};
   RawBattOrSample frame[] = {RawBattOrSample{1, 1}};
 
   // Remove the last byte from the frame to make it invalid.
   std::unique_ptr<vector<char>> frame_bytes =
       CreateFrame(frame_header, frame, 1);
   frame_bytes->pop_back();
 
+  RunStopTracingTo(BattOrAgentState::CALIBRATION_FRAME_RECEIVED);
   OnMessageRead(true, BATTOR_MESSAGE_TYPE_SAMPLES, std::move(frame_bytes));
 
+  EXPECT_FALSE(IsCommandComplete());
+
+  RunStopTracingTo(BattOrAgentState::SAMPLES_END_FRAME_RECEIVED);
+
   EXPECT_TRUE(IsCommandComplete());
-  EXPECT_EQ(BATTOR_ERROR_UNEXPECTED_MESSAGE, GetCommandError());
+  EXPECT_EQ(BATTOR_ERROR_NONE, GetCommandError());
 }
 
-TEST_F(BattOrAgentTest, StopTracingFailsIfFrameArrivesOutOfOrder) {
+TEST_F(BattOrAgentTest, StopTracingSucceedsAfterDataFrameArrivesOutOfOrder) {
+  GetAgent()->StopTracing();
+
+  // Frame with sequence number 1.
+  BattOrFrameHeader frame_header1{1, 1 * sizeof(RawBattOrSample)};
+  RawBattOrSample frame1[] = {RawBattOrSample{1, 1}};
+
+  // Skip frame with sequence number 2.
+  BattOrFrameHeader frame_header3{3, 1 * sizeof(RawBattOrSample)};
+  RawBattOrSample frame3[] = {RawBattOrSample{1, 1}};
+
   RunStopTracingTo(BattOrAgentState::CALIBRATION_FRAME_RECEIVED);
 
-  BattOrFrameHeader frame_header{1, 1 * sizeof(RawBattOrSample)};
-  RawBattOrSample frame[] = {RawBattOrSample{1, 1}};
+  OnMessageRead(true, BATTOR_MESSAGE_TYPE_SAMPLES,
+                CreateFrame(frame_header1, frame1, 1));
+  OnMessageRead(true, BATTOR_MESSAGE_TYPE_SAMPLES,
+                CreateFrame(frame_header3, frame3, 1));
 
-  OnMessageRead(true, BATTOR_MESSAGE_TYPE_SAMPLES,
-                CreateFrame(frame_header, frame, 1));
+  EXPECT_FALSE(IsCommandComplete());
 
-  // Skip frame with sequence number 2.
-  frame_header = BattOrFrameHeader{3, 1 * sizeof(RawBattOrSample)};
-  frame[0] = RawBattOrSample{1, 1};
-
-  OnMessageRead(true, BATTOR_MESSAGE_TYPE_SAMPLES,
-                CreateFrame(frame_header, frame, 1));
+  RunStopTracingTo(BattOrAgentState::SAMPLES_END_FRAME_RECEIVED);
 
   EXPECT_TRUE(IsCommandComplete());
-  EXPECT_EQ(BATTOR_ERROR_UNEXPECTED_MESSAGE, GetCommandError());
+  EXPECT_EQ(BATTOR_ERROR_NONE, GetCommandError());
 }
 
 TEST_F(BattOrAgentTest, RecordClockSyncMarker) {
   testing::InSequence s;
   EXPECT_CALL(*GetAgent()->GetConnection(), Open());
 
   BattOrControlMessage request_current_sample_msg{
       BATTOR_CONTROL_MESSAGE_TYPE_READ_SAMPLE_COUNT, 0, 0};
   EXPECT_CALL(*GetAgent()->GetConnection(),
               SendBytes(BATTOR_MESSAGE_TYPE_CONTROL,
                         BufferEq(&request_current_sample_msg,
                                  sizeof(request_current_sample_msg)),
                         sizeof(request_current_sample_msg)));
 
   EXPECT_CALL(*GetAgent()->GetConnection(),
               ReadMessage(BATTOR_MESSAGE_TYPE_CONTROL_ACK));
 
+  GetAgent()->RecordClockSyncMarker(kClockSyncId);
   RunRecordClockSyncMarkerTo(
       BattOrAgentState::RECORD_CLOCK_SYNC_MARKER_COMPLETE);
 
   EXPECT_TRUE(IsCommandComplete());
   EXPECT_EQ(BATTOR_ERROR_NONE, GetCommandError());
 }
 
 TEST_F(BattOrAgentTest, RecordClockSyncMarkerPrintsInStopTracingResult) {
   // Record a clock sync marker that says CLOCK_SYNC_ID happened at sample #2.
+  GetAgent()->RecordClockSyncMarker(kClockSyncId);
+
   RunRecordClockSyncMarkerTo(BattOrAgentState::CURRENT_SAMPLE_REQUEST_SENT);
 
   uint32_t current_sample = 1;
   OnMessageRead(true, BATTOR_MESSAGE_TYPE_CONTROL_ACK,
                 ToCharVector(current_sample));
 
   EXPECT_TRUE(IsCommandComplete());
   EXPECT_EQ(BATTOR_ERROR_NONE, GetCommandError());
 
+  GetAgent()->StopTracing();
   RunStopTracingTo(BattOrAgentState::SAMPLES_REQUEST_SENT);
 
   // Now run StopTracing, and make sure that CLOCK_SYNC_ID gets printed out with
   // sample #2 (including calibration frame samples).
   BattOrFrameHeader cal_frame_header{0, 1 * sizeof(RawBattOrSample)};
   RawBattOrSample cal_frame[] = {RawBattOrSample{1, 1}};
   OnMessageRead(true, BATTOR_MESSAGE_TYPE_SAMPLES,
                 CreateFrame(cal_frame_header, cal_frame, 1));
 
   BattOrFrameHeader frame_header1{1, 2 * sizeof(RawBattOrSample)};
@@ skipping 23 matching lines @@
   GetTaskRunner()->RunUntilIdle();
 
   GetAgent()->OnConnectionOpened(false);
   GetTaskRunner()->RunUntilIdle();
 
   EXPECT_TRUE(IsCommandComplete());
   EXPECT_EQ(BATTOR_ERROR_CONNECTION_FAILED, GetCommandError());
 }
 
 TEST_F(BattOrAgentTest, RecordClockSyncMarkerFailsIfSampleRequestSendFails) {
+  GetAgent()->RecordClockSyncMarker(kClockSyncId);
+
   RunRecordClockSyncMarkerTo(BattOrAgentState::CONNECTED);
   OnBytesSent(false);
 
   EXPECT_TRUE(IsCommandComplete());
   EXPECT_EQ(BATTOR_ERROR_SEND_ERROR, GetCommandError());
 }
 
-TEST_F(BattOrAgentTest, RecordClockSyncMarkerRetriesCurrentSampleRead) {
+TEST_F(BattOrAgentTest, RecordClockSyncMarkerFailsIfCurrentSampleReadFails) {
+  GetAgent()->RecordClockSyncMarker(kClockSyncId);
+
   RunRecordClockSyncMarkerTo(BattOrAgentState::CURRENT_SAMPLE_REQUEST_SENT);
-
   OnMessageRead(false, BATTOR_MESSAGE_TYPE_CONTROL_ACK, nullptr);
 
-  uint32_t current_sample = 1;
-  OnMessageRead(true, BATTOR_MESSAGE_TYPE_CONTROL_ACK,
-                ToCharVector(current_sample));
-
   EXPECT_TRUE(IsCommandComplete());
-  EXPECT_EQ(BATTOR_ERROR_NONE, GetCommandError());
+  EXPECT_EQ(BATTOR_ERROR_RECEIVE_ERROR, GetCommandError());
 }
 
 TEST_F(BattOrAgentTest,
        RecordClockSyncMarkerFailsIfCurrentSampleReadHasWrongType) {
+  GetAgent()->RecordClockSyncMarker(kClockSyncId);
+
   RunRecordClockSyncMarkerTo(BattOrAgentState::CURRENT_SAMPLE_REQUEST_SENT);
 
   uint32_t current_sample = 1;
   OnMessageRead(true, BATTOR_MESSAGE_TYPE_CONTROL,
                 ToCharVector(current_sample));
 
   EXPECT_TRUE(IsCommandComplete());
   EXPECT_EQ(BATTOR_ERROR_UNEXPECTED_MESSAGE, GetCommandError());
 }
 
 TEST_F(BattOrAgentTest, GetFirmwareGitHash) {
+  GetAgent()->GetFirmwareGitHash();
+
   RunGetFirmwareGitHashTo(BattOrAgentState::READ_GIT_HASH_RECEIVED);
+
   EXPECT_TRUE(IsCommandComplete());
   EXPECT_EQ(BATTOR_ERROR_NONE, GetCommandError());
   EXPECT_EQ("GITHASH", GetGitHash());
 }
 
 TEST_F(BattOrAgentTest, GetFirmwareGitHashFailsWithoutConnection) {
   GetAgent()->GetFirmwareGitHash();
+
   GetTaskRunner()->RunUntilIdle();
 
   GetAgent()->OnConnectionOpened(false);
   GetTaskRunner()->RunUntilIdle();
 
   EXPECT_TRUE(IsCommandComplete());
   EXPECT_EQ(BATTOR_ERROR_CONNECTION_FAILED, GetCommandError());
 }
 
-TEST_F(BattOrAgentTest, GetFirmwareGitHashFailsIfReadHasWrongType) {
+TEST_F(BattOrAgentTest, GetFirmwareGitHashSucceedsReadHasWrongType) {
+  GetAgent()->GetFirmwareGitHash();
+
   RunGetFirmwareGitHashTo(BattOrAgentState::GIT_FIRMWARE_HASH_REQUEST_SENT);
 
   uint32_t current_sample = 1;
   OnMessageRead(true, BATTOR_MESSAGE_TYPE_CONTROL,
                 ToCharVector(current_sample));
 
+  EXPECT_FALSE(IsCommandComplete());
+
+  RunGetFirmwareGitHashTo(BattOrAgentState::READ_GIT_HASH_RECEIVED);
+
   EXPECT_TRUE(IsCommandComplete());
-  EXPECT_EQ(BATTOR_ERROR_UNEXPECTED_MESSAGE, GetCommandError());
+  EXPECT_EQ(BATTOR_ERROR_NONE, GetCommandError());
 }
 
 TEST_F(BattOrAgentTest, GetFirmwareGitHashFailsIfInitSendFails) {
+  GetAgent()->GetFirmwareGitHash();
+
   RunGetFirmwareGitHashTo(BattOrAgentState::CONNECTED);
+
   OnBytesSent(false);
 
   EXPECT_TRUE(IsCommandComplete());
   EXPECT_EQ(BATTOR_ERROR_SEND_ERROR, GetCommandError());
 }
 
-TEST_F(BattOrAgentTest, GetFirmwareGitHashFailsIfInitAckReadFails) {
+TEST_F(BattOrAgentTest, GetFirmwareGitHashSucceedsAfterInitAckReadFails) {
+  GetAgent()->GetFirmwareGitHash();
+
   RunGetFirmwareGitHashTo(BattOrAgentState::INIT_SENT);
+  OnMessageRead(false, BATTOR_MESSAGE_TYPE_CONTROL_ACK, nullptr);
 
-  for (int i =0; i < 21; i++) {
-    OnMessageRead(false, BATTOR_MESSAGE_TYPE_CONTROL_ACK, nullptr);
+  EXPECT_FALSE(IsCommandComplete());
 
-    // Bytes will be sent because INIT will be retried.
-    OnBytesSent(true);
-  }
+  RunGetFirmwareGitHashTo(BattOrAgentState::READ_GIT_HASH_RECEIVED);
 
   EXPECT_TRUE(IsCommandComplete());
-  EXPECT_EQ(BATTOR_ERROR_TOO_MANY_INIT_RETRIES, GetCommandError());
+  EXPECT_EQ(BATTOR_ERROR_NONE, GetCommandError());
 }
 
-TEST_F(BattOrAgentTest, GetFirmwareGithashFailsIfInitWrongAckRead) {
+TEST_F(BattOrAgentTest, GetFirmwareGitHashSucceedsAfterInitWrongAckRead) {
+  GetAgent()->GetFirmwareGitHash();
+
   RunGetFirmwareGitHashTo(BattOrAgentState::INIT_SENT);
-  for (int i = 0; i < 21; i++) {
-    OnMessageRead(true, BATTOR_MESSAGE_TYPE_CONTROL_ACK,
-                  ToCharVector(kStartTracingAck));
+  OnMessageRead(true, BATTOR_MESSAGE_TYPE_CONTROL_ACK,
+                ToCharVector(kStartTracingAck));
 
-    // Bytes will be sent because INIT will be retried.
-    OnBytesSent(true);
-  }
+  EXPECT_FALSE(IsCommandComplete());
+
+  RunGetFirmwareGitHashTo(BattOrAgentState::READ_GIT_HASH_RECEIVED);
 
   EXPECT_TRUE(IsCommandComplete());
-  EXPECT_EQ(BATTOR_ERROR_TOO_MANY_INIT_RETRIES, GetCommandError());
+  EXPECT_EQ(BATTOR_ERROR_NONE, GetCommandError());
 }
 }  // namespace battor