-
Notifications
You must be signed in to change notification settings - Fork 8
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
Refactors trnsys tests #79
Refactors trnsys tests #79
Conversation
Addresses issue #68 |
Makes sure load_idf_file_and_clean_names() returns unique names and an IDF object
Makes sure csv file with schedules exists and that schedules are written in lines
Makes sure version and building information written in lines
Makes sure materials (material, AirGap, NoMass, etc.) are written in lines
Makes sure relative coords converted to absolute ones
Makes sure path to T3D file exists
Value of conditioning in REGIME section has to be the name of the conditioning system previously written in Conditioning section (must be a string, not a list or half-string etc.)
Makes sure infiltration, internal gains and conditioning are written in b18_lines
WARNING : this test does not represent a real IDF. It was modified to have the best coverage as possible
Improve coverage of trnsys.y
Improve coverage of trnsys.py
Improve coverage
tests/test_trnsys.py
Outdated
|
||
idf = add_object_and_run_ep(ep_version, idf_file, weather_file, outputs) | ||
|
||
assert type(idf) == ar.idfclass.IDF |
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.
use assert isinstance(idf, ar.idfclass.IDF)
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.
Done
archetypal/trnsys.py
Outdated
@@ -425,6 +414,23 @@ def convert_idf_to_trnbuild( | |||
return return_path | |||
|
|||
|
|||
def add_object_and_run_ep(ep_version, idf_file, weather_file, outputs): |
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.
Why did you wrap the run_eplus
method in this new add_object_and_run_ep()
?
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.
To have better coverage for trnsys.py
…running Improve coverage of trnsys.py
Ok but if an assertion does not pass, you would like the test to fail, right?
Samuel
… On Apr 18, 2020, at 1:36 PM, louisleroy5 ***@***.***> wrote:
@louisleroy5 commented on this pull request.
In tests/test_trnsys.py:
> + kwargs,
+ ) = converttesteasy
+ try:
+ (
+ idf_file,
+ weather_file,
+ window_lib,
+ output_folder,
+ trnsidf_exe,
+ template,
+ ) = _assert_files(
+ idf_file, weather_file, window_lib, output_folder, trnsidf_exe, template
+ )
+ except:
+ output_folder = os.path.relpath(settings.data_folder)
+ print("Could not assert all paths exist - OK for this test")
If the assertion do not pass
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
Oh ok! But it fails if the assertion does not pass, doesn’t it ?
… On Apr 18, 2020, at 2:36 PM, Samuel Letellier-Duchesne ***@***.***> wrote:
Ok but if an assertion does not pass, you would like the test to fail, right?
Samuel
> On Apr 18, 2020, at 1:36 PM, louisleroy5 ***@***.***> wrote:
>
>
> @louisleroy5 commented on this pull request.
>
> In tests/test_trnsys.py:
>
> > + kwargs,
> + ) = converttesteasy
> + try:
> + (
> + idf_file,
> + weather_file,
> + window_lib,
> + output_folder,
> + trnsidf_exe,
> + template,
> + ) = _assert_files(
> + idf_file, weather_file, window_lib, output_folder, trnsidf_exe, template
> + )
> + except:
> + output_folder = os.path.relpath(settings.data_folder)
> + print("Could not assert all paths exist - OK for this test")
> If the assertion do not pass
>
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub, or unsubscribe.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#79 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AIS7ISQ6VWWA5NT4K2Y2JC3RNHXMLANCNFSM4MKI6TTQ>.
|
Yes. This is the role of assertions. “If assertion fails, test fails”
Samuel
… On Apr 18, 2020, at 2:44 PM, louisleroy5 ***@***.***> wrote:
Oh ok! But it fails if the assertion does not pass, doesn’t it ?
> On Apr 18, 2020, at 2:36 PM, Samuel Letellier-Duchesne ***@***.***> wrote:
>
>
> Ok but if an assertion does not pass, you would like the test to fail, right?
>
> Samuel
>
> > On Apr 18, 2020, at 1:36 PM, louisleroy5 ***@***.***> wrote:
> >
> >
> > @louisleroy5 commented on this pull request.
> >
> > In tests/test_trnsys.py:
> >
> > > + kwargs,
> > + ) = converttesteasy
> > + try:
> > + (
> > + idf_file,
> > + weather_file,
> > + window_lib,
> > + output_folder,
> > + trnsidf_exe,
> > + template,
> > + ) = _assert_files(
> > + idf_file, weather_file, window_lib, output_folder, trnsidf_exe, template
> > + )
> > + except:
> > + output_folder = os.path.relpath(settings.data_folder)
> > + print("Could not assert all paths exist - OK for this test")
> > If the assertion do not pass
> >
> > —
> > You are receiving this because you commented.
> > Reply to this email directly, view it on GitHub, or unsubscribe.
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub <#79 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AIS7ISQ6VWWA5NT4K2Y2JC3RNHXMLANCNFSM4MKI6TTQ>.
>
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
No need to wrap a function in an other one if it does not bring any new functionality
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.
@louisleroy5 , this is what I mean. There is no additional functionality to wrap a function inside another one. In other words, calling add_object_and_run_ep
is exactly the same as calling run_eplus
with the same arguments
Yes and there is assertions for this test :
line 455 of test_trnsys.py :
# Asserts closest coords
assert x == -5
assert y == 215
assert z == 0
… On Apr 18, 2020, at 3:44 PM, Samuel Letellier-Duchesne ***@***.***> wrote:
Yes. This is the role of assertions. “If assertion fails, test fails”
Samuel
> On Apr 18, 2020, at 2:44 PM, louisleroy5 ***@***.***> wrote:
>
>
> Oh ok! But it fails if the assertion does not pass, doesn’t it ?
>
> > On Apr 18, 2020, at 2:36 PM, Samuel Letellier-Duchesne ***@***.***> wrote:
> >
> >
> > Ok but if an assertion does not pass, you would like the test to fail, right?
> >
> > Samuel
> >
> > > On Apr 18, 2020, at 1:36 PM, louisleroy5 ***@***.***> wrote:
> > >
> > >
> > > @louisleroy5 commented on this pull request.
> > >
> > > In tests/test_trnsys.py:
> > >
> > > > + kwargs,
> > > + ) = converttesteasy
> > > + try:
> > > + (
> > > + idf_file,
> > > + weather_file,
> > > + window_lib,
> > > + output_folder,
> > > + trnsidf_exe,
> > > + template,
> > > + ) = _assert_files(
> > > + idf_file, weather_file, window_lib, output_folder, trnsidf_exe, template
> > > + )
> > > + except:
> > > + output_folder = os.path.relpath(settings.data_folder)
> > > + print("Could not assert all paths exist - OK for this test")
> > > If the assertion do not pass
> > >
> > > —
> > > You are receiving this because you commented.
> > > Reply to this email directly, view it on GitHub, or unsubscribe.
> > —
> > You are receiving this because you were mentioned.
> > Reply to this email directly, view it on GitHub <#79 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AIS7ISQ6VWWA5NT4K2Y2JC3RNHXMLANCNFSM4MKI6TTQ>.
> >
>
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub, or unsubscribe.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#79 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AIS7ISXTYX7OK4MPS6M3LOLRNH7IJANCNFSM4MKI6TTQ>.
|
tests/test_trnsys.py
Outdated
) = _assert_files( | ||
idf_file, weather_file, window_lib, output_folder, trnsidf_exe, template | ||
) | ||
except: |
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.
This except statement is too general. try, except what? KeyError? ValueError?
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.
Even better, put the _assert_files
under a with pytest.raises(Exception):
and get rid of the try, except clause
Make sure to replace "Exception" by the actual exception your method raises. This way, pytest will be happy and display the correct information
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.
Tests all possibilities in _assert_files() (trnsys.py)
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.
A lot cleaner!
Actually, the Windows build is failing
Refactors tests to improve coverage and for tests to run faster