[MRG+1] MAINT: Make sure all tests are included with installation#6274
[MRG+1] MAINT: Make sure all tests are included with installation#6274GaelVaroquaux merged 4 commits intoscikit-learn:masterfrom
Conversation
|
As far as I can tell, the AppVeyor test failures are tests that weren't previously being run because of a missing Edit: the CircleCI failure is due to a matplotlib version issue, I think. |
|
oh boy... |
| from sklearn.datasets import make_multilabel_classification | ||
|
|
||
| from test_split import MockClassifier | ||
| from .test_split import MockClassifier |
There was a problem hiding this comment.
According to http://scikit-learn.org/stable/developers/contributing.html#coding-guidelines, tests should use absolute imports:
from sklearn.model_selection.tests.test_split import MockClassifierI guess it wasn't done this way originally because of the missing __init__.py ...
|
So what's the way forward here? Should I try to fix the test failures as part of this PR before we merge? The failing code is already in master, it's just not being reflected in the CI tests at the moment. I'm going to mark as [MRG] for now, despite the test failures. |
The problem if we merge is that the failures notification will hide new failures in PR. So I would rather like to fix the failures before merging. Let's make a list of them as new issues linked to this PR. |
|
If someone with a windows dev environment could have a look at those 2 failures that would be great. |
|
BTW, I restored the title layout of the broken example in circle ci: 637cebb. |
|
OK, I rebased to master so hopefully the CircleCI test will pass now. |
|
hm... appveyor is also broken on master now |
|
So |
|
This needs to be rebased on master now that #6333 has been merged. |
faf992b to
d66d71a
Compare
|
Rebased on master. |
d66d71a to
1eb6b84
Compare
|
Just a ping & reminder here: a not-insignificant number of scikit-learn's unit tests are not being run by travis. The failures in this PR are due to failing code that is already part of master. This PR just causes the tests to be run. I'd advocate merging this to bring those failures to everyone's attention, rather than letting it get buried for another few months while those tests continue to be skipped. |
|
The test_scale_and_stability failure has been fixed in #6661. Sorry I completely forgot to follow up here. If you rebase on master AppVeyor should go green and this PR can be merged without any hesitation. |
|
Seems to be working now! To prevent this in the future, we might consider having the setup process automatically crawl the source tree. For example, astropy does this: astropy_helpers/setup_helpers.py |
|
thx @jakevdp +1 for merge |
|
This one should really be merged since as @jakevdp was saying
Pinging a random subset of people with the necessary rights @rvraghav93 @TomDLT @jnothman @GaelVaroquaux. |
|
Merged. Thanks! |
|
Great, thanks! |
As I mentioned in #6273, the build scripts leave out several of the test modules, so when you run
nosetests sklearnoutside the source tree not all the tests are run.I looked into this and found ~6 submodules that had this issue. In the process of fixing all of them, I found it easier to re-organize and alphabetize portions of the setup file.
I also chose to have all submodules with a setup script import their own sub-submodules. Previously, there was a mix of doing the additions locally and doing the additions at the top level.