Parametrization in Pytest and --slow mark

ecurtin2 commented 3 years ago

In reference to https://github.com/stanfordmlgroup/ngboost/issues/41

Hey all! Am trying to get familiar with this library, so started by messing about with the unit testing that's currently in place. I saw the opportunity for a couple of changes that I recommend, so here is a PR with the changes implemented.

For the most part, the actual testing has remained unchanged but the layout has been altered to fit with pytest.mark.parametrize for the parameterized tests in distns.

Additionally, some of these tests take quite some time, so I've added in a --slow toggle to be able to only run fast tests during a development loop. Hopefully, this will make more testing a lot less painful!

Here is what the parametrized tests look like

There are still a very large number of warnings that occur during the test suite which aren't addressed in this PR.

I'm planning to add some more testing to the currently untested parts of the code, however, I don't currently understand it well enough to know the appropriate expectations.

Let me know what you think!

ryan-wolbeck commented 3 years ago

thanks @ecurtin2 ! I'll dig through this today or tomorrow

alejandroschuler commented 3 years ago

@ecurtin2 thanks a ton!! At a glance looks fine by me. Let me know if you want to chat 1:1 sometime and I can walk you through the codebase- sent you a request on linkedin. We desperately need some unit tests :)

ecurtin2 commented 3 years ago

That sounds good, I'll reach out to you in linkedin DM.

I think we can at least get some simple tests going on a large portion quickly, but If you haven't settled on the API it might be more prudent to get the design a bit more stable before adding the additional testing code, which is also susceptible to breaking change.

ryan-wolbeck commented 3 years ago

Great work @ecurtin2, would we need to modify an init somewhere to allow for the tests to be run from the root dir? I can run within within the folder but I'm getting

    def pytest_runtest_setup(item):
>       if "slow" in item.keywords and not item.config.getvalue("slow"):
E       ValueError: no option named 'slow'

ngboost/tests/conftest.py:17: ValueError

when I run from root

ecurtin2 commented 3 years ago

@ryan-wolbeck

This should behave as you expect now,

We can change the default test options to whatever feels the most useful. I just added the -v flag for simplicity and demonstration.

ecurtin2 commented 3 years ago

Squashed commits

ryan-wolbeck commented 3 years ago

@ecurtin2 I'm getting the following ERROR - ValueError: option names {'--slow'} already added when running pytest on the root

ecurtin2 commented 3 years ago

I haven't been able to reproduce, but I will give this a further look this weekend.

ryan-wolbeck commented 3 years ago

@ecurtin2 I discovered the issue, it was on my side, originally I had cloned your fork and ran the test instead of checking out the pr from this repo and the error is gone. Not sure what happened with the mismatch but sorry about that

stanfordmlgroup / ngboost

Parametrization in Pytest and --slow mark #184