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

TypedDict completions: tests added #1495

Closed
wants to merge 5 commits into from

Conversation

pappasam
Copy link
Contributor

@pappasam pappasam commented Feb 7, 2020

Continues the conversation in this issue: #1478

Note: many tests are failing at present, but not the ones I've added. Notably, one of the tests for actually completing TypedDict keyword parameters that includes quotes is failing weirdly. This tells me there may be an improperly-handled edge case for key completion somewhere. @davidhalter do you have any ideas about where this could be coming from?

case = <IntegrationTestCase: /home/sroeca/src/Personal/jedi/test/completion/pep0484_typing.py:561 "d['fo']">, actual = {"'foo"}, desired = {"'foo'"}

    def assert_case_equal(case, actual, desired):
        """
        Assert ``actual == desired`` with formatted message.

        This is not needed for typical pytest use case, but as we need
        ``--assert=plain`` (see ../pytest.ini) to workaround some issue
        due to pytest magic, let's format the message by hand.
        """
>       assert actual == desired, """
    Test %r failed.
    actual  = %s
    desired = %s
    """ % (case, actual, desired)
E       AssertionError:
E         Test <IntegrationTestCase: /home/sroeca/src/Personal/jedi/test/completion/pep0484_typing.py:561 "d['fo']"> failed.
E         actual  = {"'foo"}
E         desired = {"'foo'"}
E
E       assert {"'foo"} == {"'foo'"}
E         Extra items in the left set:
E         "'foo"
E         Extra items in the right set:
E         "'foo'"
E         Use -v to get the full diff

Given 87161df, values(from_instance=False) doesn't produce completions
anymore. Therefore, we remove from_instance as an argument.
Adds a function that takes the TypedDict as an argument.

Note: the last two tests are failing, along with lots of other tests
throughout the system.
Copy link
Owner

@davidhalter davidhalter left a comment

Choose a reason for hiding this comment

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

The completion on line 561 is actually right. The test is wrong. You should just change it to 'foo. Note that the ending quote is already there, therefore it's not needed anymore. This might look a bit strange, but it makes sense if you look at other dict completion stuff.

That's obviously my fault. So the code seems to be in a good shape and the tests should all be passing after these small changes. I think the last thing we should do is make an inheritance example.


#? str()
a_string
#? [float()]
Copy link
Owner

Choose a reason for hiding this comment

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

This line is wrong syntax, you should write #? float() instead of #? [float()]. For multiple results you could just write #? float() str(). It's separated by spaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By [float()], I mean a "list of floats". I don't know how many floats will be present in the result. If [float()] is the wrong syntax, how would I express an expected "list of floats"?

Copy link
Owner

Choose a reason for hiding this comment

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

It's not really possible to test this AFAIK with just one test. We could obviously make the test algorithm a bit smarter. However I usually test stuff like this with:

#? list()
a_list_of_strings
#? float()
a_list_of_strings[undefined_index]

BTW: I think you mean a_list_of_floats, so the name is wrong :)

@pappasam
Copy link
Contributor Author

pappasam commented Feb 9, 2020

Regarding changing the test to "'foo" from "'foo'", this sounds simple enough, but I'm curious why the same doesn't need to be done for the test on line 564 with '"bar"'? Sounds like there's either a subtle bug we're letting slide, or that there's a meaningful difference between double and single quotes that I don't fully understand.

@davidhalter
Copy link
Owner

Regarding foo. Currently Jedi doesn't add the quote at the end of a string if the next character is a quote. For 'fo' the next character is '. However for 'bar' it's r.

@pappasam
Copy link
Contributor Author

Sadly, looks like integration tests are failing with inheritance. Here are the relevant test failures; hopefully you're able to see where and why things are going weirdly.

AssertionError:
  Test <IntegrationTestCase: /home/sroeca/src/Personal/jedi/test/completion/pep0484_typing.py:572 '    foo'> failed.
  actual  = {'test.completion.pep0484_typing:def foo'}
  desired = {'builtins:instance str()'}
