Skip to content

When pickling addon state use only builtin types, and add unit test for the pickling and unpickling process.#15556

Merged
seanbudd merged 3 commits into
nvaccess:betafrom
lukaszgo1:I15554
Oct 2, 2023
Merged

When pickling addon state use only builtin types, and add unit test for the pickling and unpickling process.#15556
seanbudd merged 3 commits into
nvaccess:betafrom
lukaszgo1:I15554

Conversation

@lukaszgo1

@lukaszgo1 lukaszgo1 commented Oct 1, 2023

Copy link
Copy Markdown
Contributor

Opened against beta, to fix regression introduced in 2023.3 development cycle.

Link to issue number:

Fixes #15554

Summary of the issue:

When pickling add-ons state it is necessary to use only builtin types, so that it can be loaded in older versions of NVDA. IN PR #15439 the backCompatToVersion was mistakenly pickled as a custom named tuple MajorMinorPatch.
This means it is impossible to start older versions of NVDA with the config from the latest beta NVDA crashes on startup.

Description of user facing changes

It should be once again possible to start older versions of NVDA with the config used with the latest beta.

Description of development approach

The manualOverridesAPIVersion is pickled as a standard tuple. To make sure similar regressions are not introduced I have added unit test covering various scenarios of converting state to, and loading it from, pickled data. Note that I have intentionally not used pickle files, since when the given custom data type is added to NVDA it will always be unpickled successfully.

Testing strategy:

  • Started code from this PR with an existing config, then inspected the state file in a Python interpreter to ensure it contains only builtin types.
  • With a state file which was updated by the code from this PR, started NVDA 2023.1, ensured it launches successfully, and can load the state.

Known issues with pull request:

None known

Code Review Checklist:

  • Documentation:
    • Change log entry
    • User Documentation
    • Developer / Technical Documentation
    • Context sensitive help for GUI changes
  • Testing:
    • Unit tests
    • System (end to end) tests
    • Manual testing
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers
    • Localization in other languages / culture than English
  • API is compatible with existing add-ons.
  • Security precautions taken.

@lukaszgo1 lukaszgo1 marked this pull request as ready for review October 1, 2023 22:01
@lukaszgo1 lukaszgo1 requested a review from a team as a code owner October 1, 2023 22:01
@lukaszgo1 lukaszgo1 requested review from seanbudd and removed request for a team October 1, 2023 22:01
@seanbudd seanbudd added this to the 2023.3 milestone Oct 1, 2023
state[AddonStateCategory.OVERRIDE_COMPATIBILITY].clear()
self[AddonStateCategory.OVERRIDE_COMPATIBILITY].clear()
self.manualOverridesAPIVersion = MajorMinorPatch(*addonAPIVersion.BACK_COMPAT_TO)
self.update(state)

@seanbudd seanbudd Oct 2, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

is there a reason why using state instead of self was changed and calling update was removed?
The current design was intentional and I believe this change introduces an unnecessary risk to the beta. We have limited time to test upgrading and downgrading NVDA with add-ons as per work in #15439

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

There is simply no state local variable anymore. Now the data is loaded into the instance by fromPickledDict, which means that the loading from the dictionary can be tested independently in unit tests. The process is covered by unit tests, which is certainly an improvement to what we had previously. Reading them should give you confidence that functionally the process remained the same.

@seanbudd seanbudd added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Oct 2, 2023
seanbudd
seanbudd previously approved these changes Oct 2, 2023

@seanbudd seanbudd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @lukaszgo1

Comment thread tests/unit/test_addonHandler/test_addonsState.py Outdated
@seanbudd seanbudd merged commit de187e7 into nvaccess:beta Oct 2, 2023
@lukaszgo1 lukaszgo1 deleted the I15554 branch October 3, 2023 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants