Document/standardize how to skip network tests

[rth]

Following discussion in https://github.com/scikit-learn/scikit-learn/issues/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]

From what you explain, I like option 2.

[rth]

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.

[cmarmo]

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

[rth]

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 https://github.com/scikit-learn/scikit-learn/blob/dcfb3df9a3df5aa2a608248316d537cd6b3643ee/build_tools/azure/test_script.sh#L44 ? (see issue description for a more detailed discussion)

[cmarmo]

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]

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]

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]

@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 https://github.com/scikit-learn/scikit-learn/pull/16652/commits/cc4110434919de0c8bee7b0fcde08faf43d3be36). Is it worth it to revert my PR?

[rth]

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]

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.