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

Add python3.9 to GHA #5730

Merged
merged 11 commits into from
Oct 13, 2023
Merged

Add python3.9 to GHA #5730

merged 11 commits into from
Oct 13, 2023

Conversation

amanda11
Copy link
Contributor

Investigate if any changes needed for unit or integration tests to work with python 3.9.

@pull-request-size pull-request-size bot added the size/M PR that changes 30-99 lines. Good size to review. label Sep 16, 2022
@amanda11 amanda11 marked this pull request as draft September 16, 2022 14:08
@amanda11
Copy link
Contributor Author

amanda11 commented Sep 16, 2022

The lint fails seem to do with astroid on python3.9 only, we are fixed to 2.5.6. Lots of fixes on later versions of astroid that mention python 3.9, but with newer versions of astroid our tests fail on both python3.8 and python 3.9. So not as simple as just upgrading astroid.

Test failure:

pylint_plugins/api_models_test.py ...FFF...                                                                      [100%]

======================================================= FAILURES =======================================================
__________________________________________________ test_copied_schema __________________________________________________

    def test_copied_schema():
        code = """
        import copy
    
        class ActionAPI(object):
            schema = {"properties": {"action": {}}}
    
        class ActionCreateAPI(object):
            schema = copy.deepcopy(ActionAPI.schema)
            schema["properties"]["default_files"] = {}
        """
    
        res = parse(code)
    
        assert isinstance(res, nodes.Module)
    
        class1_node: nodes.ClassDef = res.body[1]
        assert isinstance(class1_node, nodes.ClassDef)
    
        assert "schema" in class1_node.locals
    
        # action property added but not property from the other class
        assert "action" in class1_node.locals
        assert "default_files" not in class1_node.locals
    
        class2_node: nodes.ClassDef = res.body[2]
        assert isinstance(class2_node, nodes.ClassDef)
    
        # action (copied) and default_files (added) properties added
        assert "schema" in class2_node.locals
        assert "action" in class2_node.locals
>       assert "default_files" in class2_node.locals
E       AssertionError: assert 'default_files' in {'__module__': [<Const.str l.7 at 0x7f4ac62056a0>], '__qualname__': [<Const.str l.7 at 0x7f4ac62050a0>], 'action': [<AssignName.action l.7 at 0x7f4ac61ee220>], 'schema': [<AssignName.schema l.8 at 0x7f4ac6205490>]}
E        +  where {'__module__': [<Const.str l.7 at 0x7f4ac62056a0>], '__qualname__': [<Const.str l.7 at 0x7f4ac62050a0>], 'action': [<AssignName.action l.7 at 0x7f4ac61ee220>], 'schema': [<AssignName.schema l.8 at 0x7f4ac6205490>]} = <ClassDef.ActionCreateAPI l.7 at 0x7f4ac6205100>.locals

However if I use latest astroid/pylint combinations on python3.8 then the astroid parse on the pylint_plugins copy fails to find the property added after copy. (astroid 2.12.9). So with latest, test fails on both python3.8 and python3.9.

Rolled back down to 2.6.6 astroid and still failed (python 3.8 as well).

Can reproduce problem locally just with code:

from collections.abc import Collection
import astroid
from astroid import parse, nodes
import pylint.checkers.typecheck
import api_models
code="""
 
class ActionAPI(object):
     schema = {"properties": {"action": {}}}

class ActionCreateAPI(object):
    schema = copy.deepcopy(ActionAPI.schema)
    schema["properties"]["default_files"] = {}
"""
res=parse(code)
class2_node: nodes.ClassDef = res.body[2]

The test expects res.body[2].locals to contain both action and default_files properties. But the problem is that only action is present, not default_files property.

If I rolled python 3.8 back down to astroid 2.5.6, then the test passes on python3.8 but fails on python3.9. No joy either with astroid 2.5.8, 2.6.1 on python3.9.

Tried on python 3.9 astroid           2.8.6, pylint            2.11.1. With that combination still get the missing default_files property, but also instead of the res.body having 3 entries, it ends up only having 2.

@pull-request-size pull-request-size bot added size/XXL PR that changes 1000+ lines. You should absolutely split your PR into several. and removed size/M PR that changes 30-99 lines. Good size to review. labels Mar 27, 2023
@nzlosh
Copy link
Contributor

nzlosh commented Sep 8, 2023

I hit the similar issues with py3.10 and astroid 2.15.5.

============================================================================== short test summary info ==============================================================================
FAILED pylint_plugins/api_models_test.py::test_copied_schema - AssertionError: assert 'default_files' in {'__module__': [<Const.str l.7 at 0x7f118ae0fb20>], '__qualname__': [<Const.str l.7 at 0x7f118ae0f4c0>], 'action': [<AssignName.action...
FAILED pylint_plugins/api_models_test.py::test_copied_imported_schema - AssertionError: assert 'default_files' in {'__module__': [<Const.str l.5 at 0x7f118ae5feb0>], '__qualname__': [<Const.str l.5 at 0x7f118ae4ffa0>], 'description': [<AssignName.d...
FAILED pylint_plugins/api_models_test.py::test_indirect_copied_schema - AttributeError: 'str' object has no attribute 'value'
FAILED pylint_plugins/api_models_test.py::TestTypeChecker::test_finds_no_member_on_api_model_when_property_not_in_schema - AttributeError: module 'pylint.testutils' has no attribute 'Message'
============================================================================ 4 failed, 5 passed in 0.51s ============================================================================

@pull-request-size pull-request-size bot added size/M PR that changes 30-99 lines. Good size to review. and removed size/XXL PR that changes 1000+ lines. You should absolutely split your PR into several. labels Oct 11, 2023
python3.9 removed the Slice type from the AST, so the property_name_node is not as deep as expected.
@cognifloyd
Copy link
Member

cognifloyd commented Oct 11, 2023

Bingo: py3.9 changed the AST (python/cpython@13d52c2), dropping an intermediate Slice type from something like schema["properties"], so I just pushed a commit that fixes that.

Oddly, the pants-based run did not fail with 3.9 even though it should have. I suspect that pants might be using the wrong python for our tests. I will figure out + address that when I update pants in another PR.

python3.9 removed the Slice type from the AST, so the property_name_node is not as deep as expected.
@cognifloyd cognifloyd added this to the 3.9.0 milestone Oct 11, 2023
@nzlosh
Copy link
Contributor

nzlosh commented Oct 12, 2023

@cognifloyd Great find 💯

@amanda11 amanda11 changed the title [WIP] Add python3.9 to GHA Add python3.9 to GHA Oct 12, 2023
@amanda11 amanda11 marked this pull request as ready for review October 12, 2023 15:16
@amanda11 amanda11 requested review from a team and arm4b October 12, 2023 15:16
@cognifloyd cognifloyd requested a review from a team October 12, 2023 15:36
Copy link
Member

@arm4b arm4b left a comment

Choose a reason for hiding this comment

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

Thanks a lot everyone!
let’s merge it

@arm4b arm4b merged commit 5dc2e9f into master Oct 13, 2023
34 checks passed
@arm4b arm4b deleted the py3_9 branch October 13, 2023 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/M PR that changes 30-99 lines. Good size to review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants