Skip to content
New issue

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

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

Already on GitHub? # to your account

Fix for voice command that contains no string should be removed #1965

Conversation

FrankElias77
Copy link
Contributor

@FrankElias77 FrankElias77 commented Apr 8, 2021

Fixes #1964

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
  • I have tested this PR against Core and verified behavior (if applicable, if not applicable, explain why below).

Unit Tests

Unit Test added in SDLVoiceCommandUpdateOperationSpec file to check if pendingVoiceCommands handles voice commands with no string

Core Tests

Test applied on the example app:

Modified ProxyManager to set the voiceCommandA initial state.

Case 1: The following voiceCommands defined: voiceCommandA = ["", "Test 1", " ", "Test 2", " "], voiceCommandB = ["Test 3", "Test 4"]
Case 2: The following voiceCommands defined: voiceCommandA = ["", "Test 1", " ", "Test 2", " "]
Case 3: voiceCommandA = nil
Case 4: The following voiceCommands defined: voiceCommandA = ["", " ", "\t"]

Core version / branch / commit hash / module tested against: 7.1.0
HMI name / version / branch / commit hash / module tested against: Manticore

Summary

Update pendingVoiceCommands to prevent from sending a voice command with no string

Changelog

Breaking Changes
  • N/A
Enhancements
  • N/A
Bug Fixes
  • Update pendingVoiceCommands to prevent from sending a voice command with no string

Tasks Remaining:

N/A

CLA

@FrankElias77 FrankElias77 added the bug A defect in the library label Apr 8, 2021
@FrankElias77 FrankElias77 self-assigned this Apr 8, 2021
@codecov
Copy link

codecov bot commented Apr 8, 2021

Codecov Report

Merging #1965 (2464640) into develop (b88b7d1) will decrease coverage by 0.74%.
The diff coverage is 100.00%.

❗ Current head 2464640 differs from pull request most recent head f7a0e00. Consider uploading reports for the commit f7a0e00 to get more accurate results

@@             Coverage Diff             @@
##           develop    #1965      +/-   ##
===========================================
- Coverage    85.95%   85.20%   -0.75%     
===========================================
  Files          441      441              
  Lines        22586    22603      +17     
===========================================
- Hits         19414    19260     -154     
- Misses        3172     3343     +171     

@FrankElias77 FrankElias77 marked this pull request as ready for review April 9, 2021 15:08
@NicoleYarroch NicoleYarroch linked an issue Apr 13, 2021 that may be closed by this pull request
Copy link
Contributor

@NicoleYarroch NicoleYarroch left a comment

Choose a reason for hiding this comment

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

Left some comments.

Copy link
Contributor

@NicoleYarroch NicoleYarroch left a comment

Choose a reason for hiding this comment

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

Left some comments

Copy link
Contributor

@NicoleYarroch NicoleYarroch left a comment

Choose a reason for hiding this comment

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

Left some comments

@NicoleYarroch NicoleYarroch self-requested a review April 22, 2021 19:27
Copy link
Contributor

@NicoleYarroch NicoleYarroch left a comment

Choose a reason for hiding this comment

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

Left some comments.

Copy link
Contributor

@NicoleYarroch NicoleYarroch left a comment

Choose a reason for hiding this comment

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

Suggested renaming variables for clarity.

Copy link
Contributor

@NicoleYarroch NicoleYarroch left a comment

Choose a reason for hiding this comment

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

I made one suggestion to relocate a comment.

Can you also add the smoke tests you performed under Core Tests in the PR description?

Copy link
Contributor

@NicoleYarroch NicoleYarroch left a comment

Choose a reason for hiding this comment

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

I made some suggestions to make the tests a little more readable.

@NicoleYarroch NicoleYarroch self-requested a review May 7, 2021 11:17
…ins-no-string-should-be-removed

# Conflicts:
#	SmartDeviceLink-iOS.xcodeproj/project.pbxproj
#	SmartDeviceLink/private/SDLVoiceCommandManager.m
#	SmartDeviceLinkTests/DevAPISpecs/SDLVoiceCommandManagerSpec.m
@joeljfischer joeljfischer merged commit 57de541 into develop May 25, 2021
@joeljfischer joeljfischer deleted the bugfix/issue-1964-voiceCommand-that-contains-no-string-should-be-removed branch May 25, 2021 12:53
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug A defect in the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

voiceCommand that contains no string should be removed
3 participants