Skip to content

[MRG] TST: skip test requiring internet using --skip-network#12067

Merged
jnothman merged 4 commits intoscikit-learn:masterfrom
glemaitre:is/12013
Sep 16, 2018
Merged

[MRG] TST: skip test requiring internet using --skip-network#12067
jnothman merged 4 commits intoscikit-learn:masterfrom
glemaitre:is/12013

Conversation

@glemaitre
Copy link
Copy Markdown
Member

@glemaitre glemaitre commented Sep 13, 2018

closes #12013

This should skip the test in gradient boosting if we have an issue with internet.
The flag --skip-network should be passed when running the test with pytest.

@glemaitre glemaitre changed the title [WIP] TST: skip test requiring internet if there is none [MRG] TST: skip test requiring internet if there is none Sep 13, 2018
@glemaitre glemaitre changed the title [MRG] TST: skip test requiring internet if there is none [MRG] TST: skip test requiring internet using --skip-network Sep 13, 2018
@glemaitre
Copy link
Copy Markdown
Member Author

@jorisvandenbossche
Copy link
Copy Markdown
Member

@yarikoptic this should allow you to skip the remaining test that requires internet

conftest.py Outdated


def pytest_runtest_setup(item):
if 'network' in item.keywords and item.config.getoption("--skip-network"):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does this run for you locally? Travis CI doesn't seem to be happy although it is the same setup as pandas..

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yep it passes locally. I did not find to reproduce the error yet. I should try the to reproduce the testing environment.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I follow the pytest documentation. It seems that this is working but I did not find the root of the previous issue.

Copy link
Copy Markdown
Member

@rth rth left a comment

Choose a reason for hiding this comment

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

Minor comments below, otherwise LGTM, thanks!

conftest.py Outdated
if item.name == 'sklearn.feature_extraction.hashing.FeatureHasher':
item.add_marker(skip_marker)

# Skip test which required internet if the flag is provided
Copy link
Copy Markdown
Member

@rth rth Sep 14, 2018

Choose a reason for hiding this comment

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

"test" -> "tests"
"required" -> "require"

conftest.py Outdated

# Skip test which required internet if the flag is provided
if config.getoption("--skip-network"):
skip_network = pytest.mark.skip(reason="test required internet")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe,
"test requires network connectivity"

Copy link
Copy Markdown
Member

@jnothman jnothman left a comment

Choose a reason for hiding this comment

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

How do we ensure that this flag is used in appropriate places?

@jnothman
Copy link
Copy Markdown
Member

Never mind that last comment. I suppose pinging @yarikoptic should be good enough?

Copy link
Copy Markdown
Member

@jnothman jnothman left a comment

Choose a reason for hiding this comment

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

I only wonder if there is a somewhat standard nomenclature/interface here, or if we're just doing it as we like.

@glemaitre
Copy link
Copy Markdown
Member Author

This is the way documented in pytestand used in other packages as well (e.g. pandas). The pluginpytest-remotedata, developed by astropy`, should allow to skip as well at the cost of an extra dependency.

@jnothman jnothman merged commit 43c8563 into scikit-learn:master Sep 16, 2018
jnothman pushed a commit to jnothman/scikit-learn that referenced this pull request Sep 17, 2018
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.

0.20rc1: goes online for data during testing - mark the tests?

4 participants