-
Notifications
You must be signed in to change notification settings - Fork 192
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
Improve OpenStudio's support for Python unitttest #4973
Conversation
In the Python unittest library, unittest.main() was not working as expected, because tests defined in the __main__ scope where not being detected. This meant that `openstudio labs execute_python_script foo_measure_test.rb` did not work as expected. The solution here is to register the __main__ module properly within Python. * An example Measure test is located at NREL/openstudio-common-measures-gem@0c73cdb * A Measure test can be invoked with `openstudio labs --python_path=. execute_python_script tests/test_measure.py` close #4906
I don't want to sound like a broken record, but I would urge we standardize around |
module = importlib.util.module_from_spec(spec) | ||
sys.modules[module_name] = module |
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.
Do you have a link to somewhere where they do this sys.modules["__main__"] = module
or something to convince me this is the right way to do it? Don't want to go chasing info if you already have it.
I have a completely non-justified feeling this might cause trouble in some edge case we haven't foreseen, when this isn't a unittest file or something.
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.
You could be right. Here is my justification though.
https://docs.python.org/3/library/__main__.html#import-main
Python inserts an empty __main__ module in [sys.modules](https://docs.python.org/3/library/sys.html#sys.modules) at interpreter startup, and populates it by running top-level code....
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.
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.
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.
Assuming CI comes clean, this will merge
CI Results for b5d7c00:
|
Just the usual suspect OpenStudioCLI.Labs.test_logger_rb (Failed) on windows. Thanks @kbenne |
In the Python unittest library, unittest.main() was not working as expected, because tests defined in the main scope where not being detected. This meant that
openstudio labs execute_python_script foo_measure_test.rb
did not work as expected.The solution here is to register the main module properly within Python.
openstudio labs --python_path=. execute_python_script tests/test_measure.py
close #4906
Pull request overview
Pull Request Author
src/model/test
)src/energyplus/Test
)src/osversion/VersionTranslator.cpp
)Labels:
IDDChange
APIChange
Pull Request - Ready for CI
so that CI builds your PRReview Checklist
This will not be exhaustively relevant to every PR.