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

Mark additional tests requiring network #412

Merged
merged 3 commits into from
May 21, 2024
Merged

Conversation

penguinpee
Copy link
Contributor

@penguinpee penguinpee commented May 17, 2024

Those tests also require network and should be excluded when running
pytest -m 'not network'.

Summary for Squash and Merge

Add network pytest mark to test functions that require network connection to hit third party servers. Improve test_check_availability_invalid_downloader to remove the need to use network connection just to test an error raised after an invalid argument to a custom downloader.

Copy link

welcome bot commented May 17, 2024

💖 Thank you for opening your first pull request in this repository! 💖

A few things to keep in mind:

No matter what, we are really grateful that you put in the effort to do this!

@penguinpee
Copy link
Contributor Author

Background: In Fedora the build environment is not network enabled and downloads are prohibited during build. Yet, we like to run tests whenever possible. This allows us to simply pass -m 'no network' to pytest for running all tests that don't require network.

@penguinpee
Copy link
Contributor Author

I marked a few more tests, which I discovered being unmarked after updating to the latest release.

Copy link
Member

@santisoler santisoler left a comment

Choose a reason for hiding this comment

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

Thanks @penguinpee for opening this PR! Thanks for catching those unmarked test functions.

I left a few comments below, nothing major. Let me know what do you think.

BTW, don't worry about the failing test in macos. Is not your fault. I'll fix that in a separate PR.

pooch/tests/test_core.py Outdated Show resolved Hide resolved
pooch/tests/test_core.py Outdated Show resolved Hide resolved
pooch/tests/test_downloaders.py Outdated Show resolved Hide resolved
pooch/tests/test_downloaders.py Outdated Show resolved Hide resolved
pooch/tests/test_downloaders.py Outdated Show resolved Hide resolved
@penguinpee
Copy link
Contributor Author

In a nutshell, I'll make sure all plugins are present and will test again. I suppose you are right regarding the tests relying on localftpserver and httpserver. I'll update this PR according to my findings. Apologies for being a bit hasty in my conclusions.

Those tests also require network and should be excluded when running
`pytest -m 'not network'`.
@penguinpee
Copy link
Contributor Author

This now works for me with httpserver and localftpserver using `-m 'not network'.

Minor change to improve style of decorated test functions.
Minor changes to improve style of decorated test functions.
Copy link
Member

@santisoler santisoler left a comment

Choose a reason for hiding this comment

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

Thanks for addressing my comments. And no need to apologize, the reason why these decorators shouldn't be added is subtle.

I just pushed two minor commits that move the decorators up in the list, just to improve style, nothing critical. I'm merging this.

@santisoler santisoler changed the title Mark tests requiring network Mark additional tests requiring network May 21, 2024
@santisoler santisoler merged commit 2e47b8d into fatiando:main May 21, 2024
17 of 18 checks passed
Copy link

welcome bot commented May 21, 2024

🎉 Congrats on merging your first pull request and welcome to the team! 🎉

If you would like to be added as a author on the Zenodo archive of the next release, add your full name, affiliation, and ORCID (optional) to the AUTHORS.md file of this repository. Feel free to do this in a new pull request if needed.

We hope that this was a good experience for you. Let us know if there is any way that the contributing process could be improved.

@penguinpee penguinpee mentioned this pull request Jun 8, 2024
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.

2 participants