From 05e59963ec0a88c556a09c19f90f7c44ab20d663 Mon Sep 17 00:00:00 2001 From: Kyle Benne Date: Thu, 9 Nov 2023 23:56:44 -0600 Subject: [PATCH 01/10] Demo Measure errors as C++ exceptions This shows how to turn errors arising from Swig director calls into normal C++ exceptions. ref #5018 --- ruby/engine/CMakeLists.txt | 14 ++++++++++++ ruby/engine/test/BadMeasure/measure.rb | 23 ++++++++++++++++++++ ruby/engine/test/RubyEngine_GTest.cpp | 30 ++++++++++++++++++++++++++ src/measure/Measure.i | 5 +++++ 4 files changed, 72 insertions(+) create mode 100644 ruby/engine/test/BadMeasure/measure.rb create mode 100644 ruby/engine/test/RubyEngine_GTest.cpp diff --git a/ruby/engine/CMakeLists.txt b/ruby/engine/CMakeLists.txt index a5d3dc3e5f2..601be0496cd 100644 --- a/ruby/engine/CMakeLists.txt +++ b/ruby/engine/CMakeLists.txt @@ -287,4 +287,18 @@ elseif(UNIX) ) endif() +if(BUILD_TESTING) + set(rubyengine_test_depends + openstudio_scriptengine + openstudiolib + CONAN_PKG::fmt + ) + + set(rubyengine_test_src + test/RubyEngine_GTest.cpp + ) + + CREATE_TEST_TARGETS(rubyengine "${rubyengine_test_src}" "${rubyengine_test_depends}") +endif() + install(TARGETS rubyengine EXPORT openstudio DESTINATION ${LIB_DESTINATION_DIR} COMPONENT "CLI") diff --git a/ruby/engine/test/BadMeasure/measure.rb b/ruby/engine/test/BadMeasure/measure.rb new file mode 100644 index 00000000000..d8dc5c050b3 --- /dev/null +++ b/ruby/engine/test/BadMeasure/measure.rb @@ -0,0 +1,23 @@ +class BadMeasure < OpenStudio::Measure::ModelMeasure + + def name + return "Bad Measure" + end + + def arguments(model) + raise "oops" + + # args = OpenStudio::Measure::OSArgumentVector.new + + # return args + end + + def run(model, runner, user_arguments) + super(model, runner, user_arguments) + + return true + end + +end + +BadMeasure.new.registerWithApplication diff --git a/ruby/engine/test/RubyEngine_GTest.cpp b/ruby/engine/test/RubyEngine_GTest.cpp new file mode 100644 index 00000000000..a203bf1090a --- /dev/null +++ b/ruby/engine/test/RubyEngine_GTest.cpp @@ -0,0 +1,30 @@ +#include "measure/ModelMeasure.hpp" +#include "measure/OSArgument.hpp" +#include "measure/OSMeasure.hpp" +#include "model/Model.hpp" +#include "scriptengine/ScriptEngine.hpp" +#include + +class RubyEngineFixture : public testing::Test {}; + +TEST_F(RubyEngineFixture, Demo) { + std::vector args; + + openstudio::ScriptEngineInstance rubyEngine("rubyengine", args); + + rubyEngine->registerType("openstudio::measure::OSMeasure *"); + rubyEngine->registerType("openstudio::measure::ModelMeasure *"); + rubyEngine->exec("OpenStudio::init_rest_of_openstudio()"); + + const std::string className = "BadMeasure"; + const openstudio::path scriptPath = openstudio::getApplicationSourceDirectory() / + "ruby/engine/test/BadMeasure/measure.rb"; + + auto measureScriptObject = rubyEngine->loadMeasure(scriptPath, className); + auto measure = rubyEngine->getAs(measureScriptObject); + + ASSERT_EQ(measure->name(), "Bad Measure"); + + openstudio::model::Model model; + ASSERT_ANY_THROW(measure->arguments(model)); +} diff --git a/src/measure/Measure.i b/src/measure/Measure.i index ea239940b8a..dd11f123ea7 100644 --- a/src/measure/Measure.i +++ b/src/measure/Measure.i @@ -96,6 +96,11 @@ }; #if defined SWIGRUBY + %feature("director:except") { + // Look at openstudio::evalString for a possible approach to reporting more info or a callstack. + // This will at least protect from calls to bad ruby code. + throw Swig::DirectorMethodException($error); + } %ignore OSMeasureInfoGetter; From 4382c88c25f0c4ff76d044826bc50f70736dffbc Mon Sep 17 00:00:00 2001 From: Julien Marrec Date: Fri, 10 Nov 2023 11:47:46 +0100 Subject: [PATCH 02/10] Add director:except with full traceback for Python + add GTest --- python/engine/CMakeLists.txt | 14 +++ python/engine/test/BadMeasure/measure.py | 33 ++++++ python/engine/test/BadMeasure/measure.xml | 59 ++++++++++ python/engine/test/PythonEngine_GTest.cpp | 104 ++++++++++++++++++ .../engine/test/WrongMethodMeasure/measure.py | 30 +++++ .../test/WrongMethodMeasure/measure.xml | 59 ++++++++++ ruby/engine/test/BadMeasure/measure.xml | 54 +++++++++ src/measure/Measure.i | 101 +++++++++++++++++ 8 files changed, 454 insertions(+) create mode 100644 python/engine/test/BadMeasure/measure.py create mode 100644 python/engine/test/BadMeasure/measure.xml create mode 100644 python/engine/test/PythonEngine_GTest.cpp create mode 100644 python/engine/test/WrongMethodMeasure/measure.py create mode 100644 python/engine/test/WrongMethodMeasure/measure.xml create mode 100644 ruby/engine/test/BadMeasure/measure.xml diff --git a/python/engine/CMakeLists.txt b/python/engine/CMakeLists.txt index 27d5cd211d0..1343becdea3 100644 --- a/python/engine/CMakeLists.txt +++ b/python/engine/CMakeLists.txt @@ -44,6 +44,20 @@ target_compile_options(pythonengine PRIVATE target_compile_definitions(pythonengine PRIVATE openstudio_scriptengine_EXPORTS SHARED_OS_LIBS) +if(BUILD_TESTING) + set(pythonengine_test_depends + openstudio_scriptengine + openstudiolib + CONAN_PKG::fmt + ) + + set(pythonengine_test_src + test/PythonEngine_GTest.cpp + ) + + CREATE_TEST_TARGETS(pythonengine "${pythonengine_test_src}" "${pythonengine_test_depends}") +endif() + install(TARGETS pythonengine EXPORT openstudio DESTINATION ${LIB_DESTINATION_DIR} COMPONENT "CLI") # it goes into lib/ and we want to find: diff --git a/python/engine/test/BadMeasure/measure.py b/python/engine/test/BadMeasure/measure.py new file mode 100644 index 00000000000..67471e8588f --- /dev/null +++ b/python/engine/test/BadMeasure/measure.py @@ -0,0 +1,33 @@ +import typing + +import openstudio + + +class BadMeasure(openstudio.measure.ModelMeasure): + def name(self): + return "Bad Measure" + + def modeler_description(self): + return "The arguments method calls another_method which does a raise ValueError" + + def another_method(self): + raise ValueError("oops") + + def arguments(self, model: typing.Optional[openstudio.model.Model] = None): + self.another_method() + args = openstudio.measure.OSArgumentVector() + + return args + + def run( + self, + model: openstudio.model.Model, + runner: openstudio.measure.OSRunner, + user_arguments: openstudio.measure.OSArgumentMap, + ): + """ + define what happens when the measure is run + """ + super().run(model, runner, user_arguments) # Do **NOT** remove this line + +BadMeasure().registerWithApplication() diff --git a/python/engine/test/BadMeasure/measure.xml b/python/engine/test/BadMeasure/measure.xml new file mode 100644 index 00000000000..3b0e0be4172 --- /dev/null +++ b/python/engine/test/BadMeasure/measure.xml @@ -0,0 +1,59 @@ + + + 3.1 + bad_measure + 812d3ebf-c89b-4b93-b400-110ca060b2bb + 25ad8ea8-b28b-4f45-93a6-76f056c28ca8 + 2023-11-10T10:47:04Z + 33A29C78 + BadMeasure + Bad Measure + + The arguments method calls another_method which does a raise ValueError + + + + + Envelope.Opaque + + + + Measure Function + Measure + string + + + Requires EnergyPlus Results + false + boolean + + + Measure Type + ModelMeasure + string + + + Measure Language + Python + string + + + Uses SketchUp API + false + boolean + + + + + + OpenStudio + 3.4.1 + 3.4.1 + + measure.py + py + script + E787E0E0 + + + diff --git a/python/engine/test/PythonEngine_GTest.cpp b/python/engine/test/PythonEngine_GTest.cpp new file mode 100644 index 00000000000..21a40908ab0 --- /dev/null +++ b/python/engine/test/PythonEngine_GTest.cpp @@ -0,0 +1,104 @@ +/*********************************************************************************************************************** +* OpenStudio(R), Copyright (c) Alliance for Sustainable Energy, LLC. +* See also https://openstudio.net/license +***********************************************************************************************************************/ + +#include + +#include "measure/ModelMeasure.hpp" +#include "measure/OSArgument.hpp" +#include "measure/OSMeasure.hpp" +#include "model/Model.hpp" +#include "scriptengine/ScriptEngine.hpp" + +#include + +class PythonEngineFixture : public testing::Test +{ + public: + static openstudio::path getScriptPath(const std::string& classAndDirName) { + openstudio::path scriptPath = openstudio::getApplicationSourceDirectory() / fmt::format("python/engine/test/{}/measure.py", classAndDirName); + OS_ASSERT(openstudio::filesystem::is_regular_file(scriptPath)); + return scriptPath; + } + + protected: + // initialize for each test + virtual void SetUp() override { + std::vector args; + thisEngine = std::make_unique("pythonengine", args); + + (*thisEngine)->registerType("openstudio::measure::OSMeasure *"); + (*thisEngine)->registerType("openstudio::measure::ModelMeasure *"); + } + // tear down after each test + virtual void TearDown() override { + // Want to ensure the engine is reset regardless of the test outcome, or it may throw a segfault + thisEngine->reset(); + } + + std::unique_ptr thisEngine; +}; + +TEST_F(PythonEngineFixture, BadMeasure) { + + const std::string classAndDirName = "BadMeasure"; + + const auto scriptPath = getScriptPath(classAndDirName); + auto measureScriptObject = (*thisEngine)->loadMeasure(scriptPath, classAndDirName); + auto* measurePtr = (*thisEngine)->getAs(measureScriptObject); + + ASSERT_EQ(measurePtr->name(), "Bad Measure"); + + std::string expected_exception = fmt::format(R"(SWIG director method error. In method 'arguments': `ValueError('oops')` +Python traceback follows: +``` +Traceback (most recent call last): + File "{0}", line 17, in arguments + self.another_method() + File "{0}", line 14, in another_method + raise ValueError("oops") +ValueError: oops +```)", + scriptPath.generic_string()); + + openstudio::model::Model model; + try { + measurePtr->arguments(model); + ASSERT_FALSE(true) << "Expected measure arguments(model) to throw"; + } catch (std::exception& e) { + std::string error = e.what(); + EXPECT_EQ(expected_exception, error); + } +} + +TEST_F(PythonEngineFixture, WrongMethodMeasure) { + + const std::string classAndDirName = "WrongMethodMeasure"; + + const auto scriptPath = getScriptPath(classAndDirName); + auto measureScriptObject = (*thisEngine)->loadMeasure(scriptPath, classAndDirName); + auto* measurePtr = (*thisEngine)->getAs(measureScriptObject); + + ASSERT_EQ(measurePtr->name(), "Wrong Method Measure"); + + std::string expected_exception = + fmt::format(R"(SWIG director method error. In method 'arguments': `AttributeError("'Model' object has no attribute 'nonExistingMethod'")` +Python traceback follows: +``` +Traceback (most recent call last): + File "{}", line 14, in arguments + model.nonExistingMethod() +AttributeError: 'Model' object has no attribute 'nonExistingMethod' +```)", + scriptPath.generic_string()); + + openstudio::model::Model model; + try { + measurePtr->arguments(model); + ASSERT_FALSE(true) << "Expected measure arguments(model) to throw"; + } catch (std::exception& e) { + std::string error = e.what(); + EXPECT_EQ(expected_exception, error); + } +} diff --git a/python/engine/test/WrongMethodMeasure/measure.py b/python/engine/test/WrongMethodMeasure/measure.py new file mode 100644 index 00000000000..a2f524afe3d --- /dev/null +++ b/python/engine/test/WrongMethodMeasure/measure.py @@ -0,0 +1,30 @@ +import typing + +import openstudio + + +class WrongMethodMeasure(openstudio.measure.ModelMeasure): + def name(self): + return "Wrong Method Measure" + + def modeler_description(self): + return "The arguments method calls a non existing method on the model passed as argument" + + def arguments(self, model: typing.Optional[openstudio.model.Model] = None): + model.nonExistingMethod() + args = openstudio.measure.OSArgumentVector() + + return args + + def run( + self, + model: openstudio.model.Model, + runner: openstudio.measure.OSRunner, + user_arguments: openstudio.measure.OSArgumentMap, + ): + """ + define what happens when the measure is run + """ + super().run(model, runner, user_arguments) # Do **NOT** remove this line + +WrongMethodMeasure().registerWithApplication() diff --git a/python/engine/test/WrongMethodMeasure/measure.xml b/python/engine/test/WrongMethodMeasure/measure.xml new file mode 100644 index 00000000000..47d90331a24 --- /dev/null +++ b/python/engine/test/WrongMethodMeasure/measure.xml @@ -0,0 +1,59 @@ + + + 3.1 + wrong_method_measure + 11111111-c89b-4b93-b400-110ca060b2bb + 998cdb81-addf-4fae-809d-175c6475c592 + 2023-11-10T10:37:53Z + 33A29C78 + WrongMethodMeasure + Wrong Method Measure + + The arguments method calls a non existing method on the model passed as argument + + + + + Envelope.Opaque + + + + Measure Function + Measure + string + + + Requires EnergyPlus Results + false + boolean + + + Measure Type + ModelMeasure + string + + + Measure Language + Python + string + + + Uses SketchUp API + false + boolean + + + + + + OpenStudio + 3.4.1 + 3.4.1 + + measure.py + py + script + 7DCE622C + + + diff --git a/ruby/engine/test/BadMeasure/measure.xml b/ruby/engine/test/BadMeasure/measure.xml new file mode 100644 index 00000000000..b9d91349264 --- /dev/null +++ b/ruby/engine/test/BadMeasure/measure.xml @@ -0,0 +1,54 @@ + + + 3.1 + BadMeasure + 812d3ebf-c89b-4b93-b400-110ca060b2bb + 3688e313-ded2-46f1-a9b0-c128a830e559 + 2023-11-10T08:32:40Z + EB1A0C08 + BadMeasure + BadMeasure + + + + + + + Envelope.Opaque + + + + Measure Function + Measure + string + + + Requires EnergyPlus Results + false + boolean + + + Measure Type + ModelMeasure + string + + + Uses SketchUp API + false + boolean + + + + + + OpenStudio + 0.11.5 + 0.11.5 + + measure.rb + rb + script + 91513FCD + + + diff --git a/src/measure/Measure.i b/src/measure/Measure.i index dd11f123ea7..4dbe68e5bfb 100644 --- a/src/measure/Measure.i +++ b/src/measure/Measure.i @@ -112,4 +112,105 @@ #endif +#if defined SWIGPYTHON + %feature("director:except") { + +#if 1 + if ($error != nullptr) { + + PyObject *exc_type = nullptr; + PyObject *exc_value = nullptr; + PyObject *exc_tb = nullptr; + PyErr_Fetch(&exc_type, &exc_value, &exc_tb); + PyErr_NormalizeException(&exc_type, &exc_value, &exc_tb); + PyObject *str_exc_value = PyObject_Repr(exc_value); // Now a unicode object + PyObject *pyStr2 = PyUnicode_AsEncodedString(str_exc_value, "utf-8", "Error ~"); + Py_DECREF(str_exc_value); + char *strExcValue = PyBytes_AsString(pyStr2); // NOLINT(hicpp-signed-bitwise) + Py_DECREF(pyStr2); + + std::string err_msg = "In method '$symname': `" + std::string{strExcValue} + "`"; + + // See if we can get a full traceback. + // Calls into python, and does the same as capturing the exception in `e` + // then `print(traceback.format_exception(e.type, e.value, e.tb))` + PyObject *pModuleName = PyUnicode_DecodeFSDefault("traceback"); + PyObject *pyth_module = PyImport_Import(pModuleName); + Py_DECREF(pModuleName); + + if (pyth_module == nullptr) { + err_msg += "\nCannot find 'traceback' module, this is weird"; + Swig::DirectorMethodException::raise(err_msg.c_str()); + } + + PyObject *pyth_func = PyObject_GetAttrString(pyth_module, "format_exception"); + Py_DECREF(pyth_module); // PyImport_Import returns a new reference, decrement it + + if (pyth_func || PyCallable_Check(pyth_func)) { + + PyObject *pyth_val = PyObject_CallFunction(pyth_func, "OOO", exc_type, exc_value, exc_tb); + + // traceback.format_exception returns a list, so iterate on that + if (!pyth_val || !PyList_Check(pyth_val)) { // NOLINT(hicpp-signed-bitwise) + err_msg += "\nIn reportPythonError(), traceback.format_exception did not return a list."; + Swig::DirectorMethodException::raise(err_msg.c_str()); + } + + unsigned long numVals = PyList_Size(pyth_val); + if (numVals == 0) { + err_msg += "\nNo traceback available"; + Swig::DirectorMethodException::raise(err_msg.c_str()); + } + + err_msg += "\nPython traceback follows:\n```"; + + for (unsigned long itemNum = 0; itemNum < numVals; itemNum++) { + PyObject *item = PyList_GetItem(pyth_val, itemNum); + if (PyUnicode_Check(item)) { // NOLINT(hicpp-signed-bitwise) -- something inside Python code causes warning + std::string traceback_line = PyUnicode_AsUTF8(item); + if (!traceback_line.empty() && traceback_line[traceback_line.length() - 1] == '\n') { + traceback_line.erase(traceback_line.length() - 1); + } + err_msg += "\n" + traceback_line; + } + // PyList_GetItem returns a borrowed reference, do not decrement + } + + err_msg += "\n```"; + + // PyList_Size returns a borrowed reference, do not decrement + Py_DECREF(pyth_val); // PyObject_CallFunction returns new reference, decrement + } + Py_DECREF(pyth_func); // PyObject_GetAttrString returns a new reference, decrement it + Swig::DirectorMethodException::raise(err_msg.c_str()); + } +#else + if ($error != NULL) { + PyObject *exc, *val, *tb; + PyErr_Fetch(&exc, &val, &tb); + PyErr_NormalizeException(&exc, &val, &tb); + std::string err_msg("In method '$symname': "); + + PyObject* exc_str = PyObject_GetAttrString(exc, "__name__"); + err_msg += PyUnicode_AsUTF8(exc_str); + Py_XDECREF(exc_str); + + if (val != NULL) + { + PyObject* val_str = PyObject_Str(val); + err_msg += ": "; + err_msg += PyUnicode_AsUTF8(val_str); + Py_XDECREF(val_str); + } + + Py_XDECREF(exc); + Py_XDECREF(val); + Py_XDECREF(tb); + + Swig::DirectorMethodException::raise(err_msg.c_str()); + } +#endif + } +#endif + #endif // MEASURE_I From 0e6c8451bdae76f88a0ad4ecc74cb66b46bda365 Mon Sep 17 00:00:00 2001 From: Julien Marrec Date: Fri, 10 Nov 2023 12:02:55 +0100 Subject: [PATCH 03/10] Improve testing for RubyEngine. --- ruby/engine/test/BadMeasure/measure.rb | 13 +++- ruby/engine/test/BadMeasure/measure.xml | 12 ++-- ruby/engine/test/RubyEngine_GTest.cpp | 70 +++++++++++++++---- .../engine/test/WrongMethodMeasure/measure.rb | 26 +++++++ .../test/WrongMethodMeasure/measure.xml | 59 ++++++++++++++++ 5 files changed, 156 insertions(+), 24 deletions(-) create mode 100644 ruby/engine/test/WrongMethodMeasure/measure.rb create mode 100644 ruby/engine/test/WrongMethodMeasure/measure.xml diff --git a/ruby/engine/test/BadMeasure/measure.rb b/ruby/engine/test/BadMeasure/measure.rb index d8dc5c050b3..2e1681b4e8c 100644 --- a/ruby/engine/test/BadMeasure/measure.rb +++ b/ruby/engine/test/BadMeasure/measure.rb @@ -4,12 +4,19 @@ def name return "Bad Measure" end - def arguments(model) + def modeler_description + return "The arguments method calls another_method which does a raise ValueError" + end + + def another_method raise "oops" + end - # args = OpenStudio::Measure::OSArgumentVector.new + def arguments(model) + another_method + args = OpenStudio::Measure::OSArgumentVector.new - # return args + return args end def run(model, runner, user_arguments) diff --git a/ruby/engine/test/BadMeasure/measure.xml b/ruby/engine/test/BadMeasure/measure.xml index b9d91349264..f28b1c73009 100644 --- a/ruby/engine/test/BadMeasure/measure.xml +++ b/ruby/engine/test/BadMeasure/measure.xml @@ -1,15 +1,15 @@ 3.1 - BadMeasure + bad_measure 812d3ebf-c89b-4b93-b400-110ca060b2bb - 3688e313-ded2-46f1-a9b0-c128a830e559 - 2023-11-10T08:32:40Z + 4b3097c0-9874-4478-8e7f-45fa952a91eb + 2023-11-10T10:59:53Z EB1A0C08 BadMeasure - BadMeasure + Bad Measure - + The arguments method calls another_method which does a raise ValueError @@ -48,7 +48,7 @@ measure.rb rb script - 91513FCD + D22AC6AC diff --git a/ruby/engine/test/RubyEngine_GTest.cpp b/ruby/engine/test/RubyEngine_GTest.cpp index a203bf1090a..020c33b291c 100644 --- a/ruby/engine/test/RubyEngine_GTest.cpp +++ b/ruby/engine/test/RubyEngine_GTest.cpp @@ -1,30 +1,70 @@ +/*********************************************************************************************************************** +* OpenStudio(R), Copyright (c) Alliance for Sustainable Energy, LLC. +* See also https://openstudio.net/license +***********************************************************************************************************************/ + +#include + #include "measure/ModelMeasure.hpp" #include "measure/OSArgument.hpp" #include "measure/OSMeasure.hpp" #include "model/Model.hpp" #include "scriptengine/ScriptEngine.hpp" -#include -class RubyEngineFixture : public testing::Test {}; +#include + +class RubyEngineFixture : public testing::Test +{ + public: + static openstudio::path getScriptPath(const std::string& classAndDirName) { + openstudio::path scriptPath = openstudio::getApplicationSourceDirectory() / fmt::format("ruby/engine/test/{}/measure.rb", classAndDirName); + OS_ASSERT(openstudio::filesystem::is_regular_file(scriptPath)); + return scriptPath; + } + + protected: + // initialize for each test + virtual void SetUp() override { + std::vector args; + thisEngine = std::make_unique("rubyengine", args); + + (*thisEngine)->registerType("openstudio::measure::OSMeasure *"); + (*thisEngine)->registerType("openstudio::measure::ModelMeasure *"); + (*thisEngine)->exec("OpenStudio::init_rest_of_openstudio()"); + } + // tear down after each test + virtual void TearDown() override { + // Want to ensure the engine is reset regardless of the test outcome, or it may throw a segfault + thisEngine->reset(); + } -TEST_F(RubyEngineFixture, Demo) { - std::vector args; + std::unique_ptr thisEngine; +}; - openstudio::ScriptEngineInstance rubyEngine("rubyengine", args); +TEST_F(RubyEngineFixture, BadMeasure) { + + const std::string classAndDirName = "BadMeasure"; + + const auto scriptPath = getScriptPath(classAndDirName); + auto measureScriptObject = (*thisEngine)->loadMeasure(scriptPath, classAndDirName); + auto* measurePtr = (*thisEngine)->getAs(measureScriptObject); + + ASSERT_EQ(measurePtr->name(), "Bad Measure"); + + openstudio::model::Model model; + ASSERT_ANY_THROW(measurePtr->arguments(model)); +} - rubyEngine->registerType("openstudio::measure::OSMeasure *"); - rubyEngine->registerType("openstudio::measure::ModelMeasure *"); - rubyEngine->exec("OpenStudio::init_rest_of_openstudio()"); +TEST_F(RubyEngineFixture, WrongMethodMeasure) { - const std::string className = "BadMeasure"; - const openstudio::path scriptPath = openstudio::getApplicationSourceDirectory() / - "ruby/engine/test/BadMeasure/measure.rb"; + const std::string classAndDirName = "WrongMethodMeasure"; - auto measureScriptObject = rubyEngine->loadMeasure(scriptPath, className); - auto measure = rubyEngine->getAs(measureScriptObject); + const auto scriptPath = getScriptPath(classAndDirName); + auto measureScriptObject = (*thisEngine)->loadMeasure(scriptPath, classAndDirName); + auto* measurePtr = (*thisEngine)->getAs(measureScriptObject); - ASSERT_EQ(measure->name(), "Bad Measure"); + ASSERT_EQ(measurePtr->name(), "Wrong Method Measure"); openstudio::model::Model model; - ASSERT_ANY_THROW(measure->arguments(model)); + ASSERT_ANY_THROW(measurePtr->arguments(model)); } diff --git a/ruby/engine/test/WrongMethodMeasure/measure.rb b/ruby/engine/test/WrongMethodMeasure/measure.rb new file mode 100644 index 00000000000..6f9eb26c22a --- /dev/null +++ b/ruby/engine/test/WrongMethodMeasure/measure.rb @@ -0,0 +1,26 @@ +class WrongMethodMeasure < OpenStudio::Measure::ModelMeasure + + def name + return "Wrong Method Measure" + end + + def modeler_description + return "The arguments method calls a non existing method on the model passed as argument" + end + + def arguments(model) + model.nonExistingMethod() + args = OpenStudio::Measure::OSArgumentVector.new + + return args + end + + def run(model, runner, user_arguments) + super(model, runner, user_arguments) + + return true + end + +end + +WrongMethodMeasure.new.registerWithApplication diff --git a/ruby/engine/test/WrongMethodMeasure/measure.xml b/ruby/engine/test/WrongMethodMeasure/measure.xml new file mode 100644 index 00000000000..812f4c995d2 --- /dev/null +++ b/ruby/engine/test/WrongMethodMeasure/measure.xml @@ -0,0 +1,59 @@ + + + 3.1 + wrong_method_measure + 11111111-c89b-4b93-b400-110ca060b2bb + 8c8f5697-b6e0-4ae0-90f9-cace0f872d41 + 2023-11-10T11:02:27Z + 5872977E + WrongMethodMeasure + Wrong Method Measure + + The arguments method calls a non existing method on the model passed as argument + + + + + Envelope.Opaque + + + + Measure Function + Measure + string + + + Requires EnergyPlus Results + false + boolean + + + Measure Type + ModelMeasure + string + + + Uses SketchUp API + false + boolean + + + Measure Language + Ruby + string + + + + + + OpenStudio + 3.4.1 + 3.4.1 + + measure.rb + rb + script + BB98326D + + + From d725a87335da3d3ae1015c30ba3b6d37e3eaee21 Mon Sep 17 00:00:00 2001 From: Julien Marrec Date: Fri, 10 Nov 2023 12:50:26 +0100 Subject: [PATCH 04/10] Implement a nice traceback for Ruby Director method exception --- ruby/engine/test/RubyEngine_GTest.cpp | 40 ++++++++++++++++++++-- src/measure/Measure.i | 48 ++++++++++++++++++++++++++- 2 files changed, 85 insertions(+), 3 deletions(-) diff --git a/ruby/engine/test/RubyEngine_GTest.cpp b/ruby/engine/test/RubyEngine_GTest.cpp index 020c33b291c..8719c59512b 100644 --- a/ruby/engine/test/RubyEngine_GTest.cpp +++ b/ruby/engine/test/RubyEngine_GTest.cpp @@ -13,6 +13,8 @@ #include +#include + class RubyEngineFixture : public testing::Test { public: @@ -22,6 +24,12 @@ class RubyEngineFixture : public testing::Test return scriptPath; } + static std::string stripAddressFromErrorMessage(const std::string& error_message) { + static std::regex object_address_re("0x[[:alnum:]]*>"); + + return std::regex_replace(error_message, object_address_re, "ADDRESS>"); + } + protected: // initialize for each test virtual void SetUp() override { @@ -51,8 +59,22 @@ TEST_F(RubyEngineFixture, BadMeasure) { ASSERT_EQ(measurePtr->name(), "Bad Measure"); + std::string expected_exception = fmt::format(R"(SWIG director method error. RuntimeError: oops +["/Users/julien/Software/Others/OpenStudio2/ruby/engine/test/BadMeasure/measure.rb:12:in `another_method'", "/Users/julien/Software/Others/OpenStudio2/ruby/engine/test/BadMeasure/measure.rb:16:in `arguments'"] + +Traceback: +{0}:12:in `another_method' +{0}:16:in `arguments')", + scriptPath.generic_string()); + openstudio::model::Model model; - ASSERT_ANY_THROW(measurePtr->arguments(model)); + try { + measurePtr->arguments(model); + ASSERT_FALSE(true) << "Expected measure arguments(model) to throw"; + } catch (std::exception& e) { + std::string error = e.what(); + EXPECT_EQ(expected_exception, error); + } } TEST_F(RubyEngineFixture, WrongMethodMeasure) { @@ -65,6 +87,20 @@ TEST_F(RubyEngineFixture, WrongMethodMeasure) { ASSERT_EQ(measurePtr->name(), "Wrong Method Measure"); + std::string expected_exception = + fmt::format(R"(SWIG director method error. NoMethodError: undefined method `nonExistingMethod' for # +["/Users/julien/Software/Others/OpenStudio2/ruby/engine/test/WrongMethodMeasure/measure.rb:12:in `arguments'"] + +Traceback: +{0}:12:in `arguments')", + scriptPath.generic_string()); + openstudio::model::Model model; - ASSERT_ANY_THROW(measurePtr->arguments(model)); + try { + measurePtr->arguments(model); + ASSERT_FALSE(true) << "Expected measure arguments(model) to throw"; + } catch (std::exception& e) { + std::string error = e.what(); + EXPECT_EQ(expected_exception, stripAddressFromErrorMessage(error)); + } } diff --git a/src/measure/Measure.i b/src/measure/Measure.i index 4dbe68e5bfb..c6802231742 100644 --- a/src/measure/Measure.i +++ b/src/measure/Measure.i @@ -99,7 +99,53 @@ %feature("director:except") { // Look at openstudio::evalString for a possible approach to reporting more info or a callstack. // This will at least protect from calls to bad ruby code. - throw Swig::DirectorMethodException($error); + + VALUE errinfo = rb_errinfo(); + VALUE exception_class = rb_obj_class(errinfo); + VALUE classNameValue = rb_class_name(exception_class); + std::string className(StringValuePtr(classNameValue)); + + VALUE errstr = rb_obj_as_string(errinfo); + std::string errMessage(StringValuePtr(errstr)); + + std::string totalErr = className + ": " + errMessage; + + // I just **cannot** figure out a way to get the error location in C, without calling $@.to_s. Seems nothing is available here + int locationError; + VALUE locval = rb_eval_string_protect("$@.to_s", &locationError); + std::string loc; + if (locationError == 0) { + loc = StringValuePtr(locval); + } else { + loc = "Failed to get VM location"; + } + + // Generally speaking, the backtrace is there, but not for the case where it's a stack too deep error + const ID ID_backtrace = rb_intern_const("backtrace"); + if (exception_class != rb_eSysStackError && rb_respond_to(errinfo, ID_backtrace)) { + std::string btlines; + /*volatile*/ VALUE backtrace; + if (!NIL_P(backtrace = rb_funcall(errinfo, ID_backtrace, 0))) { + VALUE backtracejoin = rb_ary_join(backtrace, rb_str_new2("\n")); + btlines = StringValuePtr(backtracejoin); + + // Get the backing C array of the ruby array + VALUE* elements = RARRAY_PTR(backtrace); + for (long c = 0; c < RARRAY_LEN(backtrace); ++c) { + VALUE entry = elements[c]; + [[maybe_unused]] char* backtrace_line = RSTRING_PTR(entry); + char* backtrace_line2 = StringValuePtr(entry); + } + } + + if (!btlines.empty()) { + loc += "\n\nTraceback:\n" + btlines; + } + } else { + } + + totalErr += "\n" + loc; + throw Swig::DirectorMethodException(totalErr.c_str()); } %ignore OSMeasureInfoGetter; From 21fff07b970b2c55e71977247df0550732fdaa6f Mon Sep 17 00:00:00 2001 From: Julien Marrec Date: Fri, 10 Nov 2023 13:03:26 +0100 Subject: [PATCH 05/10] Cleanup Measure.i from leftovers --- src/measure/Measure.i | 77 +++++++++++++------------------------------ 1 file changed, 23 insertions(+), 54 deletions(-) diff --git a/src/measure/Measure.i b/src/measure/Measure.i index c6802231742..86ce8640c10 100644 --- a/src/measure/Measure.i +++ b/src/measure/Measure.i @@ -97,9 +97,7 @@ #if defined SWIGRUBY %feature("director:except") { - // Look at openstudio::evalString for a possible approach to reporting more info or a callstack. - // This will at least protect from calls to bad ruby code. - + // Mimic openstudio::evalString for a possible approach to reporting more info or a callstack. VALUE errinfo = rb_errinfo(); VALUE exception_class = rb_obj_class(errinfo); VALUE classNameValue = rb_class_name(exception_class); @@ -161,7 +159,6 @@ #if defined SWIGPYTHON %feature("director:except") { -#if 1 if ($error != nullptr) { PyObject *exc_type = nullptr; @@ -185,8 +182,8 @@ Py_DECREF(pModuleName); if (pyth_module == nullptr) { - err_msg += "\nCannot find 'traceback' module, this is weird"; - Swig::DirectorMethodException::raise(err_msg.c_str()); + err_msg += "\nCannot find 'traceback' module, this should not happen"; + throw Swig::DirectorMethodException(err_msg.c_str()); } PyObject *pyth_func = PyObject_GetAttrString(pyth_module, "format_exception"); @@ -199,63 +196,35 @@ // traceback.format_exception returns a list, so iterate on that if (!pyth_val || !PyList_Check(pyth_val)) { // NOLINT(hicpp-signed-bitwise) err_msg += "\nIn reportPythonError(), traceback.format_exception did not return a list."; - Swig::DirectorMethodException::raise(err_msg.c_str()); - } - - unsigned long numVals = PyList_Size(pyth_val); - if (numVals == 0) { - err_msg += "\nNo traceback available"; - Swig::DirectorMethodException::raise(err_msg.c_str()); - } - - err_msg += "\nPython traceback follows:\n```"; - - for (unsigned long itemNum = 0; itemNum < numVals; itemNum++) { - PyObject *item = PyList_GetItem(pyth_val, itemNum); - if (PyUnicode_Check(item)) { // NOLINT(hicpp-signed-bitwise) -- something inside Python code causes warning - std::string traceback_line = PyUnicode_AsUTF8(item); - if (!traceback_line.empty() && traceback_line[traceback_line.length() - 1] == '\n') { - traceback_line.erase(traceback_line.length() - 1); + } else { + unsigned long numVals = PyList_Size(pyth_val); + if (numVals == 0) { + err_msg += "\nNo traceback available"; + } else { + err_msg += "\nPython traceback follows:\n```"; + + for (unsigned long itemNum = 0; itemNum < numVals; itemNum++) { + PyObject *item = PyList_GetItem(pyth_val, itemNum); + if (PyUnicode_Check(item)) { // NOLINT(hicpp-signed-bitwise) -- something inside Python code causes warning + std::string traceback_line = PyUnicode_AsUTF8(item); + if (!traceback_line.empty() && traceback_line[traceback_line.length() - 1] == '\n') { + traceback_line.erase(traceback_line.length() - 1); + } + err_msg += "\n" + traceback_line; } - err_msg += "\n" + traceback_line; + // PyList_GetItem returns a borrowed reference, do not decrement } - // PyList_GetItem returns a borrowed reference, do not decrement - } - err_msg += "\n```"; + err_msg += "\n```"; + } + } // PyList_Size returns a borrowed reference, do not decrement Py_DECREF(pyth_val); // PyObject_CallFunction returns new reference, decrement } Py_DECREF(pyth_func); // PyObject_GetAttrString returns a new reference, decrement it - Swig::DirectorMethodException::raise(err_msg.c_str()); - } -#else - if ($error != NULL) { - PyObject *exc, *val, *tb; - PyErr_Fetch(&exc, &val, &tb); - PyErr_NormalizeException(&exc, &val, &tb); - std::string err_msg("In method '$symname': "); - - PyObject* exc_str = PyObject_GetAttrString(exc, "__name__"); - err_msg += PyUnicode_AsUTF8(exc_str); - Py_XDECREF(exc_str); - - if (val != NULL) - { - PyObject* val_str = PyObject_Str(val); - err_msg += ": "; - err_msg += PyUnicode_AsUTF8(val_str); - Py_XDECREF(val_str); - } - - Py_XDECREF(exc); - Py_XDECREF(val); - Py_XDECREF(tb); - - Swig::DirectorMethodException::raise(err_msg.c_str()); + throw Swig::DirectorMethodException(err_msg.c_str()); } -#endif } #endif From a9a795c381d54d17393def60461d8abcf4c81b3a Mon Sep 17 00:00:00 2001 From: Julien Marrec Date: Fri, 10 Nov 2023 13:33:17 +0100 Subject: [PATCH 06/10] Cleanup unused bits even more --- src/measure/Measure.i | 17 +++-------------- 1 file changed, 3 insertions(+), 14 deletions(-) diff --git a/src/measure/Measure.i b/src/measure/Measure.i index 86ce8640c10..974c1f4b4a4 100644 --- a/src/measure/Measure.i +++ b/src/measure/Measure.i @@ -121,25 +121,14 @@ // Generally speaking, the backtrace is there, but not for the case where it's a stack too deep error const ID ID_backtrace = rb_intern_const("backtrace"); if (exception_class != rb_eSysStackError && rb_respond_to(errinfo, ID_backtrace)) { - std::string btlines; /*volatile*/ VALUE backtrace; if (!NIL_P(backtrace = rb_funcall(errinfo, ID_backtrace, 0))) { VALUE backtracejoin = rb_ary_join(backtrace, rb_str_new2("\n")); - btlines = StringValuePtr(backtracejoin); - - // Get the backing C array of the ruby array - VALUE* elements = RARRAY_PTR(backtrace); - for (long c = 0; c < RARRAY_LEN(backtrace); ++c) { - VALUE entry = elements[c]; - [[maybe_unused]] char* backtrace_line = RSTRING_PTR(entry); - char* backtrace_line2 = StringValuePtr(entry); + const std::string btlines = StringValuePtr(backtracejoin); + if (!btlines.empty()) { + loc += "\n\nTraceback:\n" + btlines; } } - - if (!btlines.empty()) { - loc += "\n\nTraceback:\n" + btlines; - } - } else { } totalErr += "\n" + loc; From 3875f88c7eaeb79da173502b6b2880c434561009 Mon Sep 17 00:00:00 2001 From: Julien Marrec Date: Fri, 10 Nov 2023 14:55:58 +0100 Subject: [PATCH 07/10] Don't use "$@.to_s" if we manage to produce a backtrace. The fallback is needed for Stack Too deep --- ruby/engine/test/RubyEngine_GTest.cpp | 2 -- src/measure/Measure.i | 24 +++++++++++++----------- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/ruby/engine/test/RubyEngine_GTest.cpp b/ruby/engine/test/RubyEngine_GTest.cpp index 8719c59512b..10686762c5f 100644 --- a/ruby/engine/test/RubyEngine_GTest.cpp +++ b/ruby/engine/test/RubyEngine_GTest.cpp @@ -60,7 +60,6 @@ TEST_F(RubyEngineFixture, BadMeasure) { ASSERT_EQ(measurePtr->name(), "Bad Measure"); std::string expected_exception = fmt::format(R"(SWIG director method error. RuntimeError: oops -["/Users/julien/Software/Others/OpenStudio2/ruby/engine/test/BadMeasure/measure.rb:12:in `another_method'", "/Users/julien/Software/Others/OpenStudio2/ruby/engine/test/BadMeasure/measure.rb:16:in `arguments'"] Traceback: {0}:12:in `another_method' @@ -89,7 +88,6 @@ TEST_F(RubyEngineFixture, WrongMethodMeasure) { std::string expected_exception = fmt::format(R"(SWIG director method error. NoMethodError: undefined method `nonExistingMethod' for # -["/Users/julien/Software/Others/OpenStudio2/ruby/engine/test/WrongMethodMeasure/measure.rb:12:in `arguments'"] Traceback: {0}:12:in `arguments')", diff --git a/src/measure/Measure.i b/src/measure/Measure.i index 974c1f4b4a4..8f8ba27c451 100644 --- a/src/measure/Measure.i +++ b/src/measure/Measure.i @@ -108,17 +108,8 @@ std::string totalErr = className + ": " + errMessage; - // I just **cannot** figure out a way to get the error location in C, without calling $@.to_s. Seems nothing is available here - int locationError; - VALUE locval = rb_eval_string_protect("$@.to_s", &locationError); - std::string loc; - if (locationError == 0) { - loc = StringValuePtr(locval); - } else { - loc = "Failed to get VM location"; - } - // Generally speaking, the backtrace is there, but not for the case where it's a stack too deep error + std::string loc; const ID ID_backtrace = rb_intern_const("backtrace"); if (exception_class != rb_eSysStackError && rb_respond_to(errinfo, ID_backtrace)) { /*volatile*/ VALUE backtrace; @@ -126,10 +117,21 @@ VALUE backtracejoin = rb_ary_join(backtrace, rb_str_new2("\n")); const std::string btlines = StringValuePtr(backtracejoin); if (!btlines.empty()) { - loc += "\n\nTraceback:\n" + btlines; + loc += "\nTraceback:\n" + btlines; } } } + if (loc.empty()) { + // In case we couldn't produce the backtrace, fall back on this: + // I just **cannot** figure out a way to get the error location in C, without calling $@.to_s. Seems nothing is available here + int locationError; + VALUE locval = rb_eval_string_protect("$@.to_s", &locationError); + if (locationError == 0) { + loc = StringValuePtr(locval); + } else { + loc = "Failed to get VM location"; + } + } totalErr += "\n" + loc; throw Swig::DirectorMethodException(totalErr.c_str()); From f35e8aa4824b19d8af2d7996f82f6752a30635c2 Mon Sep 17 00:00:00 2001 From: Julien Marrec Date: Fri, 10 Nov 2023 14:57:11 +0100 Subject: [PATCH 08/10] Add a test for SystemStackLevel too deep in ruby + cleanup error reporting in that case: show 8 early lines + 12 final --- ruby/engine/test/RubyEngine_GTest.cpp | 50 +++++++++++++++- .../test/StackLevelTooDeepMeasure/measure.rb | 30 ++++++++++ .../test/StackLevelTooDeepMeasure/measure.xml | 59 +++++++++++++++++++ src/measure/Measure.i | 33 +++++++++-- 4 files changed, 166 insertions(+), 6 deletions(-) create mode 100644 ruby/engine/test/StackLevelTooDeepMeasure/measure.rb create mode 100644 ruby/engine/test/StackLevelTooDeepMeasure/measure.xml diff --git a/ruby/engine/test/RubyEngine_GTest.cpp b/ruby/engine/test/RubyEngine_GTest.cpp index 10686762c5f..a83529eeb8f 100644 --- a/ruby/engine/test/RubyEngine_GTest.cpp +++ b/ruby/engine/test/RubyEngine_GTest.cpp @@ -61,7 +61,7 @@ TEST_F(RubyEngineFixture, BadMeasure) { std::string expected_exception = fmt::format(R"(SWIG director method error. RuntimeError: oops -Traceback: +Traceback (most recent call last): {0}:12:in `another_method' {0}:16:in `arguments')", scriptPath.generic_string()); @@ -89,7 +89,7 @@ TEST_F(RubyEngineFixture, WrongMethodMeasure) { std::string expected_exception = fmt::format(R"(SWIG director method error. NoMethodError: undefined method `nonExistingMethod' for # -Traceback: +Traceback (most recent call last): {0}:12:in `arguments')", scriptPath.generic_string()); @@ -102,3 +102,49 @@ TEST_F(RubyEngineFixture, WrongMethodMeasure) { EXPECT_EQ(expected_exception, stripAddressFromErrorMessage(error)); } } + +TEST_F(RubyEngineFixture, StackLevelTooDeepMeasure) { + + const std::string classAndDirName = "StackLevelTooDeepMeasure"; + + const auto scriptPath = getScriptPath(classAndDirName); + auto measureScriptObject = (*thisEngine)->loadMeasure(scriptPath, classAndDirName); + auto* measurePtr = (*thisEngine)->getAs(measureScriptObject); + + ASSERT_EQ(measurePtr->name(), "Stack Level Too Deep Measure"); + + std::string expected_exception = fmt::format(R"(SWIG director method error. SystemStackError: stack level too deep + +Traceback (most recent call last): +{0}:16:in `arguments' +{0}:12:in `s' +{0}:12:in `s' +{0}:12:in `s' +{0}:12:in `s' +{0}:12:in `s' +{0}:12:in `s' + ... 10061 levels... +{0}:12:in `s' +{0}:12:in `s' +{0}:12:in `s' +{0}:12:in `s' +{0}:12:in `s' +{0}:12:in `s' +{0}:12:in `s' +{0}:12:in `s' +{0}:12:in `s' +{0}:12:in `s' +{0}:12:in `s' +{0}:12:in `s')", + scriptPath.generic_string()); + + openstudio::model::Model model; + try { + measurePtr->arguments(model); + ASSERT_FALSE(true) << "Expected measure arguments(model) to throw"; + } catch (std::exception& e) { + std::string error = e.what(); + EXPECT_EQ(expected_exception, stripAddressFromErrorMessage(error)); + } +} + diff --git a/ruby/engine/test/StackLevelTooDeepMeasure/measure.rb b/ruby/engine/test/StackLevelTooDeepMeasure/measure.rb new file mode 100644 index 00000000000..23c9dc5c339 --- /dev/null +++ b/ruby/engine/test/StackLevelTooDeepMeasure/measure.rb @@ -0,0 +1,30 @@ +class StackLevelTooDeepMeasure < OpenStudio::Measure::ModelMeasure + + def name + return "Stack Level Too Deep Measure" + end + + def modeler_description + return "The arguments method calls an infinitely recursive function" + end + + def s(x) + return x * s(x-1) + end + + def arguments(model) + s(10) + args = OpenStudio::Measure::OSArgumentVector.new + + return args + end + + def run(model, runner, user_arguments) + super(model, runner, user_arguments) + + return true + end + +end + +StackLevelTooDeepMeasure.new.registerWithApplication diff --git a/ruby/engine/test/StackLevelTooDeepMeasure/measure.xml b/ruby/engine/test/StackLevelTooDeepMeasure/measure.xml new file mode 100644 index 00000000000..7fe301f57e8 --- /dev/null +++ b/ruby/engine/test/StackLevelTooDeepMeasure/measure.xml @@ -0,0 +1,59 @@ + + + 3.1 + stack_level_too_deep_measure + 22222222-c89b-4b93-b400-220ca060b2bb + b9722f6a-e289-4915-8297-6dad9bf08556 + 2023-11-10T13:43:11Z + 5872977E + StackLevelTooDeepMeasure + Stack Level Too Deep Measure + + The arguments method calls an infinitely recursive function + + + + + Envelope.Opaque + + + + Measure Function + Measure + string + + + Requires EnergyPlus Results + false + boolean + + + Measure Type + ModelMeasure + string + + + Uses SketchUp API + false + boolean + + + Measure Language + Ruby + string + + + + + + OpenStudio + 3.4.1 + 3.4.1 + + measure.rb + rb + script + 9AB518E3 + + + diff --git a/src/measure/Measure.i b/src/measure/Measure.i index 8f8ba27c451..819ebdf546e 100644 --- a/src/measure/Measure.i +++ b/src/measure/Measure.i @@ -117,17 +117,42 @@ VALUE backtracejoin = rb_ary_join(backtrace, rb_str_new2("\n")); const std::string btlines = StringValuePtr(backtracejoin); if (!btlines.empty()) { - loc += "\nTraceback:\n" + btlines; + loc += "\nTraceback (most recent call last):\n" + btlines; } } } if (loc.empty()) { - // In case we couldn't produce the backtrace, fall back on this: + // In case we couldn't produce the backtrace (SystemStackError: stack level too deep), fall back on this: // I just **cannot** figure out a way to get the error location in C, without calling $@.to_s. Seems nothing is available here + // $@ is an array, seems to be already ordered from most recent to older int locationError; - VALUE locval = rb_eval_string_protect("$@.to_s", &locationError); + VALUE btArray = rb_eval_string_protect("$@", &locationError); // "$@.reverse" if (locationError == 0) { - loc = StringValuePtr(locval); + // Get the backing C array of the ruby array + VALUE* elements = RARRAY_PTR(btArray); + long arrayLen = RARRAY_LEN(btArray); + const long back_trace_limit = 20; + loc += "\nTraceback (most recent call last):"; + + if (arrayLen > back_trace_limit) { + long omit = arrayLen - back_trace_limit; + + const long initial_back_trace_limit_when_too_long = 8; + for (long c = arrayLen - 1 ; c > arrayLen - initial_back_trace_limit_when_too_long; --c) { + VALUE entry = elements[c]; + std::string backtrace_line = RSTRING_PTR(entry); + loc += "\n" + std::string(backtrace_line); + } + loc += "\n\t... " + std::to_string(omit) + " levels..."; + + arrayLen = back_trace_limit - initial_back_trace_limit_when_too_long; + } + for (long c = 0; c < arrayLen; ++c) { + VALUE entry = elements[c]; + std::string backtrace_line = RSTRING_PTR(entry); + loc += "\n" + std::string(backtrace_line); + } + } else { loc = "Failed to get VM location"; } From 6dc9b6536794c78f804a385b2e0a0202778706c4 Mon Sep 17 00:00:00 2001 From: Julien Marrec Date: Fri, 10 Nov 2023 15:06:46 +0100 Subject: [PATCH 09/10] Align ruby and python tracebacks --- python/engine/test/PythonEngine_GTest.cpp | 48 ++++++++++++--- .../test/StackLevelTooDeepMeasure/measure.py | 32 ++++++++++ .../test/StackLevelTooDeepMeasure/measure.xml | 59 +++++++++++++++++++ src/measure/Measure.i | 8 +-- 4 files changed, 135 insertions(+), 12 deletions(-) create mode 100644 python/engine/test/StackLevelTooDeepMeasure/measure.py create mode 100644 python/engine/test/StackLevelTooDeepMeasure/measure.xml diff --git a/python/engine/test/PythonEngine_GTest.cpp b/python/engine/test/PythonEngine_GTest.cpp index 21a40908ab0..6c44bf739bb 100644 --- a/python/engine/test/PythonEngine_GTest.cpp +++ b/python/engine/test/PythonEngine_GTest.cpp @@ -51,15 +51,13 @@ TEST_F(PythonEngineFixture, BadMeasure) { ASSERT_EQ(measurePtr->name(), "Bad Measure"); std::string expected_exception = fmt::format(R"(SWIG director method error. In method 'arguments': `ValueError('oops')` -Python traceback follows: -``` + Traceback (most recent call last): File "{0}", line 17, in arguments self.another_method() File "{0}", line 14, in another_method raise ValueError("oops") -ValueError: oops -```)", +ValueError: oops)", scriptPath.generic_string()); openstudio::model::Model model; @@ -84,13 +82,47 @@ TEST_F(PythonEngineFixture, WrongMethodMeasure) { std::string expected_exception = fmt::format(R"(SWIG director method error. In method 'arguments': `AttributeError("'Model' object has no attribute 'nonExistingMethod'")` -Python traceback follows: -``` + Traceback (most recent call last): File "{}", line 14, in arguments model.nonExistingMethod() -AttributeError: 'Model' object has no attribute 'nonExistingMethod' -```)", +AttributeError: 'Model' object has no attribute 'nonExistingMethod')", + scriptPath.generic_string()); + + openstudio::model::Model model; + try { + measurePtr->arguments(model); + ASSERT_FALSE(true) << "Expected measure arguments(model) to throw"; + } catch (std::exception& e) { + std::string error = e.what(); + EXPECT_EQ(expected_exception, error); + } +} + +TEST_F(PythonEngineFixture, StackLevelTooDeepMeasure) { + + const std::string classAndDirName = "StackLevelTooDeepMeasure"; + + const auto scriptPath = getScriptPath(classAndDirName); + auto measureScriptObject = (*thisEngine)->loadMeasure(scriptPath, classAndDirName); + auto* measurePtr = (*thisEngine)->getAs(measureScriptObject); + + ASSERT_EQ(measurePtr->name(), "Stack Level Too Deep Measure"); + + std::string expected_exception = + fmt::format(R"(SWIG director method error. In method 'arguments': `RecursionError('maximum recursion depth exceeded')` + +Traceback (most recent call last): + File "{0}", line 16, in arguments + s(10) + File "{0}", line 6, in s + return s(x) + File "{0}", line 6, in s + return s(x) + File "{0}", line 6, in s + return s(x) + [Previous line repeated 996 more times] +RecursionError: maximum recursion depth exceeded)", scriptPath.generic_string()); openstudio::model::Model model; diff --git a/python/engine/test/StackLevelTooDeepMeasure/measure.py b/python/engine/test/StackLevelTooDeepMeasure/measure.py new file mode 100644 index 00000000000..164eedb84fb --- /dev/null +++ b/python/engine/test/StackLevelTooDeepMeasure/measure.py @@ -0,0 +1,32 @@ +import typing + +import openstudio + +def s(x): + return s(x) + +class StackLevelTooDeepMeasure(openstudio.measure.ModelMeasure): + def name(self): + return "Stack Level Too Deep Measure" + + def modeler_description(self): + return "The arguments method calls an infinitely recursive function" + + def arguments(self, model: typing.Optional[openstudio.model.Model] = None): + s(10) + args = openstudio.measure.OSArgumentVector() + + return args + + def run( + self, + model: openstudio.model.Model, + runner: openstudio.measure.OSRunner, + user_arguments: openstudio.measure.OSArgumentMap, + ): + """ + define what happens when the measure is run + """ + super().run(model, runner, user_arguments) # Do **NOT** remove this line + +StackLevelTooDeepMeasure().registerWithApplication() diff --git a/python/engine/test/StackLevelTooDeepMeasure/measure.xml b/python/engine/test/StackLevelTooDeepMeasure/measure.xml new file mode 100644 index 00000000000..27ce36c5bcc --- /dev/null +++ b/python/engine/test/StackLevelTooDeepMeasure/measure.xml @@ -0,0 +1,59 @@ + + + 3.1 + stack_level_too_deep_measure + 22222222-c89b-4b93-b400-220ca060b2bb + c2e1967b-0672-4e80-84fd-9204e701f10a + 2023-11-10T14:04:04Z + 4EDA3A53 + StackLevelTooDeepMeasure + Stack Level Too Deep Measure + + The arguments method calls an infinitely recursive function + + + + + Envelope.Opaque + + + + Measure Function + Measure + string + + + Requires EnergyPlus Results + false + boolean + + + Measure Type + ModelMeasure + string + + + Uses SketchUp API + false + boolean + + + Measure Language + Python + string + + + + + + OpenStudio + 3.4.1 + 3.4.1 + + measure.py + py + script + C952AC5B + + + diff --git a/src/measure/Measure.i b/src/measure/Measure.i index 819ebdf546e..522cd69bd86 100644 --- a/src/measure/Measure.i +++ b/src/measure/Measure.i @@ -211,14 +211,14 @@ // traceback.format_exception returns a list, so iterate on that if (!pyth_val || !PyList_Check(pyth_val)) { // NOLINT(hicpp-signed-bitwise) - err_msg += "\nIn reportPythonError(), traceback.format_exception did not return a list."; + err_msg += "\ntraceback.format_exception did not return a list."; } else { unsigned long numVals = PyList_Size(pyth_val); if (numVals == 0) { err_msg += "\nNo traceback available"; } else { - err_msg += "\nPython traceback follows:\n```"; - + // err_msg += "\nPython traceback follows:\n```"; + err_msg += '\n'; for (unsigned long itemNum = 0; itemNum < numVals; itemNum++) { PyObject *item = PyList_GetItem(pyth_val, itemNum); if (PyUnicode_Check(item)) { // NOLINT(hicpp-signed-bitwise) -- something inside Python code causes warning @@ -231,7 +231,7 @@ // PyList_GetItem returns a borrowed reference, do not decrement } - err_msg += "\n```"; + // err_msg += "\n```"; } } From ded922f7a507f71ac8e081a25bd2b4d3d8234896 Mon Sep 17 00:00:00 2001 From: Julien Marrec Date: Fri, 10 Nov 2023 15:21:30 +0100 Subject: [PATCH 10/10] MSVC complains Py_ssize_t -> long, just use Py_ssize_t MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit warning C4244: ‘initializing’: conversion from ‘Py_ssize_t’ to ‘unsigned long’, --- src/measure/Measure.i | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/measure/Measure.i b/src/measure/Measure.i index 522cd69bd86..a3cedd922cd 100644 --- a/src/measure/Measure.i +++ b/src/measure/Measure.i @@ -213,13 +213,13 @@ if (!pyth_val || !PyList_Check(pyth_val)) { // NOLINT(hicpp-signed-bitwise) err_msg += "\ntraceback.format_exception did not return a list."; } else { - unsigned long numVals = PyList_Size(pyth_val); + Py_ssize_t numVals = PyList_Size(pyth_val); if (numVals == 0) { err_msg += "\nNo traceback available"; } else { // err_msg += "\nPython traceback follows:\n```"; err_msg += '\n'; - for (unsigned long itemNum = 0; itemNum < numVals; itemNum++) { + for (Py_ssize_t itemNum = 0; itemNum < numVals; itemNum++) { PyObject *item = PyList_GetItem(pyth_val, itemNum); if (PyUnicode_Check(item)) { // NOLINT(hicpp-signed-bitwise) -- something inside Python code causes warning std::string traceback_line = PyUnicode_AsUTF8(item);