Repository: knative/pkg
Pull request #1370 (2020-06-02 19:37:07)

Junithelper refactor into a function that can be re-used.
Junithelper was previously just a command, refactoring it as a function so it can be re-used by upstream vendors.

/cc @chizhg @chaodaiG 

knative-prow-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/1370#" title="Author self-approved">peterfeifanchen</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **chizhg**
You can assign the PR to them by writing `/assign @chizhg` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fpkg).

<details open>
Needs approval from an approver in each of these files:

- **[test/OWNERS](https://github.com/knative/pkg/blob/master/test/OWNERS)**
- **[testutils/OWNERS](https://github.com/knative/pkg/blob/master/testutils/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["chizhg"]} -->
knative-metrics-robot: The following is the coverage report on the affected files.
Say `/test pull-knative-pkg-go-coverage` to re-run this coverage report

File | Old Coverage | New Coverage | Delta
---- |:------------:|:------------:|:-----:
[test/junit/junit.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1370/pull-knative-pkg-go-coverage/1267903207326093312/artifacts/line-cov.html#file0) | 74.1% | 92.1% | 18.0

knative-prow-robot: @peterfeifanchen: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-pkg-unit-tests | 154a6bfe77befbf3fe8af7372c7d6256f9bb99e7 | [link](https://prow.knative.dev/view/gcs/knative-prow/pr-logs/pull/knative_pkg/1370/pull-knative-pkg-unit-tests/1267903207330287617) | `/test pull-knative-pkg-unit-tests`

[Full PR test history](https://gubernator.knative.dev/pr/knative_pkg/1370). [Your PR dashboard](https://gubernator.knative.dev/pr/peterfeifanchen).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
chaodaiG: Might be my personal preference, I would prefer
```
- junithelper
  - cmd
    - main.go
  - pkg
    - junit.go
```
peterfeifanchen: /test pull-knative-pkg-unit-tests 
peterfeifanchen: > Might be my personal preference, I would prefer
> 
> ```
> - junithelper
>   - cmd
>     - main.go
>   - pkg
>     - junit.go
> ```

I actually prefer it the other way since when you import you can tell its junit.{something} rather than a very innocuous pkg.{something}
chaodaiG: > I actually prefer it the other way since when you import you can tell its junit.{something} rather than a very innocuous pkg.{something}

Makes sense to me. I still feel it's a bit weird for a child package to import parent package, normally it's the other way around. The previous suggestion could be:
```
- junithelper
  - cmd
    - main.go
  - pkg
    - junit
      - junit.go
```

Alternatively, both on top level:
```
- junithelper
- junit
```

Patch:
diff --git a/controller/controller.go b/controller/controller.go
index 15496b8a9..3981c5820 100644
--- a/controller/controller.go
+++ b/controller/controller.go
@@ -18,6 +18,7 @@ package controller
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"sync"
 	"time"
@@ -448,7 +449,7 @@ func (c *Impl) processNextWorkItem() bool {
 func (c *Impl) handleErr(err error, key types.NamespacedName) {
 	c.logger.Errorw("Reconcile error", zap.Error(err))
 
-	// Re-queue the key if it's an transient error.
+	// Re-queue the key if it's a transient error.
 	// We want to check that the queue is shutting down here
 	// since controller Run might have exited by now (since while this item was
 	// being processed, queue.Len==0).
@@ -483,8 +484,8 @@ func (c *Impl) FilteredGlobalResync(f func(interface{}) bool, si cache.SharedInf
 }
 
 // NewPermanentError returns a new instance of permanentError.
-// Users can wrap an error as permanentError with this in reconcile,
-// when he does not expect the key to get re-queued.
+// Users can wrap an error as permanentError with this in reconcile
+// when they do not expect the key to get re-queued.
 func NewPermanentError(err error) error {
 	return permanentError{e: err}
 }
@@ -495,16 +496,21 @@ type permanentError struct {
 	e error
 }
 
-// IsPermanentError returns true if given error is permanentError
+// IsPermanentError returns true if the given error is a permanentError or
+// wraps a permanentError.
 func IsPermanentError(err error) bool {
-	switch err.(type) {
-	case permanentError:
-		return true
-	default:
-		return false
-	}
+	return errors.Is(err, permanentError{})
+}
+
+// Is implements the Is() interface of error. It returns whether the target
+// error can be treated as equivalent to a permanentError.
+func (permanentError) Is(target error) bool {
+	_, ok := target.(permanentError)
+	return ok
 }
 
+var _ error = permanentError{}
+
 // Error implements the Error() interface of error.
 func (err permanentError) Error() string {
 	if err.e == nil {
@@ -514,6 +520,12 @@ func (err permanentError) Error() string {
 	return err.e.Error()
 }
 
+// Unwrap implements the Unwrap() interface of error. It returns the error
+// wrapped inside permanentError.
+func (err permanentError) Unwrap() error {
+	return err.e
+}
+
 // Informer is the group of methods that a type must implement to be passed to
 // StartInformers.
 type Informer interface {


Test patch:
diff --git a/controller/controller_test.go b/controller/controller_test.go
index 50823858b..3f857c78d 100644
--- a/controller/controller_test.go
+++ b/controller/controller_test.go
@@ -19,6 +19,7 @@ package controller
 import (
 	"context"
 	"errors"
+	"fmt"
 	"sync"
 	"testing"
 	"time"
@@ -865,10 +866,39 @@ func TestStartAndShutdownWithWork(t *testing.T) {
 	checkStats(t, reporter, 1, 0, 1, trueString)
 }
 
+type fakeError struct{}
+
+var _ error = (*fakeError)(nil)
+
+func (*fakeError) Error() string {
+	return "I always error"
+}
+
+func TestPermanentError(t *testing.T) {
+	err := new(fakeError)
+	permErr := NewPermanentError(err)
+	if !IsPermanentError(permErr) {
+		t.Errorf("Expected type %T to be a permanentError", permErr)
+	}
+	if IsPermanentError(err) {
+		t.Errorf("Expected type %T to not be a permanentError", err)
+	}
+
+	wrapPermErr := fmt.Errorf("wrapped: %w", permErr)
+	if !IsPermanentError(wrapPermErr) {
+		t.Error("Expected wrapped permanentError to be equivalent to a permanentError")
+	}
+
+	unwrapErr := new(fakeError)
+	if !errors.As(permErr, &unwrapErr) {
+		t.Errorf("Could not unwrap %T from permanentError", unwrapErr)
+	}
+}
+
 type ErrorReconciler struct{}
 
 func (er *ErrorReconciler) Reconcile(context.Context, string) error {
-	return errors.New("I always error")
+	return new(fakeError)
 }
 
 func TestStartAndShutdownWithErroringWork(t *testing.T) {
@@ -922,8 +952,7 @@ func TestStartAndShutdownWithErroringWork(t *testing.T) {
 type PermanentErrorReconciler struct{}
 
 func (er *PermanentErrorReconciler) Reconcile(context.Context, string) error {
-	err := errors.New("I always error")
-	return NewPermanentError(err)
+	return NewPermanentError(new(fakeError))
 }
 
 func TestStartAndShutdownWithPermanentErroringWork(t *testing.T) {
diff --git a/test/junit/junit.go b/test/junit/junit.go
index 2e53da618..bc69067be 100644
--- a/test/junit/junit.go
+++ b/test/junit/junit.go
@@ -21,6 +21,8 @@ package junit
 import (
 	"encoding/xml"
 	"fmt"
+	"io/ioutil"
+	"log"
 )
 
 // TestStatusEnum is a enum for test result status
@@ -144,3 +146,24 @@ func UnMarshal(buf []byte) (*TestSuites, error) {
 	}
 	return &testSuites, nil
 }
+
+// CreateXMLErrorMsg outputs a junit testsuite, testname and error message to the destination path
+// in XML format
+func CreateXMLErrorMsg(testSuite, testName, errMsg, dest string) {
+	suites := TestSuites{}
+	suite := TestSuite{Name: testSuite}
+	var errP *string
+	if errMsg != "" {
+		errP = &errMsg
+	}
+	suite.AddTestCase(TestCase{
+		Name:    testName,
+		Failure: errP,
+	})
+	suites.AddTestSuite(&suite)
+	contents, err := suites.ToBytes("", "")
+	if err != nil {
+		log.Fatal(err)
+	}
+	ioutil.WriteFile(dest, contents, 0644)
+}
diff --git a/test/junit/junit_test.go b/test/junit/junit_test.go
index c4b1aa081..21cc64dc7 100644
--- a/test/junit/junit_test.go
+++ b/test/junit/junit_test.go
@@ -19,6 +19,9 @@ limitations under the License.
 package junit
 
 import (
+	"io/ioutil"
+	"os"
+	"path"
 	"testing"
 )
 
@@ -148,3 +151,24 @@ func TestAddTestSuite(t *testing.T) {
 		t.Fatalf("Expected 2, actual %d", len(testSuites.Suites))
 	}
 }
+
+func TestCreateXMLErrorMsg(t *testing.T) {
+	testDir := "test_output"
+	os.RemoveAll(testDir) // clean up in case there were stale side-effects from previous runs
+	if err := os.Mkdir(testDir, 0777); err != nil {
+		t.Fatalf("cannot create directory %q", testDir)
+	}
+	defer os.RemoveAll(testDir) // clean up
+	dest := path.Join(testDir, "TestCreateXMLErrorTestFile")
+	CreateXMLErrorMsg("dummySuite", "dummyTest", "dummyError has occured", dest)
+	expected := `<testsuites><testsuite name="dummySuite" time="0" failures="1" tests="1"><testcase name="dummyTest" time="0" classname=""><failure>dummyError has occured</failure><properties></properties></testcase><properties></properties></testsuite></testsuites>`
+
+	got, err := ioutil.ReadFile(dest)
+	if err != nil {
+		t.Fatalf("cannot read %q, error %v", dest, err)
+	}
+
+	if string(got) != expected {
+		t.Fatalf("expected:\n%q\n, got:\n%q", expected, got)
+	}
+}
diff --git a/testutils/junithelper/main.go b/testutils/junithelper/main.go
index 64be657d8..f5f9a833a 100644
--- a/testutils/junithelper/main.go
+++ b/testutils/junithelper/main.go
@@ -18,8 +18,6 @@ package main
 
 import (
 	"flag"
-	"io/ioutil"
-	"log"
 
 	"knative.dev/pkg/test/junit"
 )
@@ -38,20 +36,5 @@ func main() {
 	flag.StringVar(&dest, "dest", "junit_result.xml", "Where junit xml writes to")
 	flag.Parse()
 
-	suites := junit.TestSuites{}
-	suite := junit.TestSuite{Name: suite}
-	var errP *string
-	if errMsg != "" {
-		errP = &errMsg
-	}
-	suite.AddTestCase(junit.TestCase{
-		Name:    name,
-		Failure: errP,
-	})
-	suites.AddTestSuite(&suite)
-	contents, err := suites.ToBytes("", "")
-	if err != nil {
-		log.Fatal(err)
-	}
-	ioutil.WriteFile(dest, contents, 0644)
+	junit.CreateXMLErrorMsg(suite, name, errMsg, dest)
 }