Merge pull request #18 from tanakad/feature/improve-error

disktnk · disktnk · commit b463cdc66f61 · 2015-10-30T17:24:23.000+09:00
Return stack trace error
diff --git a/p/_test_pyfunc.py b/p/_test_pyfunc.py
@@ -5,9 +5,13 @@ class negator(object):
     def __call__(self, x):
         return -x
 
+
 def divideByZero(n):
     return n / 0
 
+
 class alwaysFail(object):
     def __call__(self):
         return divideByZero(42)
+
+not_func_attr = 'test'
diff --git a/p/_test_syntax_error.py b/p/_test_syntax_error.py
@@ -0,0 +1,10 @@
+# this python file is not valid, use for only unit test called from goconvey.
+
+
+class SyntaxErrorTest(object):
+
+    def __init__(self):
+        pass
+
+    def test(self, hoge):
+        print self.hoge = hoge
diff --git a/p/pyerror.go b/p/pyerror.go
@@ -128,7 +128,7 @@ func getPyErr() error {
 	mainMsg := strings.TrimSpace(extractLineFromFormattedErrorMessage(formatted, ln-1))
 	syntaxErr := ""
 	nTracebackLines := ln - 1
-	if isSyntaxError(C.PyTuple_GetItem(excInfo.p, 0)) {
+	if isSyntaxError(C.PyTuple_GetItem(excInfo.p, 0)) && ln > 2 {
 		firstLine := extractLineFromFormattedErrorMessage(formatted, ln-3)
 		secondLine := extractLineFromFormattedErrorMessage(formatted, ln-2)
 		syntaxErr = firstLine + secondLine
diff --git a/p/pyfunc.go b/p/pyfunc.go
@@ -5,7 +5,6 @@ package p
 */
 import "C"
 import (
-	"errors"
 	"fmt"
 	"pfi/sensorbee/sensorbee/data"
 	"runtime"
@@ -29,7 +28,8 @@ func invokeDirect(pyObj *C.PyObject, name string, args ...data.Value) (Object, e
 	go func() {
 		defer func() {
 			if r := recover(); r != nil {
-				ch <- &Result{Object{}, fmt.Errorf("cannot call '%v' due to panic: %v", name, r)}
+				ch <- &Result{Object{}, fmt.Errorf(
+					"cannot call '%v' due to panic: %v", name, r)}
 			}
 		}()
 
@@ -40,32 +40,25 @@ func invokeDirect(pyObj *C.PyObject, name string, args ...data.Value) (Object, e
 		var res Object
 		pyFunc, err := getPyFunc(pyObj, name)
 		if err != nil {
-			ch <- &Result{res, fmt.Errorf("%v at '%v'", err.Error(), name)}
+			ch <- &Result{res, fmt.Errorf("fail to get '%v' function: %v", name,
+				err.Error())}
 			return
 		}
 		defer pyFunc.decRef()
 
-		pyArg := C.PyTuple_New(C.Py_ssize_t(len(args)))
-		if pyArg == nil {
-			ch <- &Result{res, getPyErr()}
+		pyArg, err := convertArgsGo2Py(args)
+		if err != nil {
+			ch <- &Result{res, fmt.Errorf(
+				"fail to convert argument in calling '%v' function: %v",
+				name, err.Error())}
 			return
 		}
-		defer C.Py_DecRef(pyArg)
-
-		for i, v := range args {
-			o, err := newPyObj(v)
-			if err != nil {
-				ch <- &Result{res, fmt.Errorf("%v at '%v'", err.Error(), name)}
-				return
-			}
-			// PyTuple object takes over the value's reference, and not need to
-			// decrease reference counter.
-			C.PyTuple_SetItem(pyArg, C.Py_ssize_t(i), o.p)
-		}
+		defer pyArg.decRef()
 
-		ret, err := pyFunc.callObject(Object{p: pyArg})
+		ret, err := pyFunc.callObject(pyArg)
 		if err != nil {
-			ch <- &Result{res, fmt.Errorf("%v in '%v'", err.Error(), name)}
+			ch <- &Result{res, fmt.Errorf("fail to call '%v' function: %v", name,
+				err.Error())}
 			return
 		}
 
@@ -86,7 +79,8 @@ func invoke(pyObj *C.PyObject, name string, args ...data.Value) (data.Value, err
 	go func() {
 		defer func() {
 			if r := recover(); r != nil {
-				ch <- &Result{data.Null{}, fmt.Errorf("cannot call '%v' due to panic: %v", name, r)}
+				ch <- &Result{data.Null{}, fmt.Errorf(
+					"cannot call '%v' due to panic: %v", name, r)}
 			}
 		}()
 
@@ -97,21 +91,25 @@ func invoke(pyObj *C.PyObject, name string, args ...data.Value) (data.Value, err
 		var res data.Value
 		pyFunc, err := getPyFunc(pyObj, name)
 		if err != nil {
-			ch <- &Result{res, fmt.Errorf("%v at '%v'", err.Error(), name)}
+			ch <- &Result{res, fmt.Errorf("fail to get '%v' function: %v", name,
+				err.Error())}
 			return
 		}
 		defer pyFunc.decRef()
 
 		pyArg, err := convertArgsGo2Py(args)
 		if err != nil {
-			ch <- &Result{res, fmt.Errorf("%v at '%v'", err.Error(), name)}
+			ch <- &Result{res, fmt.Errorf(
+				"fail to convert argument in calling '%v' function: %v",
+				name, err.Error())}
 			return
 		}
 		defer pyArg.decRef()
 
 		ret, err := pyFunc.callObject(pyArg)
 		if err != nil {
-			ch <- &Result{res, fmt.Errorf("%v in '%v'", err.Error(), name)}
+			ch <- &Result{res, fmt.Errorf("fail to call '%v' function: %v", name,
+				err.Error())}
 			return
 		}
 		defer ret.decRef()
@@ -131,11 +129,11 @@ func getPyFunc(pyObj *C.PyObject, name string) (ObjectFunc, error) {
 
 	pyFunc := C.PyObject_GetAttrString(pyObj, cFunc)
 	if pyFunc == nil {
-		return ObjectFunc{}, errors.New("cannot load function")
+		return ObjectFunc{}, getPyErr()
 	}
 
 	if ok := C.PyCallable_Check(pyFunc); ok == 0 {
-		return ObjectFunc{}, errors.New("cannot call function")
+		return ObjectFunc{}, fmt.Errorf("'%v' is not callable object", name)
 	}
 
 	return ObjectFunc{Object{p: pyFunc}}, nil
diff --git a/p/pyfunc_test.go b/p/pyfunc_test.go
@@ -7,6 +7,47 @@ import (
 	"testing"
 )
 
+func TestGetPyFuncError(t *testing.T) {
+	Convey("Given an initialized pyfunc test module", t, func() {
+		ImportSysAndAppendPath("")
+
+		mdl, err := LoadModule("_test_pyfunc")
+		So(err, ShouldBeNil)
+		So(mdl, ShouldNotBeNil)
+		Reset(func() {
+			mdl.DecRef()
+		})
+
+		Convey("When func name is not exist", func() {
+			_, err := safePythonCall(func() (Object, error) {
+				o, err := getPyFunc(mdl.p, "not_exit_func")
+				if err != nil {
+					return Object{}, err
+				}
+				// this code is illegal, should not cast ObjectFunc to Object,
+				// but this function is only to confirm error
+				return Object{p: o.p}, nil
+			})
+			So(err, ShouldNotBeNil)
+			So(err.Error(), ShouldContainSubstring, "AttributeError")
+		})
+
+		Convey("When given name does not indicate function attribute", func() {
+			_, err := safePythonCall(func() (Object, error) {
+				o, err := getPyFunc(mdl.p, "not_func_attr")
+				if err != nil {
+					return Object{}, err
+				}
+				// this code is illegal, should not cast ObjectFunc to Object,
+				// but this function is only to confirm error
+				return Object{p: o.p}, nil
+			})
+			So(err, ShouldNotBeNil)
+			So(err.Error(), ShouldContainSubstring, "not callable")
+		})
+	})
+}
+
 func TestPyFunc(t *testing.T) {
 	Convey("Given an initialized pyfunc test module", t, func() {
 		ImportSysAndAppendPath("")
diff --git a/p/pymodule.go b/p/pymodule.go
@@ -32,10 +32,10 @@ func LoadModule(name string) (ObjectModule, error) {
 		state := GILState_Ensure()
 		defer GILState_Release(state)
 
-		pyMdl, err := C.PyImport_ImportModule(cModule)
+		pyMdl := C.PyImport_ImportModule(cModule)
 		if pyMdl == nil {
-			ch <- &Result{ObjectModule{}, fmt.Errorf("cannot load '%v' module: %v",
-				name, err)}
+			ch <- &Result{ObjectModule{}, fmt.Errorf(
+				"fail to load '%v' module: %v", name, getPyErr())}
 			return
 		}
 
@@ -118,8 +118,8 @@ func (m *ObjectModule) GetClass(name string) (ObjectInstance, error) {
 
 		pyInstance := C.PyObject_GetAttrString(m.p, cName)
 		if pyInstance == nil {
-			ch <- &Result{ObjectInstance{}, fmt.Errorf("cannot get '%v' instance",
-				name)}
+			ch <- &Result{ObjectInstance{}, fmt.Errorf(
+				"fail to get '%v' instance: %v", name, getPyErr())}
 			return
 		}
 		ch <- &Result{ObjectInstance{Object{p: pyInstance}}, nil}
diff --git a/p/pymodule_test.go b/p/pymodule_test.go
@@ -6,6 +6,28 @@ import (
 	"testing"
 )
 
+func TestLoadModuleError(t *testing.T) {
+	Convey("Given a python interpreter set up default import path", t, func() {
+		ImportSysAndAppendPath("")
+
+		Convey("When get module with not exist module name", func() {
+			_, err := LoadModule("not_exist_module")
+			Convey("Then an error should be occurred", func() {
+				So(err, ShouldNotBeNil)
+				So(err.Error(), ShouldContainSubstring, "ImportError")
+			})
+		})
+
+		Convey("When get syntax error module", func() {
+			_, err := LoadModule("_test_syntax_error")
+			Convey("Then an error should be occurred", func() {
+				So(err, ShouldNotBeNil)
+				So(err.Error(), ShouldContainSubstring, "SyntaxError")
+			})
+		})
+	})
+}
+
 func TestNewInstanceAndStateness(t *testing.T) {
 	Convey("Given an initialized python module", t, func() {
 
@@ -19,6 +41,7 @@ func TestNewInstanceAndStateness(t *testing.T) {
 			_, err := mdl.NewInstance("NonexistentClass")
 			Convey("Then an error should be occurred", func() {
 				So(err, ShouldNotBeNil)
+				So(err.Error(), ShouldContainSubstring, "AttributeError")
 			})
 		})
 
@@ -115,6 +138,7 @@ func TestNewInstanceAndStateness(t *testing.T) {
 			_, err := mdl.NewInstance("PythonTest", params)
 			Convey("Then an error should be occurred", func() {
 				So(err, ShouldNotBeNil)
+				So(err.Error(), ShouldContainSubstring, "TypeError")
 			})
 		})
 
@@ -158,6 +182,14 @@ func TestNewInstanceAndStateness(t *testing.T) {
 				So(actual, ShouldEqual, "instance_value")
 			})
 		})
+
+		Convey("When get an invalid class instance by GetClass", func() {
+			_, err := mdl.GetClass("NonexistentClass")
+			Convey("Then an error should be occurred", func() {
+				So(err, ShouldNotBeNil)
+				So(err.Error(), ShouldContainSubstring, "AttributeError")
+			})
+		})
 	})
 }
 
