8000 Document/standardize how to skip network tests · Issue #16750 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

Document/standardize how to skip network tests #16750

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
Open
rth opened this issue Mar 23, 2020 · 10 comments
Open

Document/standardize how to skip network tests #16750

rth opened this issue Mar 23, 2020 · 10 comments
Assignees
@thomasjpfan
Labels
Documentation module:test-suite everything related to our tests

Comments

@rth
Copy link
Member
rth commented Mar 23, 2020

Following discussion in #16743 I think it would be good to document and standardize how we skip tests that require network or download (this could also apply to tests that are skipped for other reason). Currently there are these mechanisms,

  1. SKLEARN_SKIP_NETWORK_TESTS for some tests
  2. network pytest marker for other tests, meaning that such tests can be deselected with -m "not network" pytest option
  3. also the --skip-network pytest option, that skips tests with the network marker. The issue with this is that it requires a conftests.py, which is currently not available e.g. when scikit-learn is installed and tests are run with pytest --pyargs sklearn.

I would propose to remove 1, and standardize/document the use of either 2 or 3. If we go with 3 we might need to move conftests.py under the sklearn folder, although it's also not ideal.
@adrinjalali
Copy link
Member

From what you explain, I like option 2.

@rth
Copy link
Member Author
rth commented Mar 23, 2020

From what you explain, I like option 2.

Yes, me too. The only limitation of option 2 is deselects the tests instead of skipping it (i.e. by default it will just not show it in the log). And it doesn't seem like it's possible to show deselected tests with the -r option. Though maybe it's not too important.
8000

@cmarmo
Copy link
Contributor
cmarmo commented Mar 23, 2020

Having #16652 merged, now the --skip-network pytest option works as is (at least on linux).

@rth
Copy link
Member Author
rth commented Mar 23, 2020

Having #16652 merged, now the --skip-network pytest option works as is (at least on linux).

It works, but not if conftest.py is not present, as currently happens in CI I think

scikit-learn/build_tools/azure/test_script.sh

Line 44 in dcfb3df

$TEST_CMD --pyargs sklearn
? (see issue description for a more detailed discussion)

@cmarmo
Copy link
Contributor
cmarmo commented Mar 23, 2020

It works, but not if conftest.py is not present, as currently happens in CI I think

euh... sorry not sure to understand..
I ran locally the command line

python -m pytest --showlocals --durations=20 -Werror::DeprecationWarning -Werror::FutureWarning --skip-network sklearn/ensemble/tests/test_gradient_boosting.py

almost a copy of the CI (except the coverage thing and the number of test) and the network dependent test is skipped. It wasn't working before because the pytest marker was undeclared.
No need to add conftest.py in the command line... but maybe I'm missing the point...

@rth
Copy link
Member Author
rth commented Mar 23, 2020

The workflow I mean is more,

cd scikit-learn/
pip install .
cd /tmp/
pytest --pyargs sklearn

The important part is the --pyargs which tells pytest to import sklearn, find the folder where it's installed and run pytest there. It would typically be some python folder under site-packages/ and won't include conftest.py (or any other config files for that matter). We do this e.g. in Azure CI and on on conda-forge to ensure that tests pass with the installed version, not just the dev version.

Again it's not really a big issue, but just something to keep in mind.

@cmarmo
Copy link
Contributor
cmarmo commented Mar 23, 2020

The important part is the --pyargs which tells pytest to import sklearn, find the folder where it's installed and run pytest there.

Understood, thanks for the clarification.

@cmarmo
Copy link
Contributor
cmarmo commented Mar 27, 2020

@rth, sorry, me again...
After your explanation I realized that setup.cfg is copied in the tmp directory where the code is built either on azure and Travis.
To have the --skip-network available in the CI build, custom markers should be defined in setup.cfg (this works too, see cc41104).
Is it worth it to revert my PR?

@rth
Copy link
Member Author
rth commented Mar 27, 2020

After your explanation I realized that setup.cfg is copied in the tmp directory where the code is built either on azure and Travis.

We could put it in setup.cfg indeed, up to you. But we have important things in conftest.py anyway (like skipping tests that don't work under some conditions), so that wouldn't really address the general issue.

@cmarmo
Copy link
Contributor
cmarmo commented Mar 27, 2020

But we have important things in conftest.py anyway (like skipping tests that don't work under some conditions), so that wouldn't really address the general issue.

I'm leaving it as is, then. Thanks for your answer.

@thomasjpfan thomasjpfan self-assigned this Jun 9, 2020
@thomasjpfan thomasjpfan mentioned this issue Jun 10, 2020
Closed
@cmarmo cmarmo added the module:test-suite everything related to our tests label Sep 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@thomasjpfan thomasjpfan

Labels
Documentation module:test-suite everything related to our tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

CI Uses marker to control network access in tests thomasjpfan/scikit-learn
4 participants
@rth @cmarmo @adrinjalali @thomasjpfan
0