-
-
Notifications
You must be signed in to change notification settings - Fork 7.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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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!
139484a
to
9501deb
Compare
@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
def onselect(epress, erelease):
> n_onselect += 1
E UnboundLocalError: local variable 'n_onselect' referenced before assignment
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 a nonlocal
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 for Mock
? Then assert_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.
9501deb
to
b093665
Compare
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_completed
flag for that.PR Checklist
Tests and Styling
pytest
passes).flake8-docstrings
and 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).