Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add error logging when removing empty voice commands and voice command strings #1799

Merged
merged 3 commits into from
Mar 25, 2022

Conversation

noah-livio
Copy link
Contributor

@noah-livio noah-livio commented Mar 24, 2022

Fixes #1798

This PR is ready for review.

Risk

This PR makes no API changes.

Testing Plan

  • I have verified that I have not introduced new warnings in this PR (or explain why below)
  • I have run the unit tests with this PR - N/A
  • I have tested this PR against Core and verified behavior
  • I have tested Android and Java SE

Unit Tests

N/A - This PR only adds logging

Core Tests

  1. Create VoiceCommand containing no command strings
  2. Attempt to send the VoiceCommand to an HMI using VoiceCommandManager
  3. Observe debug logs

Expected Results: The VoiceCommand fails to send and a warning is logged

Observed Results: The VoiceCommand fails to send and a warning is logged

- Ford TDK 3.4 (19353_DEVTEST_r133796)
- SDL Core v7.1.1 SDL HMI 5.5.1
- SDL Core v8.1.1 SDL HMI 5.7

Summary

Adds more logging to BaseVoiceCommandManager.removeEmptyVoiceCommands()

Changelog

Breaking Changes
  • None
Enhancements
  • None
Bug Fixes

CLA

@codecov
Copy link

codecov bot commented Mar 24, 2022

Codecov Report

❗ No coverage uploaded for pull request base (5.4.0_RC@0f75391). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##             5.4.0_RC    #1799   +/-   ##
===========================================
  Coverage            ?   54.06%           
  Complexity          ?     5522           
===========================================
  Files               ?      562           
  Lines               ?    25718           
  Branches            ?     3370           
===========================================
  Hits                ?    13905           
  Misses              ?    10556           
  Partials            ?     1257           

@joeljfischer joeljfischer added bug A defect in the library manager-screen Relating to the manager layer - screen managers labels Mar 25, 2022
@joeljfischer joeljfischer added this to In progress in 5.4.0 via automation Mar 25, 2022
5.4.0 automation moved this from In progress to Review in progress Mar 25, 2022
Copy link
Contributor

@JulianKast JulianKast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few suggestions

continue;
}
String trimmedString = voiceCommandString.trim();
if (trimmedString.length() > 0) {
voiceCommandStrings.add(trimmedString);
} else {
DebugTool.logWarning(TAG, "Empty voice command string removed");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
DebugTool.logWarning(TAG, "Empty voice command string removed");
DebugTool.logWarning(TAG, "Empty string removed from voice command");

}
}
if (voiceCommandStrings.size() > 0) {
voiceCommand.setVoiceCommands(voiceCommandStrings);
validatedVoiceCommands.add(voiceCommand);
} else {
DebugTool.logWarning(TAG, "Empty voice command removed");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
DebugTool.logWarning(TAG, "Empty voice command removed");
DebugTool.logWarning(TAG, "Voice command will not be uploaded as it contained no valid strings");

@@ -217,21 +217,27 @@ private void updateIdsOnVoiceCommands(List<VoiceCommand> voiceCommands) {
List<VoiceCommand> validatedVoiceCommands = new ArrayList<>();
for (VoiceCommand voiceCommand : voiceCommands) {
if (voiceCommand == null) {
DebugTool.logWarning(TAG, "Null voice command removed");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
DebugTool.logWarning(TAG, "Null voice command removed");
DebugTool.logWarning(TAG, "Voice command is null, it will not be uploaded");

continue;
}
List<String> voiceCommandStrings = new ArrayList<>();
for (String voiceCommandString : voiceCommand.getVoiceCommands()) {
if (voiceCommandString == null) {
DebugTool.logWarning(TAG, "Null voice command string removed");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
DebugTool.logWarning(TAG, "Null voice command string removed");
DebugTool.logWarning(TAG, "Removing null string from Voice Command");

@JulianKast JulianKast merged commit 4784a4b into 5.4.0_RC Mar 25, 2022
5.4.0 automation moved this from Review in progress to Done Mar 25, 2022
@JulianKast JulianKast deleted the bugfix/issue_1798 branch March 25, 2022 17:38
@theresalech theresalech removed this from Done in 5.4.0 Mar 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A defect in the library manager-screen Relating to the manager layer - screen managers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants