-
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
Fix #5147 and generally modernize measure templates + get openstudio to run pytest #5149
Conversation
* make it clear that if you use runner.lastEnergyPlusSQLFile you need to reopen it potentially * Use a chdir context
…ated functions to get warnings/errors (stepWarnings, not warnings)
…) + upgrade openstudio measure --examples
It's painfully slow to have to translate it from 1.13.0 to 3.7.0 everytime. Maybe we could consider using 3.2.0 or something like that, but there's no point keeping it so old
…====================== test session starts ============================== platform linux -- Python 3.8.18, pytest-7.4.2, pluggy-1.3.0 -- /home/julien/Virtualenvs/py39/bin/python3 cachedir: .pytest_cache rootdir: /home/julien/Software configfile: setup.cfg collecting ... collected 0 items -- generated xml file: /home/julien/Software/Others/OpenStudio/ruby/junit.xml -- ============================ no tests ran in 0.05s =============================
…un the minitest/pytest on it
…ed, missed this one ``` E:145: 56: [Correctable] Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols. E:162: 56: [Correctable] Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols. E:171: 35: [Correctable] Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols. NoMethodError: undefined method `taint' for "/home/julien/.cache/rubocop_cache":String ```
…ed you, so I forgive you) https://godbolt.org/z/Wo7dPqvs3 ```c++ #include <fmt/compile.h> #include <fmt/format.h> #include <type_traits> #include <variant> using VariantType = std::variant<int, unsigned int>; template <class> inline constexpr bool always_false_v = false; constexpr void test_type(auto&& arg) { using T = std::decay_t<decltype(arg)>; if constexpr (std::is_same_v<T, unsigned int>) { fmt::print(FMT_COMPILE("Hey there unsigned! cppcheck thought you wouldn't be here\n")); } else if constexpr (std::is_same_v<T, int>) { fmt::print(FMT_COMPILE("Hello integer, how are you?\n")); } else { static_assert(always_false_v<T>, "non-exhaustive visitor!"); } } constexpr void test_variant_type(const VariantType& arg) { std::visit( [](auto&& arg) { using T = std::decay_t<decltype(arg)>; if constexpr (std::is_same_v<T, int>) { fmt::print(FMT_COMPILE("Hello integer, how are you?\n")); } else if constexpr (std::is_same_v<T, unsigned int>) { fmt::print(FMT_COMPILE("Hey there unsigned! cppcheck thought you wouldn't be here\n")); } else { static_assert(always_false_v<T>, "non-exhaustive visitor!"); } }, arg); } int main() { int i = 10; test_type(i); unsigned u = 10; test_type(u); fmt::print("Variant\n"); auto iv = VariantType(i); auto uv = VariantType(u); test_variant_type(iv); test_variant_type(uv); } ```
e94ba7e
to
5bf4fe2
Compare
auto canonicalTestDir = openstudio::filesystem::canonical(opt.directoryPath); | ||
|
||
// Pytest | ||
fmt::print(fmt::fg(fmt::color::yellow), | ||
"┌{0:─^{2}}┐\n" | ||
"│{1: ^{2}}│\n" | ||
"└{0:─^{2}}┘", | ||
"", "Starting Python Tests", 80); | ||
fmt::print("\n"); | ||
auto pytestOutDir = canonicalTestDir / "test_results" / "pytest"; | ||
|
||
// -o junit_familar=legacy is to mimic having a pytest.ini with: | ||
// [pytest] | ||
// junit_family=legacy | ||
// This uses xunit1 format, and adds the line + file at which error occured, something we don't have with xunit2 (default) | ||
|
||
auto runPytestCmd = fmt::format( | ||
R"python( | ||
import pytest | ||
|
||
pytest.main([ | ||
"--junit-xml={junit}", | ||
"--cov={test_dir}", | ||
"--cov-report=term-missing", | ||
"--cov-report=json:{json}", | ||
"--cov-report=lcov:{lcov}", | ||
"--cov-report=html:{html}", | ||
"--cov-report=xml:{xml}", | ||
"-o", "junit_family=legacy", | ||
"{test_dir}", | ||
]) | ||
)python", | ||
fmt::arg("junit", (pytestOutDir / "junit.xml").generic_string()), // | ||
fmt::arg("test_dir", canonicalTestDir.generic_string()), // | ||
fmt::arg("json", (pytestOutDir / "python_coverage.json").generic_string()), | ||
fmt::arg("lcov", (pytestOutDir / "python_coverage.lcov").generic_string()), | ||
fmt::arg("html", (pytestOutDir / "python_coverage_html").generic_string()), | ||
fmt::arg("xml", (pytestOutDir / "python_coverage.xml").generic_string())); | ||
|
||
fmt::print("runPytestCmd={}\n", runPytestCmd); | ||
pythonEngine->exec(runPytestCmd); | ||
|
||
fmt::print(fmt::fg(fmt::color::red), | ||
"┌{0:─^{2}}┐\n" | ||
"│{1: ^{2}}│\n" | ||
"└{0:─^{2}}┘", | ||
"", "Starting ruby tests", 80); | ||
fmt::print("\n"); |
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.
openstudio measure --run-tests will now run Pytests with coverage
this won't be on the dashboard yet.
@DavidGoldwasser @kbenne will need to figure out who should trigger pytest, the C++ CLI or the measure tester gem.
Anyways, for now, I think like this we could just tweak measure-tester-gem to consume the pytest xmls if they are present, since the ruby code runs after the python one.
But if you use measure-tester as a gem and use the rake task, then you'd have to manually run pytest.
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 is great. We can try to do work in measure tester gem shortly after 3.8.0 is out to include in 3.8.x or 3.9.0. Many Python measures may also use additional python libraries. In the case when their is extra libraries, I wouldn't expect tests to run with out of the box install without some extra configuration for the additional libraries.
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.
it should work as long as you pass --python_path
options to the CLI.
--python_path DIR Add additional directory to add to front of PYTHONPATH environment variable (may be used more than once)
jinja2 == 3.1.3 | ||
numpy == 1.24.4 | ||
pandas == 2.0.3 | ||
pytest == 7.4.2 | ||
pytest == 8.1.1 | ||
coverage == 7.4.4 | ||
pytest-cov == 5.0.0 |
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.
Bumped pytest and added coverage / pytest-cov.
I bumped jinja2 too.
pandas and numpy are frozen because we use python 3.8
@@ -806,7 +806,7 @@ def self.find(*paths, ignore_error: true) | |||
ps = [path] | |||
while file = ps.shift | |||
catch(:prune) do | |||
yield file.dup.taint | |||
yield file.dup # .taint |
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.
That was leading to a nasty error when running rubocop via measure tester
@@ -59,7 +59,7 @@ static constexpr std::array<std::pair<std::string_view, std::string_view>, 3> ap | |||
{"resources", "resource"}, | |||
}}; | |||
|
|||
static constexpr std::array<std::string_view, 1> ignoredSubFolders{"tests/output"}; | |||
static constexpr std::array<std::string_view, 3> ignoredSubFolders{"tests/output", "__pycache__", "tests/__pycache__"}; |
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.
When updating BCLMeasure / copying it, do not grab __pycache__
At some point we may want to also not grab anything .gitignored
assert_equal(1, result.stepInfo.size) | ||
assert_empty(result.stepWarnings) |
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.
Throughout the tests, I modernized the templates to
- Use non-deprecated methods (warnings is deprecated since 2.x, replaced with stepWarnings)
- Use minitest assertions that provide much better failure messages
# Ensure the model did not start with a space named like requested | ||
refute_includes(model.getSpaces.map(&:nameString), "New Space") |
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.
another example
sqlFile = runner.lastEnergyPlusSqlFile.get | ||
if !sqlFile.connectionOpen | ||
sqlFile.reopen | ||
end | ||
hours = sqlFile.hoursSimulated | ||
refute_empty(hours) | ||
assert_equal(8760.0, hours.get) |
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.
That makes it clear that #5147 shouldn't happen if you do things correctly.
The reporting measure.rb does sql_file.close at the end.
CURRENT_DIR_PATH = Path(__file__).parent.absolute() | ||
sys.path.insert(0, str(CURRENT_DIR_PATH.parent)) | ||
from measure import EnergyPlusMeasureName | ||
sys.path.pop(0) | ||
del sys.modules['measure'] |
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 be able to load several pytest files when --run-tests is happening, and because every measure is measure.py
, we need del sys.modules['measures'] or it remembers the first one it found
# This allows running openstudio CLI on this file (`openstudio test_measure.py`, maybe with extra args) | ||
if __name__ == "__main__": | ||
import sys | ||
if len(sys.argv) > 1: | ||
pytest.main([__file__] + sys.argv[1:]) | ||
else: | ||
pytest.main() |
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 bit is really optional, but I think it's nice to be able to do this, like you could do openstudio measure_test.rb
@pytest.mark.parametrize("measure_language", MEASURE_LANGUAGES) | ||
@pytest.mark.parametrize("measure_type", MEASURE_TYPES) | ||
def test_create_measure_and_run_tests(osclipath: Path, measure_type: str, measure_language: str, verbose: bool = VERBOSE): | ||
measure_dir_path = get_measure_dir(measure_type=measure_type, measure_language=measure_language) | ||
if measure_dir_path.is_dir(): | ||
shutil.rmtree(measure_dir_path) | ||
|
||
cmd = get_cmd(osclipath=osclipath, measure_type=measure_type, measure_language=measure_language) | ||
|
||
if verbose: | ||
print(f"Instantiating new measure {measure_language} - {measure_type}:") | ||
print(f"cmd = {shlex.join(cmd)}") | ||
output = _run_subprocess_and_capture(cmd) | ||
if verbose: | ||
print(output) | ||
|
||
if measure_language == 'Ruby': | ||
run_minitest(osclipath=osclipath, measure_dir_path=measure_dir_path, verbose=verbose) | ||
else: | ||
run_pytest(osclipath=osclipath, measure_dir_path=measure_dir_path, verbose=verbose) |
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 pytest will:
- Instantiate every Measure Type + Measure Language combination
- Calll pytest or minitest on it
This ensures we have WORKING templates.
file(MAKE_DIRECTORY "${PROJECT_BINARY_DIR}/Testing/") | ||
add_test(NAME OpenStudioCLI.test_bcl_measure_templates | ||
COMMAND ${Python_EXECUTABLE} -m pytest --verbose --os-cli-path $<TARGET_FILE:openstudio> "${CMAKE_CURRENT_SOURCE_DIR}/test/test_bcl_measure_templates.py" | ||
WORKING_DIRECTORY "${PROJECT_BINARY_DIR}/Testing/" | ||
) |
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.
register the pytest as a cmakelists one.
Voluntarily not using "-n auto" (pytest-xdist) so I check for loading errors (the thing del sys.modules['measure']
fixes).
I used system pytest (because it wasn't working with the CLI at first) but we could replace it with openstudio, or do it with both.
Mind that the ReportingMeasure tests are quite slow (there's a simulation happening)
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.
And maybe another one for openstudio measure --run_tests Testing/bcl_templates
once all measures have been instantiated too.
We can figure that out later
…Measure 5155 ruby engine fixture.wrong method measure
Find system ruby 3.2
Not sure why copy is even overriden... It's an alias to cp anyways. Read https://www.ruby-lang.org/en/news/2019/12/12/separation-of-positional-and-keyword-arguments-in-ruby-3-0/
Lol windows runner is always so fun
|
CI Results for 6525992:
|
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.