Switch *testing.T uses to equivalent interface + structured logging

[coryrc]

Working to introduce structured logging to our tests. See #907

This work allows these test functions to be called by objects other
than *testing.T. The t.Error() calls are made compatible with
structured logging (wrapping Zap sugared logger calls) or code using
testing.T.

[coryrc]

/cc @yanweiguo
I'm not sure if the pkg tests are properly testing metricstest; should I be doing another test too?

[coryrc]

@yanweiguo
I ran knative/serving units tests (go test -v -race ./...) with this branch [0]. They passed.
... which, I suppose, doesn't really help validate the change much.

[0] Gopkg.toml change:

 [[override]]
   name = "knative.dev/pkg"
-  branch = "release-0.11"
+  branch = "testingTcompat"
+  source = "github.com/coryrc/pkg"

[coryrc]

/assign @chaodaiG
/assign @yanweiguo
/assign @mattmoor

[chaodaiG]

@yanweiguo
I ran knative/serving units tests (go test -v -race ./...) with this branch [0]. They passed.
... which, I suppose, doesn't really help validate the change much.

[0] Gopkg.toml change:

 [[override]]
   name = "knative.dev/pkg"
-  branch = "release-0.11"
+  branch = "testingTcompat"
+  source = "github.com/coryrc/pkg"

@coryrc , can you create a PR in serving with the same Gopkg.toml change as above, would like to see how it works in serving in action

[yanweiguo]

/cc @yanweiguo
I'm not sure if the pkg tests are properly testing metricstest; should I be doing another test too?

@coryrc Functions in metricstest are used for unit tests in serving or eventing. @chaodaiG's suggestion should work.

[coryrc]

I did as you asked; it passed. (link just above this)

[coryrc]

It doesn't actually exercise any of the calls I made (because I only changed how it prints errors & the tests all pass), but given it's a statically-typed language, I wouldn't expect it to break.

[yanweiguo]

/lgtm
/approve
for metrics dir change.

[chaodaiG]

/approve
So that Cory is unblocked testing this feature out. @coryrc please loop me in when you are working on #907

[coryrc]

/assign @vaikas

[vaikas]

/lgtm
/approve

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chaodaiG, coryrc, vaikas, yanweiguo

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• apis/OWNERS [vaikas]
• metrics/OWNERS [vaikas,yanweiguo]
• test/OWNERS [chaodaiG,vaikas]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[vagababov]

I've just experienced this and I am ready to say that this is horrendously bad UX.

L: TestMetricsReported (0.00s)
    concurrency_reporter_test.go:303: Reporter got wrong number of tags metric request_concurrency got 6 want 5
    concurrency_reporter_test.go:303: Reporter expected a different tag value for key metric request_concurrency key configuration_name got config-real-name want config-rev1
    concurrency_reporter_test.go:303: Reporter got an extra tag metric request_concurrency gotName container_name gotValue activator
    concurrency_reporter_test.go:303: Reporter expected a different tag value for key metric request_concurrency key service_name got service-real-name want service-rev1

is not a useful error reporting.
I never really understood the whole testing.T to test.T refactoring, but now experiencing it firsthand, I think it is an extremely bad way.

We should stop doing this. Formatters exist for a reason
/cc @mattmoor @coryrc @evankanderson

[knative-prow-robot]

@vagababov: GitHub didn't allow me to request PR reviews from the following users: coryrc.

Note that only knative members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

I've just experienced this and I am ready to say that this is horrendously bad UX.

L: TestMetricsReported (0.00s)
   concurrency_reporter_test.go:303: Reporter got wrong number of tags metric request_concurrency got 6 want 5
   concurrency_reporter_test.go:303: Reporter expected a different tag value for key metric request_concurrency key configuration_name got config-real-name want config-rev1
   concurrency_reporter_test.go:303: Reporter got an extra tag metric request_concurrency gotName container_name gotValue activator
   concurrency_reporter_test.go:303: Reporter expected a different tag value for key metric request_concurrency key service_name got service-real-name want service-rev1

is not a useful error reporting.
I never really understood the whole testing.T to test.T refactoring, but now experiencing it firsthand, I think it is an extremely bad way.

We should stop doing this. Formatters exist for a reason
/cc @mattmoor @coryrc @evankanderson

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[vagababov]

For more context, I was writing metrics tests and I simply could not decipher what is wrong there at all.