Add initial TextBox widget testing #20451
Conversation
There was a problem hiding this comment.
Thank you for opening your first PR into Matplotlib!
If you have not heard from us in a while, please feel free to ping @matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.
You can also join us on gitter for real-time discussion.
For details on testing, writing docs, and our review process, please see the developer guide
We strive to be a welcoming and open project. Please follow our Code of Conduct.
lib/matplotlib/tests/test_widgets.py
Outdated
| assert tool.color == '1.0' | ||
|
|
||
|
|
||
| def test_TextBox(): |
There was a problem hiding this comment.
Is there a reason to nest these like this rather than just call the above test_TextBox?
There was a problem hiding this comment.
Not particularly, I have changed that now. Thanks!
lib/matplotlib/tests/test_widgets.py
Outdated
| tool.begin_typing(tool.text) | ||
| tool.stop_typing() | ||
| assert tool.text == 'x**1' | ||
| assert tool.color == '1.0' |
There was a problem hiding this comment.
Probably should assert this before it changes, as well.
| assert tool.text == 'x**2' | ||
| tool.on_submit(submit) | ||
| tool.on_text_change(change) | ||
| tool.begin_typing(tool.text) |
There was a problem hiding this comment.
It doesn't really make sense to enter text into the box, and not verify that that actually ended up in the box, but instead, replace the text entirely on the submit handler.
If you want to verify that the handlers were called, then set a global, or use a Mock object that counts calls. And to verify that the setters work, just call them directly (and assert a check after.)
There was a problem hiding this comment.
Yes thank you so much, I have modified according to your suggestions.
|
I have updated the code to take into consideration both comments. I eliminated the indentations and used Mock objects to count calls as per the suggestion. |
|
@dmatos2012 The source of the missing coverage is that first time contributors do no trigger the GHA steps without intervention from the maintainers (because people were abusing the system by opening up PRs against projects that would gut their CI and replace it with crypto mining 🤦🏻 ) . Hopefully with the extra runs we will get your coverage up! |
|
@dmatos2012 Are you comfortable rebasing this PR? The fix for those failed tests is already merged to the default branch. |
452cce5 to
db72f7f
Compare
|
Hi @tacaswell I just did it, I think my heart rate is now 190 bpm, but I hope I did it correct. If there is anything else I need to update, change please let me know :). Thanks ! |
|
@dmatos2012 rebasing and force pushing will eventually become less terrifying ;) |
PR Summary
This PR addresses the test coverage of TextBox widget on Issue #20370. This is my first contributing issue, so I grabbed initial approach by the issue opener. I would gladly change whatever is needed.
PR Checklist
pytestpasses).flake8on changed files to check).flake8-docstringsand runflake8 --docstring-convention=all).doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).