Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Add some tests for minspan{x,y} in RectangleSelector#22154
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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.
This is nice, I have left one suggestion for improvement!
Uh oh!
There was an error while loading.Please reload this page.
| @pytest.mark.parametrize('minspany, y1', [[0, 10], [1, 10.5], [1, 11]]) | ||
| def test_rectangle_minspan(spancoords, minspanx, x1, minspany, y1): | ||
| ax = get_ax() | ||
| ax._n_onselect = 0 |
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.
AFAICS this dynamically adds an attribute. If so, this needs explanation:
| ax._n_onselect=0 | |
| # add an attribute to track the number of onselect calls | |
| ax._n_onselect=0 |
OTOH you are only working with a single axis. Couldn't you just use a local variable?
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.
I tried a local variable and it gives
defonselect(epress,erelease):>n_onselect+=1EUnboundLocalError:localvariable'n_onselect'referencedbeforeassignment
I've added a comment though
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.
That would probably need anonlocal statement. But I can live with a comment as well.
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.
Seems like a use case forMock? Thenassert_not_called,assert_called_once_with, etc.
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.
I'm happy to look into mocking, but would advocate for this PR to go in first so#21945 can be tested with these tests, and because the current code is how other widget tests do similar checks.
PR Summary
There aren't currently any tests for the
minspan{x,y}arguments, so add some. There isn't any public API for checking if a given selector is currently 'active', so I had to use the privatetool._selection_completedflag for that.PR Checklist
Tests and Styling
pytestpasses).flake8-docstringsand runflake8 --docstring-convention=all).Documentation
doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).