Add type hints to test_message.py #1407
Conversation
|
@SBNetto01 Thanks for looking into this 👍 Did you see the other PR for this? I think there is only one other? Were you wanting feedback at this point, or unsure how to proceed given the failing linting/tests? Do you understand why the linting and tests are failing? I'd be happy to discuss briefly here, or chat in more depth on chat.zulip.org if you need help with this. |
|
Hello @zulip/server-refactoring members, this pull request was labeled with the "area: refactoring" label, so you may want to check it out! |
neiljp
added
the
PR awaiting update
|
Thank you for your reply and sorry about the delay! I was waiting on some feedback, since the failed linting/tests are a bit weird. I wasn't able to use the --fix recommended on a few of the documentations, and all of them are related to "empty spaces" if I'm not mistaken, but it's not related to my modifications whatsoever. Would love to know how to proceed with this. |
There was a problem hiding this comment.
@SBNetto01 I've unfortunately only just got back to this, but here are some details if you're still interested!
I left some details inline - generally you seem to have a good grasp of where the typing goes in the code and what some of the types are, but others appear to not match at all - did you use a tool for this? As I mention inline, if you can understand how pytest uses parametrize better then I suspect you'll be able to determine a lot of the types more fully.
One challenge that I forgot from the previous change where we typed other tests was moving from a simple Dict to a TypedDict (like Message). The main challenge with those is that historically we used an approach of base data that is updated in various different ways in each test case, but mypy doesn't like using certain general dict operations where literal keys are not involved.
So, you may wish to keep some types looser (eg. Dict[str, Any] for what may become Message) in a first pass if that helps you get further, and then we can refine from there.
For reference, the general solution for the latter case generally involves updating the tests:
a) to refer to specific literal strings as keys into the dict (where possible), or
b) using factory functions/fixtures where the desired values are passed in (constructed as required, rather than modified later)
If you're on chat.zulip.org, do start a topic in #zulip-terminal if you're interested :)
| @@ -25,7 +29,7 @@ | |||
|
|
|||
| class TestMessageBox: | |||
| @pytest.fixture(autouse=True) | |||
| def mock_external_classes(self, mocker, initial_index): | |||
| def mock_external_classes(self, mocker: MockerFixture, initial_index: int) -> None: | |||
There was a problem hiding this comment.
Here initial_index is a fixture, not an int. Please see #1337 for some details on fixtures and where they're often defined. If it's not obvious where a parameter comes from, it's often a fixture!
| def test_init( | ||
| self, | ||
| mocker: MockerFixture, | ||
| message_type: str, |
There was a problem hiding this comment.
For message_type, we likely want the new MessageType in api_types.py; that's also a string, but a more constrained type.
| def test_soup2markup(self, content, expected_markup, mocker): | ||
| def test_soup2markup( | ||
| self, content: str, | ||
| expected_markup: str, |
There was a problem hiding this comment.
Please ensure you understand how parametrize works, so that you can determine a good type for expected_markup.
| message: Dict[str, Any], | ||
| last_message: Optional[Dict[str, Any]] |
There was a problem hiding this comment.
These dicts, and those in other changes, should be replaced by Message, again in api_types.py.
| self, | ||
| mocker: MockerFixture, | ||
| message: Dict[str, Any], | ||
| to_vary_in_last_message: bool |
There was a problem hiding this comment.
These are another case of you needing to understand parametrize first.
| ): | ||
| mocker: MockerFixture, | ||
| messages_successful_response: Dict[str, Any], | ||
| msg_type: MessageType, |
There was a problem hiding this comment.
Ah, I see you used MessageType here - but note that the variable type is different here, which I can tell from reading the parametrize (as per other comments).
| mocker: MockerFixture, | ||
| messages_successful_response: Dict[str, Any], | ||
| msg_type: MessageType, | ||
| msg_narrow: list, |
There was a problem hiding this comment.
I think mypy will complain if you use a list with no typing, even if that typing is Any (which we may need here) - and with our current setup we likely need List too.
| message_links: List[str], | ||
| expected_text: Optional[str], | ||
| expected_attrib: Optional[Dict[str, Any]], | ||
| expected_footlinks_width: Optional[int] |
There was a problem hiding this comment.
These need you to understand parametrize.
What does this PR do, and why?
This PR adds the adequate type hints in each function from test_message.py, solving issue #1337
External discussion & connections
topicHow did you test this?
Self-review checklist for each commit