AssertionError:
  Test <IntegrationTestCase: /home/sroeca/src/Personal/jedi/test/completion/pep0484_typing.py:574 '    an_int'> failed.
  actual  = set()
  desired = {'builtins:instance int()'}

assert set() == {'builtins:instance int()'}
  Extra items in the right set:
  'builtins:instance int()'
  Use -v to get the full diff
AssertionError:
  Test <IntegrationTestCase: /home/sroeca/src/Personal/jedi/test/completion/pep0484_typing.py:583 '    a_string'> failed.
  actual  = set()
  desired = {'builtins:instance str()'}

assert set() == {'builtins:instance str()'}
  Extra items in the right set:
  'builtins:instance str()'
  Use -v to get the full diff
AssertionError:
  Test <IntegrationTestCase: /home/sroeca/src/Personal/jedi/test/completion/pep0484_typing.py:585 '    a_list_of_floats'> failed.
  actual  = set()
  desired = {'builtins:instance list()'}

assert set() == {'builtins:instance list()'}
  Extra items in the right set:
  'builtins:instance list()'
  Use -v to get the full diff
AssertionError:
  Test <IntegrationTestCase: /home/sroeca/src/Personal/jedi/test/completion/pep0484_typing.py:587 '    a_list_of_floats[0]'> failed.
  actual  = set()
  desired = {'builtins:instance float()'}

assert set() == {'builtins:instance float()'}
  Extra items in the right set:
  'builtins:instance float()'
  Use -v to get the full diff
AssertionError:
  Test <IntegrationTestCase: /home/sroeca/src/Personal/jedi/test/completion/pep0484_typing.py:589 '    an_int'> failed.
  actual  = set()
  desired = {'builtins:instance int()'}

assert set() == {'builtins:instance int()'}
  Extra items in the right set:
  'builtins:instance int()'
  Use -v to get the full diff
AssertionError:
  Test <IntegrationTestCase: /home/sroeca/src/Personal/jedi/test/completion/pep0484_typing.py:591 '    another_variable'> failed.
  actual  = set()
  desired = {'builtins:instance int()'}

assert set() == {'builtins:instance int()'}
  Extra items in the right set:
  'builtins:instance int()'
  Use -v to get the full diff
AssertionError:
  Test <IntegrationTestCase: /home/sroeca/src/Personal/jedi/test/completion/pep0484_typing.py:594 '    a_string.isuppe'> failed.
  actual  = set()
  desired = {'isupper'}

assert set() == {'isupper'}
  Extra items in the right set:
  'isupper'
  Use -v to get the full diff
AssertionError:
  Test <IntegrationTestCase: /home/sroeca/src/Personal/jedi/test/completion/pep0484_typing.py:596 '    a_list_of_floats.po'> failed.
  actual  = set()
  desired = {'pop'}

assert set() == {'pop'}
  Extra items in the right set:
  'pop'
  Use -v to get the full diff
AssertionError:
  Test <IntegrationTestCase: /home/sroeca/src/Personal/jedi/test/completion/pep0484_typing.py:598 '    an_int.as_integer_rati'> failed.
  actual  = set()
  desired = {'as_integer_ratio'}

assert set() == {'as_integer_ratio'}
  Extra items in the right set:
  'as_integer_ratio'
  Use -v to get the full diff

#? str()
foo
#? int()
an_int
Copy link
Owner

Choose a reason for hiding this comment

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

foo and an_int should be #?. They don't infer anything, this is not a bug, Python would pretty much say the same thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I see. I've changed the check for an_int to #? and removed the check for foo (it's apparently a function we've defined for other tests, so it has a weird type that's tied to other tests and I think it's prudent to avoid testing for that).

Note: foo is defined as a function a the module level so I remove it
from consideration here to avoid complicating this test with other tests
in the module.
@davidhalter
Copy link
Owner

I'll look into it.

@davidhalter
Copy link
Owner

Merged to master in bb91b96

Fixed the issues in 6097373. The issue was actually that the mro doesn't work anymore if the class is already not a class anymore. I realized that we could just do this when an instance of the TypedDict would be created. For now this works great.

Thanks again for your work on this one!

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.

2 participants