Fix soft button object invalid states#1776
Fix soft button object invalid states#1776RHenigan merged 20 commits intosmartdevicelink:developfrom
Conversation
Make SoftButtonObject throw IllegalStateException when given empty state list
Convert throwing of IllegalStateException to setting a boolean variable, printing an error with DebugTool, and returning Changed unit tests to reflect this new intended behavior Added a private boolean variable and public getter function to make unit testing simpler The alternative to reading a flag to trigger tests failing is reading logs programmatically, which is more complicated and less reliable
base/src/main/java/com/smartdevicelink/managers/screen/SoftButtonObject.java
Outdated
Show resolved
Hide resolved
base/src/main/java/com/smartdevicelink/managers/screen/SoftButtonObject.java
Show resolved
Hide resolved
base/src/main/java/com/smartdevicelink/managers/screen/SoftButtonObject.java
Outdated
Show resolved
Hide resolved
...android/src/androidTest/java/com/smartdevicelink/managers/screen/SoftButtonManagerTests.java
Outdated
Show resolved
Hide resolved
...android/src/androidTest/java/com/smartdevicelink/managers/screen/SoftButtonManagerTests.java
Outdated
Show resolved
Hide resolved
base/src/main/java/com/smartdevicelink/managers/screen/SoftButtonObject.java
Outdated
Show resolved
Hide resolved
base/src/main/java/com/smartdevicelink/managers/screen/SoftButtonObject.java
Show resolved
Hide resolved
Remove attemptedToAssignEmptyStateList and getAttemptedToAssignEmptyStateList() from SoftButtonObject Change unit tests to reflect the removals stated above Add testConstructingSoftButtonObjectWithNonEmptyStateList() and testAssignNonEmptyStateListToSoftButtonObject() to test normal functionality
|
Fixes #1774 This PR is ready for review. RiskNone Testing Plan
Unit Tests
Core TestsTests performed by modifying a local clone of hello_sdl_android
SDL Core / 8.0.0 / master / 68f082169e0a40fccd9eb0db3c83911c28870f07 / module tested against: SDL Core on Ubuntu 20.04.3 LTS Virtual Machine SummaryThis pull request adds an if statement to ChangelogBreaking Changes
Enhancements
Bug FixesCLA
|
| import com.smartdevicelink.proxy.rpc.listeners.OnRPCNotificationListener; | ||
| import com.smartdevicelink.test.Validator; | ||
|
|
||
| import org.junit.Assert; |
There was a problem hiding this comment.
We can remove this unused import
| import static org.mockito.Mockito.mock; | ||
| import static org.mockito.Mockito.when; | ||
|
|
||
| import android.database.sqlite.SQLiteCantOpenDatabaseException; |
There was a problem hiding this comment.
we can remove this. unused import
| * {@link SoftButtonManager} | ||
| */ | ||
| @RunWith(AndroidJUnit4.class) | ||
| //@RunWith(JUnit4.class) |
There was a problem hiding this comment.
We can remove this comment as well
| SoftButtonObject softButtonObject = new SoftButtonObject("hi", softButtonState, null); | ||
|
|
||
| softButtonObject.setStates(stateList); | ||
| assertNotEquals(stateList, softButtonObject.getStates()); |
There was a problem hiding this comment.
Rather than assert that these are not equal we could assert that the state on the Object is left unchanged after calling setStates.
assertEquals(softButtonState, softButtonObject.getStates)
You may want to use the constructor that takes a list for states rather than a single state. or you could make a list out of softButtonState before you make the assertion
|
|
||
| stateList.add(softButtonState); | ||
| softButtonObject.setStates(stateList); | ||
| assertEquals(stateList, softButtonObject.getStates()); |
There was a problem hiding this comment.
For this test it may be beneficial to have stateList contain a state that is different than what was passed in the constructor. this way we are verifying the states are in fact being updated.
| this.currentStateName = initialStateName; | ||
| this.buttonId = SOFT_BUTTON_ID_NOT_SET_VALUE; | ||
| this.onEventListener = onEventListener; | ||
| this.states = states; |
There was a problem hiding this comment.
Can we put this back to the same line it was previously on? this will help to reduce noise in the diff for changes that aren't really changes
There was a problem hiding this comment.
If you mean where it was when I was using setStates() in the constructor, I don't think I can without making the diff noisier, because now the check is happening there and the check obviously has to happen first to prevent assignment in the error case. This change is just reverting the states assignment back to how it was in develop before I forked. Maybe I could squash commits? But I think that might be more trouble than it's worth
There was a problem hiding this comment.
Just move this.states = states; back under this.name= name;
There was a problem hiding this comment.
Oh okay, will do
| } | ||
| return null; | ||
| } | ||
|
|
There was a problem hiding this comment.
just cleaning up whitespace
| import java.util.Collections; | ||
| import java.util.List; | ||
|
|
||
|
|
There was a problem hiding this comment.
just cleaning up white space.
Remove whitespace and unused imports Change logic of tests based on smartdevicelink#1776 feedback Rename testConstructingSoftButtonObjectWithNonEmptyStateList() to testConstructSoftButtonObjectWithNonEmptyStateList() to match naming of related tests
Add testAssignSameNameStateListToSoftButtonObject() to test the new behavior
|
Fixes #1774 This PR is ready for review. RiskNone Testing Plan
Unit Tests
Core TestsTests performed by modifying a local clone of hello_sdl_android
SDL Core / 8.0.0 / master / 68f082169e0a40fccd9eb0db3c83911c28870f07 / module tested against: SDL Core on Ubuntu 20.04.3 LTS Virtual Machine SummaryThis pull request adds an if statement to ChangelogBreaking Changes
Enhancements
Bug FixesCLA
|
| // If the list of states is empty, throw an error with DebugTool and return | ||
| if (states.isEmpty()) { | ||
| DebugTool.logError(TAG,"The state list is empty"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Alignment note:
On iOS I did this
BOOL hasStateWithInitialName = NO;
for (SDLSoftButtonState *state in states) {
if ([state.name isEqualToString:initialStateName]) {
hasStateWithInitialName = YES;
}
}
NSAssert(![self sdl_hasTwoStatesOfSameName:states], @"A SoftButtonObject must have states with different names.");
NSAssert(hasStateWithInitialName, @"A SoftButtonObject must have a state with initialStateName.");
if ([self sdl_hasTwoStatesOfSameName:states]) { return nil; }
if (!hasStateWithInitialName) { return nil; }The advantage this has is only throwing an exception in DEBUG builds (I don't know that is available on Java Suite). Second, this tests not only that there are states, but also that the initial state name is available. The for loop won't run if there are no states, so the boolean will just stay false. That check at least should be a part of this PR as well.
Last, I also made a change to SoftButtonState on iOS to check if both text and image are null, which is an invalid state configuration. I think that should be a part of this PR as well.
There was a problem hiding this comment.
Understood, I'll work on implementing this
base/src/main/java/com/smartdevicelink/managers/screen/SoftButtonObject.java
Show resolved
Hide resolved
Make constructor check for valid initialStateName in state list Adjust tests to reflect new behavior
|
I have added the |
|
Fixes #1774 This PR is ready for review. RiskNone Testing Plan
Unit Tests
Core TestsTests performed by modifying a local clone of hello_sdl_android
SDL Core / 8.0.0 / master / 68f082169e0a40fccd9eb0db3c83911c28870f07 / module tested against: SDL Core on Ubuntu 20.04.3 LTS Virtual Machine SummaryThis pull request adds an if statement to Additionally ChangelogBreaking Changes
Enhancements
Bug FixesCLA
|
Also remove comment
|
Older unit tests fail as a result of the new hasStateWithInitialName check. ScreenManagerTests.testAssigningIdsToSoftButtonObjects |
Update SoftButtonManagerTests.testSoftButtonObjectEquals()
Update ScreenManagerTests.testAssigningIdsToSoftButtonObjects())
|
|
||
| import androidx.annotation.NonNull; | ||
|
|
||
| import com.livio.BuildConfig; |
|
|
||
| import androidx.annotation.NonNull; | ||
|
|
||
| import com.livio.BuildConfig; |
base/src/main/java/com/smartdevicelink/managers/screen/SoftButtonObject.java
Show resolved
Hide resolved
RHenigan
left a comment
RHenigan
left a comment
There was a problem hiding this comment.
testAssignNonEmptyStateListToSoftButtonObject is now failing, this test would need to set a list with a state that is equivalent to the currentState. We should also test to verify the negative is true as well (confirm setting a list without the currentState fails to update)
| boolean hasStateWithCurrentName = false; | ||
| for (SoftButtonState state : states) { | ||
| if(state.getName().equals(currentStateName)) { | ||
| hasStateWithCurrentNameName = true; |
There was a problem hiding this comment.
| hasStateWithCurrentNameName = true; | |
| hasStateWithCurrentName = true; |
There was a problem hiding this comment.
Good catch, and that'll be easy to fix
There was a problem hiding this comment.
Also to avoid confusion for the developer we will still want to set the states in the case the currentState is not present in the list, but we will still want to log the error.
We should test and confirm that the SoftButtonManager can handle this error case (trying to upload a SoftButton where the currentState is not in the list)
|
Fixes #1774 This PR is ready for review. RiskNone Testing Plan
Unit Tests
Core TestsTests performed by modifying a local clone of hello_sdl_android
SDL Core / 8.0.0 / master / 68f082169e0a40fccd9eb0db3c83911c28870f07 / module tested against: SDL Core on Ubuntu 20.04.3 LTS Virtual Machine SummaryThis pull request adds an if statement to Additionally ChangelogBreaking Changes
Enhancements
Bug FixesCLA
|
Codecov Report
@@ Coverage Diff @@
## develop #1776 +/- ##
=============================================
+ Coverage 54.36% 54.40% +0.04%
- Complexity 5512 5521 +9
=============================================
Files 562 562
Lines 25515 25540 +25
Branches 3319 3327 +8
=============================================
+ Hits 13872 13896 +24
+ Misses 10383 10381 -2
- Partials 1260 1263 +3
|
|
Awaiting iOS Alignment before merging |
joeljfischer
left a comment
joeljfischer
left a comment
There was a problem hiding this comment.
This also seems to be missing the changes to SoftButtonState to ensure a state never has neither text nor an image.
| } | ||
| } | ||
| if (!hasStateWithCurrentName) { | ||
| DebugTool.logError(TAG, "A SoftButtonObject should have a state with currentStateName."); |
There was a problem hiding this comment.
| DebugTool.logError(TAG, "A SoftButtonObject should have a state with currentStateName."); | |
| DebugTool.logError(TAG, "A SoftButtonObject setting states must contain a state with currentStateName."); |
Can you also include the name of currentStateName?
There was a problem hiding this comment.
Yeah, this is an easy change
That check is already handled in the constructor of |
Rephrase debug output and add extra detail
joeljfischer
added
bug
4 participants
Fixes #1774
This PR is ready for review.
Risk
This PR adds a new public getter function to SoftButtonObject
Testing Plan
Unit Tests
testConstructSoftButtonObjectWithEmptyStateList()passes ifgetAttemptedToAssignEmptyStateList()returns true after an empty state list is passed to the main primary constructor and fails otherwise.testAssignEmptyStateListToSoftButtonObject()passes if angetAttemptedToAssignEmptyStateList()returns true after an empty state list is passed tosetStates()of an instance ofSoftButtonObjectthat already has at least one state in its state list.Core Tests
Tests performed by modifying a local clone of hello_sdl_android
SoftButtonObjectwith a state list containing a single state, added it to a list ofSoftButtonObjects, and attempted to display them to the Generic HMI usingScreenManager. Results:SoftButtonObjectwith an empty state list, added it to a list ofSoftButtonObjects, and attempted to display them to the Generic HMI usingScreenManager. Results:DebugToolstating that the state list is emptySoftButtonObjectwith a state list containing a single state, reassigned the state list to an empty list usingsetStates(), added it to a list ofSoftButtonObjects, and attempted to display them to the Generic HMI usingScreenManager. Results:DebugToolstating that the state list is emptySDL Core / 8.0.0 / master / 68f082169e0a40fccd9eb0db3c83911c28870f07 / module tested against: SDL Core on Ubuntu 20.04.3 LTS Virtual Machine
Generic HMI / 0.11.0 / master / 47e0ad42f05b27adff61befd864e79c2ab4b8cec / module tested against: Generic HMI on Ubuntu 20.04.3 LTS Virtual Machine
Summary
This pull request adds an if statement to
SoftButtonObject.setStates()that checks if the given state list is empty and displays if it is. This PR also replaces the direct state list assignment in the main constructor with usage ofsetStates()to enforce the check in the constructor without writing redundant code.Changelog
Breaking Changes
Enhancements
Bug Fixes
CLA