10000 CI Uses marker to control network access in tests by thomasjpfan · Pull Request #17553 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

CI Uses marker to control network access in tests #17553

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 9 commits into from
Closed

CI Uses marker to control network access in tests #17553

wants to merge 9 commits into from

Conversation

thomasjpfan
Copy link
Member

Reference Issues/PRs

Fixes #16750

What does this implement/fix? Explain your changes.

This fixes an issues issue when running nightly builds when running fetch_* with pytest-xdist. The run tests that require network:

pytest sklearn -m 'not skipnetwork'

SKLEARN_RUN_NETWORK_TESTS is only used by our CI to signal when to pass the -m 'not skipnetwork' to pytest.

Note this is kind of extending the use of markers to pass cli options, which is a slight hack.

Any other comments?

This combined with #16928 should fix the errors on the nightly build.

I have tried moving the conftest.py around, but it wasn't too nice. (It would have been nice to get that working tho)

CC: @rth @adrinjalali

@jnothman
Copy link
Member

Do we need to use a double negative? Isn't "network" better than "not skipnetwork"

@thomasjpfan
Copy link
Member Author

I would much prefer -m network over -m 'not skipnetwork', but the pytest -m network will only run tests marked with -m network. There is most likely a some "creative programming" to get this to work.

In the end, I still think I am hacking the marker interface to pass cli args, maybe it's time to use some of the suggestions in pytest-dev/pytest#1596

@adrinjalali
Copy link
Member

does this mean we should always use the _fxt functions in tests instead? we don't do that now.

@thomasjpfan
Copy link
Member Author

we don't do that now.

We are doing it now. The only place we do not do it is test_feature_importance_regression. All the tests on sklearn/datasets uses the fixture.

@adrinjalali
Copy link
Member

yeah that's the one I found and I thought there must be more, but I guess not then.

@adrinjalali adrinjalali mentioned this pull request Jun 16, 2020
Merged
@adrinjalali
Copy link
Member

This is to remind us that this needs to also change checking for network support added in #17599

@thomasjpfan
Copy link
Member Author

Yea I saw it getting merged. I need to rethink this PR a little bit.

@thomasjpfan thomasjpfan mentioned this pull request Jun 30, 2020
Closed
@glemaitre glemaitre mentioned this pull request Jul 8, 2020
Merged
@thomasjpfan thomasjpfan mentioned this pull request Jul 26, 2020
Closed
@thomasjpfan thomasjpfan mentioned this pull request Jan 5, 2021
Closed
@thomasjpfan thomasjpfan mentioned this pull request Jan 5, 2021
Merged
Base automatically changed from master to main January 22, 2021 10:52
@thomasjpfan
Copy link
Member Author

Closing for now, pytest's marker system is not well suited for this. Recently we made improvements with error messages using SKLEARN_SKIP_NETWORK_TESTS.

@thomasjpfan thomasjpfan closed this Apr 9, 2021
@thomasjpfan thomasjpfan mentioned this pull request Mar 10, 2022
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
module:datasets module:ensemble module:test-suite everything related to our tests module:utils
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Document/standardize how to skip network tests
4 participants
@thomasjpfan @jnothman @adrinjalali @cmarmo
0