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

Bugfix/issue 1828 Good text failing #1831

Merged
merged 9 commits into from
Sep 21, 2022
Merged

Bugfix/issue 1828 Good text failing #1831

merged 9 commits into from
Sep 21, 2022

Conversation

ChloeMJM
Copy link
Contributor

@ChloeMJM ChloeMJM commented Sep 14, 2022

Fixes #1828

This PR is ready for review.

Risk

This PR makes minor 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).
  • I have tested Android, *Java SE, and *Java EE

Unit Tests

*Note: This branch has been tested for Android by Chloe, Julian Tested JavaSE

  • testOnShowFailBadDataDoesNotUpdateScreen() : Tests the show failure branch within the sendShow method which notifies the currentScreenDataUpdateListener of an errorState.
  • testUpdateTargetStateWithErrorStateNullDoesNotUpdateCurrentScreen(): Tests the updateTargetStateWithErrorState method to ensure that fields set to null will not be updated on the current screen.
  • testUpdateTargetStateWithErrorBadDataDoesNotUpdateCurrentScreen() : Tests the updateTargetStateWithErrorState method to ensure that fields set to bad data will not be updated on the current screen.
  • testUpdateTargetStateWithErrorBadDataAndGoodData() : Tests the updateTargetStateWithErrorState method to ensure that fields set to bad data will not be updated on the current screen but fields set with good data will be updated.

Core Tests

In Manticore verified that properties in screen manager when set would not fail when one attribute was given bad data

Summary

  • In TextAndGraphicUpdateOperation: modified the sendShow method to call currentScreenDataUpdateListener.onError(updatedState) on failure.
  • Modified the onError path of the currentScreenDataUpdateListener override to call updatePendingOperationsWithFailedScreenState(errorState), passing in an errorState.
  • In BaseTextAndGraphicManager: added in method updatePendingOperationsWithFailedScreenState which sends the errorState to updateOperation.updateTargetStateWithErrorState(errorState)
  • In TextAndGraphicUpdateOperation: added method updateTargetStateWithErrorState that ensures every field is null safe and compares the errorState to the updatedState. If they are the same, then it sets the updatedState to the currentScreenData associated with the field to keep the field from being updated with bad data.
  • Added unit tests.

Changelog

Enhancements
  • Screen data will update even if other fields may have bad data.
Bug Fixes
  • Screen data will update even if other fields may have bad data.

Tasks Remaining

  • Create fix.
  • Add unit tests.
  • Code review.

CLA

@ChloeMJM ChloeMJM changed the title Bugfix/issue 1828 Bugfix/issue 1828 Good text failing Sep 14, 2022
@codecov
Copy link

codecov bot commented Sep 14, 2022

Codecov Report

Merging #1831 (c86d26d) into develop (b99a2ca) will decrease coverage by 0.01%.
The diff coverage is 54.23%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop    #1831      +/-   ##
=============================================
- Coverage      54.04%   54.02%   -0.02%     
- Complexity      5532     5534       +2     
=============================================
  Files            562      562              
  Lines          25821    25862      +41     
  Branches        3398     3411      +13     
=============================================
+ Hits           13954    13971      +17     
- Misses         10596    10610      +14     
- Partials        1271     1281      +10     
Impacted Files Coverage Δ
...managers/screen/TextAndGraphicUpdateOperation.java 70.22% <48.48%> (-0.93%) ⬇️
...ink/managers/screen/BaseTextAndGraphicManager.java 63.29% <61.53%> (-0.88%) ⬇️
...smartdevicelink/encoder/VirtualDisplayEncoder.java 45.36% <0.00%> (-0.34%) ⬇️
...martdevicelink/transport/SdlBroadcastReceiver.java 3.02% <0.00%> (ø)
...tdevicelink/transport/SdlRouterStatusProvider.java 0.00% <0.00%> (ø)
...anagers/lifecycle/BaseSystemCapabilityManager.java 67.76% <0.00%> (+0.08%) ⬆️

@ChloeMJM ChloeMJM changed the base branch from master to develop September 14, 2022 21:26
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.

First round code review

…supersedePreviousOperations and currentOperationListener variables. Changed passed in reference from updatedState to null for currentScreenDataListener.onError() when text update has not been sent.
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.

Don't stress about code coverage, given the design, it would require a lot of rewriting of current unit tests to get coverage in areas we did not have before to get to 60%

…rentScreenDataUpdateListener.onError(updatedState) from null to updatedState.
@JulianKast JulianKast merged commit 44b38cb into develop Sep 21, 2022
@JulianKast JulianKast deleted the bugfix/issue_1828 branch September 21, 2022 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setting bad data in one T&G field then good data quickly in another can lead to the good data failing.
2 participants