New tests added #82
New tests added #82
Conversation
|
I see that the branch |
|
To be able to checkout locally @ClaudiaLSS branch I did
We only have read acces unless @ClaudiaLSS grant us write access like described in the docs Note: 82 is the number of this PR, for a different PR you should change it in the command |
There was a problem hiding this comment.
Thanks for your efforts @ClaudiaLSS it feels good to have some tests !
I managed to make all test pass (except the tests where maximum_profile is used in the User class).
I made suggestion that you can accept in batch and group into one single commit (from github directly) or in several commits as you wish.
A general comment is to delete the unused import statements (I see them greyed out in pycharm)
I would like also that we start linting the code using black it is a pip package you can install and simply run black tests/ and it lints automatically your files
I would suggest that changes are first made such as to be able to run the tests and that they pass, then we merge and you can improve the tests in a subsequent PR
tests/test_calc_peak_time_range.py
Outdated
|
|
||
| # Tests that the peak time range is calculated correctly for a single user class with the default peak_enlarge value | ||
| def test_single_user_class_default_peak_enlarge(self): | ||
| user_list = [User(maximum_profile=np.array([1, 2, 3, 4, 5]))] |
There was a problem hiding this comment.
User class does not have argument maximum_profile this leads to an error when I run the tests locally. You probably locally changed the User class and forgot to commit and push this change.
tests/test_calc_peak_time_range.py
Outdated
| def test_calc_peak_time_range_with_inputs(self): | ||
| j=1 | ||
| peak_enlarge=0.15 | ||
| user_list=load_usecase(j,peak_enlarge ) |
There was a problem hiding this comment.
| user_list=load_usecase(j,peak_enlarge ) | |
| user_list=load_usecase(j) |
tests/test_peak_time_range.py
Outdated
| from ramp.core.initialise import initialise_inputs | ||
| from ramp.core.stochastic_process import calc_peak_time_range | ||
|
|
||
| def load_usecase(j=None, fname=None): |
There was a problem hiding this comment.
If you reuse the same function several times then define it under tests/utils.py and import it. That way it is easier to maintain the tests on the long term
tests/test_peak_time_range.py
Outdated
| import os | ||
| import pytest | ||
| import numpy as np | ||
| import random | ||
| import math | ||
| from scipy.stats import norm | ||
|
|
||
| from ramp.core.core import User, Appliance | ||
| from ramp.core.core import UseCase | ||
| from ramp.core.initialise import initialise_inputs | ||
| from ramp.core.stochastic_process import calc_peak_time_range |
There was a problem hiding this comment.
| import os | |
| import pytest | |
| import numpy as np | |
| import random | |
| import math | |
| from scipy.stats import norm | |
| from ramp.core.core import User, Appliance | |
| from ramp.core.core import UseCase | |
| from ramp.core.initialise import initialise_inputs | |
| from ramp.core.stochastic_process import calc_peak_time_range | |
| import numpy as np | |
| from ramp.core.initialise import initialise_inputs | |
| from ramp.core.stochastic_process import calc_peak_time_range |
In the suggestion I removed the unused imports
tests/test_calc_peak_time_range.py
Outdated
| def test_single_user_class_default_peak_enlarge(self): | ||
| user_list = [User(maximum_profile=np.array([1, 2, 3, 4, 5]))] | ||
| peak_time_range = calc_peak_time_range(user_list) | ||
| assert isinstance(peak_time_range, np.ndarray) |
There was a problem hiding this comment.
As the return value of calc_peak_time_range is the return of np.arange() function I don't think it makes sense to test for np.ndarray here
tests/test_calc_peak_time_range.py
Outdated
| assert isinstance(peak_time_range, np.ndarray) | ||
| assert peak_time_range.shape[0] > 0 | ||
| assert peak_time_range.dtype == int | ||
| assert np.all(peak_time_range >= 0) |
There was a problem hiding this comment.
Those are the same assert than for all the other tests, shouldn't be the test more specific to the change of peak_enlarge value in this case?
tests/test_peak_time_range.py
Outdated
| @@ -0,0 +1,39 @@ | |||
| #!/usr/bin/env python3 | |||
There was a problem hiding this comment.
How is this test different than test_calc_peak_time_range.py::test_calc_peak_time_range_with_inputs ?
There was a problem hiding this comment.
Thanks a lot for the review! I think the file [test_peak_time_range.py] was not supposed to be there, the real one was calc_peak_time_range.py. The first one needs to be deleted, sorry!
There was a problem hiding this comment.
No problems, this is what reviews are for :)
|
|
||
| #Tests that the method returns a value greater than func_cycle | ||
| @pytest.mark.usefixtures("appliance_instance") | ||
| def test_rand_time_equal_or_greater_than_func_cycle(self, appliance_instance): |
There was a problem hiding this comment.
I think for such random function the test should run on at least 100 iteration of the method and make sure for none of these call is the rand_time < func_cycle
Co-authored-by: Pierre Francois <[email protected]>
- test focused on peak time range values added. - at least 100 iteration of the method added to make sure none of these gives rand_time < func_cycle - new tests for switch on coincidence added
|
The failing doc is being tackled in #85 and is due to deprecated config files, this should not impact this PR :) |
…sult-files-command-cli Hotfix/customize result files output path command cli
|
I have just approved #85; should we rebase the |
Maybe @ClaudiaLSS can simply merge development into her branch and we merge dev into |
|
@ClaudiaLSS - what is your stand on this PR? |
Hi @Bachibouzouk ! I'll try this. |
|
Hi @Bachibouzouk! I think now dev and test branches are identical. I still don't have much experience with this but can you see if everything is ok? |
|
@ClaudiaLSS - you merged |
|
This PR is continued in #93 |
No description provided.