Merge pull request #5023 from NREL/5018-measure-errors

Fix #5018 - C++ CLI: Calling a non existing method in a measure ends up with a crash and stack trace

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:

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()

python/engine/test/BadMeasure/measure.xml

@@ -0,0 +1,59 @@
+<?xml version="1.0"?>
+<measure>
+  <schema_version>3.1</schema_version>
+  <name>bad_measure</name>
+  <uid>812d3ebf-c89b-4b93-b400-110ca060b2bb</uid>
+  <version_id>25ad8ea8-b28b-4f45-93a6-76f056c28ca8</version_id>
+  <version_modified>2023-11-10T10:47:04Z</version_modified>
+  <xml_checksum>33A29C78</xml_checksum>
+  <class_name>BadMeasure</class_name>
+  <display_name>Bad Measure</display_name>
+  <description></description>
+  <modeler_description>The arguments method calls another_method which does a raise ValueError</modeler_description>
+  <arguments />
+  <outputs />
+  <provenances />
+  <tags>
+    <tag>Envelope.Opaque</tag>
+  </tags>
+  <attributes>
+    <attribute>
+      <name>Measure Function</name>
+      <value>Measure</value>
+      <datatype>string</datatype>
+    </attribute>
+    <attribute>
+      <name>Requires EnergyPlus Results</name>
+      <value>false</value>
+      <datatype>boolean</datatype>
+    </attribute>
+    <attribute>
+      <name>Measure Type</name>
+      <value>ModelMeasure</value>
+      <datatype>string</datatype>
+    </attribute>
+    <attribute>
+      <name>Measure Language</name>
+      <value>Python</value>
+      <datatype>string</datatype>
+    </attribute>
+    <attribute>
+      <name>Uses SketchUp API</name>
+      <value>false</value>
+      <datatype>boolean</datatype>
+    </attribute>
+  </attributes>
+  <files>
+    <file>
+      <version>
+        <software_program>OpenStudio</software_program>
+        <identifier>3.4.1</identifier>
+        <min_compatible>3.4.1</min_compatible>
+      </version>
+      <filename>measure.py</filename>
+      <filetype>py</filetype>
+      <usage_type>script</usage_type>
+      <checksum>E787E0E0</checksum>
+    </file>
+  </files>
+</measure>

python/engine/test/PythonEngine_GTest.cpp

@@ -0,0 +1,136 @@
+/***********************************************************************************************************************
+*  OpenStudio(R), Copyright (c) Alliance for Sustainable Energy, LLC.
+*  See also https://openstudio.net/license
+***********************************************************************************************************************/
+
+#include <gtest/gtest.h>
+
+#include "measure/ModelMeasure.hpp"
+#include "measure/OSArgument.hpp"
+#include "measure/OSMeasure.hpp"
+#include "model/Model.hpp"
+#include "scriptengine/ScriptEngine.hpp"
+
+#include <fmt/format.h>
+
+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<std::string> args;
+    thisEngine = std::make_unique<openstudio::ScriptEngineInstance>("pythonengine", args);
+
+    (*thisEngine)->registerType<openstudio::measure::OSMeasure*>("openstudio::measure::OSMeasure *");
+    (*thisEngine)->registerType<openstudio::measure::ModelMeasure*>("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<openstudio::ScriptEngineInstance> 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<openstudio::measure::ModelMeasure*>(measureScriptObject);
+
+  ASSERT_EQ(measurePtr->name(), "Bad Measure");
+
+  std::string expected_exception = fmt::format(R"(SWIG director method error. In method 'arguments': `ValueError('oops')`
+
+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<openstudio::measure::ModelMeasure*>(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'")`
+
+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);
+  }
+}
+
+TEST_F(PythonEngineFixture, StackLevelTooDeepMeasure) {
+
+  const std::string classAndDirName = "StackLevelTooDeepMeasure";
+
+  const auto scriptPath = getScriptPath(classAndDirName);
+  auto measureScriptObject = (*thisEngine)->loadMeasure(scriptPath, classAndDirName);
+  auto* measurePtr = (*thisEngine)->getAs<openstudio::measure::ModelMeasure*>(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;
+  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);
+  }
+}

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()

python/engine/test/StackLevelTooDeepMeasure/measure.xml

@@ -0,0 +1,59 @@
+<?xml version="1.0"?>
+<measure>
+  <schema_version>3.1</schema_version>
+  <name>stack_level_too_deep_measure</name>
+  <uid>22222222-c89b-4b93-b400-220ca060b2bb</uid>
+  <version_id>c2e1967b-0672-4e80-84fd-9204e701f10a</version_id>
+  <version_modified>2023-11-10T14:04:04Z</version_modified>
+  <xml_checksum>4EDA3A53</xml_checksum>
+  <class_name>StackLevelTooDeepMeasure</class_name>
+  <display_name>Stack Level Too Deep Measure</display_name>
+  <description></description>
+  <modeler_description>The arguments method calls an infinitely recursive function</modeler_description>
+  <arguments />
+  <outputs />
+  <provenances />
+  <tags>
+    <tag>Envelope.Opaque</tag>
+  </tags>
+  <attributes>
+    <attribute>
+      <name>Measure Function</name>
+      <value>Measure</value>
+      <datatype>string</datatype>
+    </attribute>
+    <attribute>
+      <name>Requires EnergyPlus Results</name>
+      <value>false</value>
+      <datatype>boolean</datatype>
+    </attribute>
+    <attribute>
+      <name>Measure Type</name>
+      <value>ModelMeasure</value>
+      <datatype>string</datatype>
+    </attribute>
+    <attribute>
+      <name>Uses SketchUp API</name>
+      <value>false</value>
+      <datatype>boolean</datatype>
+    </attribute>
+    <attribute>
+      <name>Measure Language</name>
+      <value>Python</value>
+      <datatype>string</datatype>
+    </attribute>
+  </attributes>
+  <files>
+    <file>
+      <version>
+        <software_program>OpenStudio</software_program>
+        <identifier>3.4.1</identifier>
+        <min_compatible>3.4.1</min_compatible>
+      </version>
+      <filename>measure.py</filename>
+      <filetype>py</filetype>
+      <usage_type>script</usage_type>
+      <checksum>C952AC5B</checksum>
+    </file>
+  </files>
+</measure>

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()