-
Notifications
You must be signed in to change notification settings - Fork 87
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
Clean up fixture names and usages in tests #895
Conversation
Codecov Report
@@ Coverage Diff @@
## master #895 +/- ##
==========================================
+ Coverage 99.79% 99.81% +0.02%
==========================================
Files 197 197
Lines 9205 9168 -37
==========================================
- Hits 9186 9151 -35
+ Misses 19 17 -2
Continue to review full report at Codecov.
|
@@ -23,7 +22,6 @@ def make_mock_import_module(libs_to_exclude): | |||
def _import_module(library): | |||
if library in libs_to_exclude: | |||
raise ImportError("Cannot import {}; excluded by mock muahahaha".format(library)) | |||
return import_module(library) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed because this was never called and was causing codecov to fail!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah gotcha. Cool! As discussed, we have coverage for this via the core dependencies CI job, so its fine by me to remove this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm such a fan of this, it makes things so much cleaner!
@angela97lin please ping me whenever you get the conflicts resolved and I'll review/approve! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good stuff! Left one note on the changelog, otherwise good to go
X, y = datasets.make_classification(n_samples=100, n_features=20, | ||
n_informative=2, n_redundant=2, random_state=0) | ||
|
||
return X, y | ||
|
||
|
||
@pytest.fixture | ||
def X_y_reg(): | ||
def X_y_regression(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice
@@ -23,7 +22,6 @@ def make_mock_import_module(libs_to_exclude): | |||
def _import_module(library): | |||
if library in libs_to_exclude: | |||
raise ImportError("Cannot import {}; excluded by mock muahahaha".format(library)) | |||
return import_module(library) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah gotcha. Cool! As discussed, we have coverage for this via the core dependencies CI job, so its fine by me to remove this.
Closes #891 by removing X_y fixture in conftest.py / test_auto_regression.py, renaming X_y to X_y_binary, renaming X_y_reg to X_y_regression, and cleaning up some instances where we were using X_y for regression problems.
I think it's a good idea to rename X_y to X_y_binary because there were a few instances where we were using X_y for regression problems, and hopefully having a more clear name will help avoid that in the future!