chore: Refactor test helpers into reusable ktest library #398
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: justinsb 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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
approved
justinsb
force-pushed
the
create_ktest_help
branch
from
32df8ff to
4b44032
Compare
k8s-ci-robot
added
size/XL
|
Sadly it looks like I can't start using this / remove the old I feel like I'm using modules "wrong", I guess we should be using a separate repo. Even if we were using a separate repo, we would have the same sequence of PRs, I guess. |
|
(This is breaking down #383 into its required commits) |
| @@ -41,6 +41,7 @@ echo "kubectl version is $(kubectl version --client)" | |||
| rm -f go.work go.work.sum | |||
| go work init . | |||
| go work use applylib | |||
| go work use ktest | |||
There was a problem hiding this comment.
This, even more, suggests to me that the workspace file should really be checked into the repo (assuming, which I think is true but haven't double-checked, that all referenced modules live within the same repo). Doing that would, I believe, help with making local development easier, so that one can just clone the repo and have it work both in an editor and with other tooling (e.g. go test) without having to first figure out how to make the modules reference each-other in sane ways.
There was a problem hiding this comment.
Yes, my only concern is that I haven't seen it elsewhere, but given the pain of not having it checked in you have persuaded me and I think it's worth doing. If we find something unexpected, then we can revert it. I'll do a separate PR I guess where we do all this in one (that's the hope, anyway!)
ktest/httprecorder/http_recorder.go
Outdated
| if err != nil { | ||
| panic("failed to read request body") |
There was a problem hiding this comment.
Why not return an error value here, since the signature allows it?
| } | ||
| } | ||
|
|
||
| func resetTimestamp(body string) string { |
There was a problem hiding this comment.
This method replaces LastTransitionTime on conditions, but does not care about other timestamps in the payload (e.g. metadata.creationTimestamp). Should it?
| } | ||
|
|
||
| type RequestLog struct { | ||
| // We have seen dropped logs from concurrent requests |
There was a problem hiding this comment.
| // We have seen dropped logs from concurrent requests | |
| // Avoid risking dropped logs from concurrent requests |
The previous comment makes it a little ambiguous if we've seen dropped logs without this mutex (i.e. the comment explains why we have it) or if we've seen it despite the mutext (implying there might be a concurrency bug somewhere that merits more investigation at some point).
This suggestion is assuming the former is the right interpretation.
ktest/httprecorder/request_log.go
Outdated
| for i := range l.entries { | ||
| request := &l.entries[i].Request | ||
| u := request.URL | ||
| r, err := regexp.Compile(find) |
There was a problem hiding this comment.
This should probably be done once, outside the loop.
ktest/httprecorder/request_log.go
Outdated
| u := request.URL | ||
| r, err := regexp.Compile(find) | ||
| if err != nil { | ||
| klog.Fatalf("failed to compile regex %q: %v", find, err) |
There was a problem hiding this comment.
Can we make this return the error instead?
There was a problem hiding this comment.
It makes it harder to use, but I changed this method to accept a t *testing.T as its first parameter; this shows it should be used from test code, and then we can easily call t.Fatalf (which is probably equivalent to a panic anyway, but now we're guarding it to be used from test code)
This will avoid a circular (module) dependency between mockkubeapiserver and kdp itself.
justinsb
force-pushed
the
create_ktest_help
branch
from
4b44032 to
ea24fc2
Compare
|
Cleaned this up, but let's try adding go.work in #383 |
|
/hold |
k8s-ci-robot
added
the
do-not-merge/hold
|
PR needs rebase. 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-sigs/prow repository. |
k8s-ci-robot
added
the
needs-rebase
|
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
k8s-ci-robot
added
the
lifecycle/stale
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
k8s-ci-robot
added
lifecycle/rotten
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /close |
|
@k8s-triage-robot: Closed this PR. In response to this:
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-sigs/prow repository. |
This will avoid a circular (module) dependency between
mockkubeapiserver and kdp itself.