-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Tests: Add test for InputText in UserInputs #2870
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
Tests: Add test for InputText in UserInputs #2870
Conversation
|
Performance benchmarks:
|
|
Hi mentors, I've fixed the pre-commit issues and all checks are passing now. Ready for review when you have time! |
|
@pragam-m25 PR is appreciated but we already have tests for checkbox here. I didn't know that we are missing the test for Also what gives you the idea that hinglish(hindi written in english) is an acceptable language for the comments? Also the function name is very convoluted, it can be something simple like |
|
Hi, thanks for opening a PR! Could you motivatie why you added these two specific tests? Also you deleted our PR template, we have that for a reason, to answer these kinds of questions. Please follow it. :) |
|
Hi @EwoutH
Thanks! |
|
Hi @Sahil-Chhoker, Thanks so much for the detailed review! This is really helpful.
I've just pushed the new changes. It looks like the main build checks are failing now, and I'm currently debugging that. I'll let you know when it's all green. |
|
Hi @Sahil-Chhoker, just to update: I've found and fixed the bug (it was a test placement error, as you pointed out). All checks are passing now. Ready for review when you have time! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work, @pragam-m25! Just remove the comments, the code is self-explanatory. After that, this PR is good to go.
Also can you update the title and description of this PR as we are just adding tests for InputText.
61ec1d2 to
5daddfe
Compare
|
Hi @Sahil-Chhoker, thanks! I've:
All checks should be passing now. Ready for merge! |
|
Sahil thanks for reviewing, and @pragam-m25 congratulations on getting your first PR merged! If you're serious about GSoC, say hi to us in our chat (https://app.element.io/#/room/#project-mesa:matrix.org) and read this part of our contribution guide carefully: https://github.com/projectmesa/mesa/blob/main/CONTRIBUTING.md#im-a-developer-but-not-a-modeller We're an ABM library, so the first part is getting up to speed with ABM! The best way to do that is to start building models. |
|
Hi @EwoutH, Thank you so much! I'm really excited to get my first PR merged. That's great advice. I'll join the chat right away and start reading the contributing guide you linked. I'm looking forward to learning more about ABM and building some models. Thanks again for all the guidance! |
* Tests: Add tests for Checkbox and InputText in UserInputs * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Fix: Remove typo in comment for codespell * Refactor: Move InputText test to test_solara_viz_updated.py as per review * Fix: Correctly place test method inside TestMakeUserInput class --------- Co-authored-by: pragam-m25 <pragam279@gmail.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Description
This PR adds a new test to
test_solara_viz_updated.pyfor theUserInputscomponent for theInputTexttype.This improves the test coverage for this component.
Related Issue
Addresses part of #1781
Checklist