Skip to content
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: remove private magicgui access in tests #5331

Merged
merged 5 commits into from
Nov 13, 2022

Conversation

tlambert03
Copy link
Contributor

@tlambert03 tlambert03 commented Nov 12, 2022

Description

This removes a reference to a private module-level variable in magicgui that is going to be moved in pyapp-kit/magicgui#497. It's only used in the tests, and the specific test (resolution of forward references) is well tested on the magicgui side at this point. So simply removing it is fine

@tlambert03 tlambert03 changed the title test: remove private magicgui access in tests tests: remove private magicgui access in tests Nov 12, 2022
@github-actions github-actions bot added the tests Something related to our tests label Nov 12, 2022
Copy link
Contributor

@alisterburt alisterburt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome, thanks for taking the time to clear up here!

@codecov
Copy link

codecov bot commented Nov 12, 2022

Codecov Report

Merging #5331 (2d7475c) into main (cd5c314) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #5331   +/-   ##
=======================================
  Coverage   89.08%   89.09%           
=======================================
  Files         582      582           
  Lines       49311    49310    -1     
=======================================
+ Hits        43929    43932    +3     
+ Misses       5382     5378    -4     
Impacted Files Coverage Δ
napari/_tests/test_magicgui.py 96.42% <100.00%> (-0.02%) ⬇️
napari/utils/theme.py 89.79% <0.00%> (+0.68%) ⬆️
napari/utils/info.py 83.33% <0.00%> (+1.11%) ⬆️
napari/_qt/__init__.py 55.17% <0.00%> (+6.89%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@Czaki
Copy link
Collaborator

Czaki commented Nov 13, 2022

But if I understand correctly, the whole test should be removed based on its description:

object triggers the appropriate imports to resolve the class in time.

@tlambert03
Copy link
Contributor Author

I can do that too if that's what you prefer

@tlambert03
Copy link
Contributor Author

ok removed

This reverts commit 7a15c41.
@tlambert03
Copy link
Contributor Author

I had to revert the removal since unrelated tests (test_expected_number_of_layer_methods ) started failing:

_______________ test_expected_number_of_layer_methods[Image-5] ________________
[305](https://github.com/napari/napari/actions/runs/3455538721/jobs/5767631644#step:8:306)
  
[306](https://github.com/napari/napari/actions/runs/3455538721/jobs/5767631644#step:8:307)
  cls = 'Image', expectation = 5
[307](https://github.com/napari/napari/actions/runs/3455538721/jobs/5767631644#step:8:308)
  
[308](https://github.com/napari/napari/actions/runs/3455538721/jobs/5767631644#step:8:309)
      @pytest.mark.parametrize(
[309](https://github.com/napari/napari/actions/runs/3455538721/jobs/5767631644#step:8:310)
          'cls, expectation', EXPECTED_NUMBER_OF_LAYER_METHODS.items()
[310](https://github.com/napari/napari/actions/runs/3455538721/jobs/5767631644#step:8:311)
      )
[311](https://github.com/napari/napari/actions/runs/3455538721/jobs/5767631644#step:8:312)
      def test_expected_number_of_layer_methods(cls, expectation):
[312](https://github.com/napari/napari/actions/runs/3455538721/jobs/5767631644#step:8:313)
          """
[313](https://github.com/napari/napari/actions/runs/3455538721/jobs/5767631644#step:8:314)
          Make sure we do find all the methods attached to a layer via keybindings
[314](https://github.com/napari/napari/actions/runs/3455538721/jobs/5767631644#step:8:315)
          """
[315](https://github.com/napari/napari/actions/runs/3455538721/jobs/5767631644#step:8:316)
          layer_methods = _get_all_keybinding_methods(getattr(layers, cls))
[316](https://github.com/napari/napari/actions/runs/3455538721/jobs/5767631644#step:8:317)
  >       assert len(layer_methods) == expectation
[317](https://github.com/napari/napari/actions/runs/3455538721/jobs/5767631644#step:8:318)
  E       AssertionError: assert 8 == 5
[318](https://github.com/napari/napari/actions/runs/3455538721/jobs/5767631644#step:8:319)
  E        +  where 8 = len({'activate_image_pan_zoom_mode', 'activate_image_select_mode', 'hold_to_pan_zoom', 'orient_plane_normal_along_view_direction', 'orient_plane_normal_along_x', 'orient_plane_normal_along_y', ...})

I have never quite understood where those seemingly magic expectation numbers come from... it appears that in general/historicall we just change them to make tests pass... which kinda defeats the purpose of the test.

It's out of scope for this PR. Sorry @Czaki.

Copy link
Collaborator

@Czaki Czaki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's out of scope for this PR. Sorry @Czaki.

Ah. There is a test that uses the side effect of another test to pass... Maybe we should have a test turn that runs each test in an independent python interpreter?

In the context of this PR I'm fine, but it would be nice if you could update the docstring of test function to reflect that it is for validate if the expected magicgui widget is returned.

@tlambert03
Copy link
Contributor Author

There is a test that uses the side effect of another test to pass... Maybe we should have a test turn that runs each test in an independent python interpreter?

exactly, and it's just generally been updated to make it pass given whatever the current side effects are 😂 ... that whole test should probably be removed/refactored. it doesn't appear to be a complicated thing to check, but it needs to be redesigned (in another PR)

@tlambert03 tlambert03 merged commit e0b7c88 into napari:main Nov 13, 2022
@tlambert03 tlambert03 deleted the remove-private-magicgui branch November 13, 2022 15:32
@Czaki Czaki mentioned this pull request Jun 7, 2023
@Czaki Czaki added maintenance PR with maintance changes, triaged-0.4.18 To mark PR that is triaged in 0.4.18 release process labels Jun 13, 2023
@Czaki Czaki added this to the 0.4.18 milestone Jun 13, 2023
Czaki pushed a commit that referenced this pull request Jun 16, 2023
* test: remove private magicgui access

* remove full test

* Revert "remove full test"

This reverts commit 7a15c41.

* fix magicgui.widgets namespace

* update docstring
Czaki pushed a commit that referenced this pull request Jun 17, 2023
* test: remove private magicgui access

* remove full test

* Revert "remove full test"

This reverts commit 7a15c41.

* fix magicgui.widgets namespace

* update docstring
Czaki pushed a commit that referenced this pull request Jun 18, 2023
* test: remove private magicgui access

* remove full test

* Revert "remove full test"

This reverts commit 7a15c41.

* fix magicgui.widgets namespace

* update docstring
Czaki pushed a commit that referenced this pull request Jun 19, 2023
* test: remove private magicgui access

* remove full test

* Revert "remove full test"

This reverts commit 7a15c41.

* fix magicgui.widgets namespace

* update docstring
Czaki pushed a commit that referenced this pull request Jun 21, 2023
* test: remove private magicgui access

* remove full test

* Revert "remove full test"

This reverts commit 7a15c41.

* fix magicgui.widgets namespace

* update docstring
Czaki pushed a commit that referenced this pull request Jun 21, 2023
* test: remove private magicgui access

* remove full test

* Revert "remove full test"

This reverts commit 7a15c41.

* fix magicgui.widgets namespace

* update docstring
Czaki pushed a commit that referenced this pull request Jun 21, 2023
* test: remove private magicgui access

* remove full test

* Revert "remove full test"

This reverts commit 7a15c41.

* fix magicgui.widgets namespace

* update docstring
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance PR with maintance changes, tests Something related to our tests triaged-0.4.18 To mark PR that is triaged in 0.4.18 release process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants