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

Fix tests by disble broken one #319

Merged
merged 3 commits into from
Oct 13, 2023
Merged

Fix tests by disble broken one #319

merged 3 commits into from
Oct 13, 2023

Conversation

Czaki
Copy link
Collaborator

@Czaki Czaki commented Oct 11, 2023

In this PR I have disabled the test depending on example-plugin python package as it was hidden by the creator.

Comment on lines +27 to 29
@pytest.mark.skip("package looks deleted from pypi")
def test_fetch_npe1_manifest_with_writer():
mf = fetch_manifest("example-plugin")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@tlambert03 You have created these tests. Did you know who is the author of the example-plugin and why it was removed from pypi?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Ugh sorry @Czaki! Yeah I removed that plugin because I was playing with other things - sorry, didn't realize tests were using it. I can pop it back up,but if we think this test (and others like it) might be critical, we can think about maintaining a dummy plugin within the napari org.

Copy link
Collaborator Author

@Czaki Czaki Oct 12, 2023

Choose a reason for hiding this comment

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

This is the package used for the npe1 test. So I do not think that it will require maintenance.

But we could migrate it if you wish. Did you still have content of this project?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should definitely migrate it to npe2 and keep it up to date in the napari org!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But of course, we could think about a better name if such a need exists.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah agreed, I think it should be test-dummy-plugin or something. And we should hide it from hub. Or even not add the framework classifier?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think that not adding a classifier could be a good idea.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we have different ideas here; I was imagining an actual example-plugin called such, because I can imagine people looking up "napari example plugin" and it would be an easy way in that's not as "scary" as the cookiecutter.

Maybe this just serves a completely different purpose from the testing, in that case maybe this is not the right conversation :P

Copy link
Contributor

Choose a reason for hiding this comment

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

ooops sorry @brisvag I missed this comment. I think maybe these are two separate things? I always viewed the cookiecutter as the default plugin template, but I could imagine maintaining some other example plugin - I'm not sure what you think the benefits of it would be, over the cookiecutter though?

In general, I think that should be separate to a plugin that we use for testing purposes though since they're different targets. But yeah, happy to consider a richer demo-like plugin!

@codecov
Copy link

codecov bot commented Oct 12, 2023

Codecov Report

Merging #319 (89f59d8) into main (bd0d46c) will decrease coverage by 0.25%.
Report is 1 commits behind head on main.
The diff coverage is n/a.

@@             Coverage Diff             @@
##              main     #319      +/-   ##
===========================================
- Coverage   100.00%   99.75%   -0.25%     
===========================================
  Files           37       37              
  Lines         2814     2814              
===========================================
- Hits          2814     2807       -7     
- Misses           0        7       +7     

see 1 file with indirect coverage changes

Copy link
Contributor

@DragaDoncila DragaDoncila left a comment

Choose a reason for hiding this comment

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

@Czaki I'm happy to approve to get at least some green in CI. Once I've got the dummy plugin up (over the weekend most likely), we can come back and update the skipped test.

@Czaki
Copy link
Collaborator Author

Czaki commented Oct 12, 2023

I do not have permission to merge it. Should I change codecov settings to increase the allowed drop?

@nclack
Copy link
Collaborator

nclack commented Oct 13, 2023

Should I change codecov settings to increase the allowed drop?

In the past, for cases like this, we've just forced the merge. I like keeping the codecov settings like they are so that drops are visible.

However, I don't have the permissions to allow the merge. @tlambert03 @DragaDoncila Do you know who does? I'd love @Czaki to get permissions here.

@tlambert03
Copy link
Collaborator

I don't understand, all napari core devs are on the team here with write access... as are you.

Is it an admin access thing?

@Czaki
Copy link
Collaborator Author

Czaki commented Oct 13, 2023

However, I don't have the permissions to allow the merge. @tlambert03 @DragaDoncila Do you know who does? I'd love @Czaki to get permissions here.

Just check. @tlambert03 is the only one who has such permissions, but any person with admin access to napari organization could add the next such persons.

Is it an admin access thing?

yes

@tlambert03
Copy link
Collaborator

@Czaki Just gave you admin

@Czaki Czaki changed the title Fix tests Fix tests by disble broken one Oct 13, 2023
@Czaki Czaki enabled auto-merge (squash) October 13, 2023 16:31
@Czaki Czaki disabled auto-merge October 13, 2023 16:31
@Czaki Czaki merged commit cb5ba4a into napari:main Oct 13, 2023
18 of 19 checks passed
@Czaki Czaki deleted the fix_tests branch October 13, 2023 16:32
Copy link
Collaborator

@nclack nclack left a comment

Choose a reason for hiding this comment

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

Edit: Hah, nvm! I got this in too late :)

Looked into the coverage drop. It is related to fetching plugin info over git or git+https. I added a comment with some more details.

I think I'd prefer it if we add an example or decide to remove the functionality.

tests/test_fetch.py Show resolved Hide resolved
@Czaki
Copy link
Collaborator Author

Czaki commented Oct 13, 2023

@nclack The whole idea is to reenable it within a week but to allow current PR work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants