-
-
Notifications
You must be signed in to change notification settings - Fork 412
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
Fix test keybinding for layer actions #5406
Conversation
Codecov Report
@@ Coverage Diff @@
## main #5406 +/- ##
==========================================
+ Coverage 89.02% 89.09% +0.07%
==========================================
Files 595 595
Lines 50579 50579
==========================================
+ Hits 45026 45062 +36
+ Misses 5553 5517 -36
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
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 like this change. Some plugins will not fail fixed tests.
napari/_tests/test_viewer.py
Outdated
""" | ||
layer_methods = _get_all_keybinding_methods(getattr(layers, cls)) | ||
assert len(layer_methods) == expectation | ||
_ = layer_class( |
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.
Why assignment to _
?
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.
we just need to initialize it, but it's otherwise useless.
_ = ( | ||
make_napari_viewer() | ||
) # instantiate to make sure everything is initialized correctly | ||
_assert_shortcuts_exist_for_each_action(Viewer) | ||
|
||
|
||
@pytest.mark.xfail |
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.
is this xfail required?
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.
Not sure, I left this untouched from before. I think the this boils down to the same issue below where you need to first instantiate the object for actions to be correctly initialized. I think we can ust remove it?
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.
Actually, probably better to leave it, it shows that we have some issue with intitialization so we don't forget to solve it at some point. I'd say solving it is out of scope for this PR, though.
…ons for later tests
2fb8278
to
04e4bd1
Compare
Thanks to the codecov warnings I noticed that it wasn't being run correctly. Now it's fixed! We needed to navigate the full |
Sorry, I didn't have time to review this, but I am so happy those magic method counts are gone now! |
Description
Fixes #5404.
I changed the test so they actually do what they were intended to: ensuring that keybinding existed for each action (rather than having hardcoded numbers like we have now).
To do so I had to programmatically find all relevant actions and shortcuts.
This fixes a few things:
Image.plane
.Type of change
References
How has this been tested?
as there are small differences between the two Qt bindings.
Final checklist:
trans.
to make them localizable.For more information see our translations guide.