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

Address inconsistent _lazy_values after attribute set #45

Closed

Conversation

addyess
Copy link
Contributor

@addyess addyess commented Apr 13, 2023


Reason for this change

After loading any Kubernetes Object from_dict, some of it's attributes may be loaded lazily -- meaning they aren't converted to their lightkube object type until referenced, and remain attached to the object in the _lazy_values attribute until read.

If you change one of these attributes via an attribute set, the _lazy_value attribute should be invalidated otherwise a to_dict will use the lazy_value view rather than the lightkube native view


Adding unit tests that shows #44 fails in master branch

lightkube ❯ coverage run --source=lightkube -m pytest tests/ -k issue        
=================================================================================== test session starts ====================================================================================
platform linux -- Python 3.10.7, pytest-7.3.0, pluggy-1.0.0
rootdir: /home/addyess/git/addyess/lightkube
plugins: respx-0.20.1, anyio-3.6.2, asyncio-0.21.0
asyncio: mode=strict
collected 143 items / 142 deselected / 1 selected                                                                                                                                          

tests/test_dataclasses_dict.py F                                                                                                                                                     [100%]

========================================================================================= FAILURES =========================================================================================
______________________________________________________________________________________ test_issue_44 _______________________________________________________________________________________

    def test_issue_44():
        inst = C.from_dict({"c1": "val"})                               # Setup a C object without setting c2
        assert inst.to_dict() == {"c1": "val"}                          # de-serialize to a dict
        inst.c2 = [A("abc")]                                            # Add c2 list
>       assert inst.to_dict() == {"c1": "val", "c2": [{"a1": "abc"}]}   # Expect c2 list to show up in dict
E       AssertionError: assert {'c1': 'val'} == {'c1': 'val',...'a1': 'abc'}]}
E         Omitting 1 identical items, use -vv to show
E         Right contains 1 more item:
E         {'c2': [{'a1': 'abc'}]}
E         Use -v to get more diff

tests/test_dataclasses_dict.py:48: AssertionError
================================================================================= short test summary info ==================================================================================
FAILED tests/test_dataclasses_dict.py::test_issue_44 - AssertionError: assert {'c1': 'val'} == {'c1': 'val',...'a1': 'abc'}]}
============================================================================ 1 failed, 142 deselected in 0.28s =============================================================================

Depends on changes below for unit-tests to function

@addyess addyess force-pushed the issue/44/_inconsistent_lazy_values branch from a787ebb to 8f466d0 Compare April 13, 2023 20:29
tests/test_async_client.py Outdated Show resolved Hide resolved
tests/test_client.py Outdated Show resolved Hide resolved
@addyess addyess force-pushed the issue/44/_inconsistent_lazy_values branch from f254cde to c683d92 Compare April 13, 2023 20:40
@addyess addyess changed the title Add tests for issue #44 Address inconsistent _lazy_values after attribute set Apr 13, 2023
Copy link
Owner

@gtsystem gtsystem left a comment

Choose a reason for hiding this comment

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

Hi @addyess, thanks for reporting the issue and prepare a fix for it.
Your change looks good but please take a look at my comment regarding the breaking change of .log() method.

tests/test_client.py Outdated Show resolved Hide resolved
@addyess
Copy link
Contributor Author

addyess commented Apr 14, 2023

Cool, I've backed out the changes to the tests regarding Client.log(...) and moved to a separate PR

@gtsystem
Copy link
Owner

Cool, I've backed out the changes to the tests regarding Client.log(...) and moved to a separate PR

Change is good now. Let's wait for the other fix to get merged first so that we keep master stable.

@gtsystem
Copy link
Owner

Can you rebase?

@addyess
Copy link
Contributor Author

addyess commented Apr 17, 2023

Can you rebase?

Done. thanks for the reviews

@gtsystem
Copy link
Owner

Can you rebase?

Done. thanks for the reviews

I still get the warning "This branch cannot be rebased due to conflicts"

@addyess
Copy link
Contributor Author

addyess commented Apr 17, 2023

I still get the warning "This branch cannot be rebased due to conflicts"

Can you identify the conflicts? I don't see them.

image

@gtsystem
Copy link
Owner

gtsystem commented Apr 17, 2023

I still get the warning "This branch cannot be rebased due to conflicts"

Can you identify the conflicts? I don't see them.

image

I rebased manually in #47. There was a conflict on the logging testing lines. Please take a look. Only difference is that I kept both previous and new tests for logging, so we cover both options.

@addyess
Copy link
Contributor Author

addyess commented Apr 17, 2023

I rebased manually in #47. There was a conflict on the logging testing lines. Please take a look. Only difference is that I kept both previous and new tests for logging, so we cover both options.

closing in favor of #47

@addyess addyess closed this Apr 17, 2023
@gtsystem gtsystem mentioned this pull request Apr 17, 2023
@addyess addyess deleted the issue/44/_inconsistent_lazy_values branch April 17, 2023 19:50
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.

None yet

2 participants