Add tests #111
Add tests #111
Conversation
There was a problem hiding this comment.
The test interface in this case seems a bit odd. It seems like you are asserting that the 3 underlying error events were consumed within a space of 5 seconds. What might be better is if you fed a real event (i.e. object with the right name) + 3 error events into the channel (real event last), and asserted that the real event was the only object received from the channel, within some threshold x of how long we expect events through the channel to take.
Given that the issue that this is fixing was related to an edge case with an early return from the go-routine, it might be good to also test the various edge cases of that function (i.e. valid object but not the right name, valid object with the right name, structured error event, object with meta.Accessor failing to return) and make sure that none of them close the channel returned by WatchNames.
|
@MbolotSuse testing that regular objects sounds much better to me. I have made this change and am also confirming that no objects of event type error are coming through. I think after that change more is being tested and most of what you mentioned is being addressed, however we are focusing on testing this case versus backfilling tests as we are expediting the PR it tests. Also, it was already testing whether the channel is being closed https://github.com/rancher/steve/pull/111/files#diff-d5098a956c2e5f40d45e7c6acc86b5a12f5d8047d828bcca818e1d142322b50dR83. |
| c = watch.NewFakeWithChanSize(5, true) | ||
| defer c.Stop() | ||
| errMsgsToSend := []string{"err1", "err2", "err3"} | ||
| c.Add(&v1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "testsecret1"}}) |
There was a problem hiding this comment.
Nit:
Both testsecret1 and testsecret2 are part of the names we are watching. Would also be good to feed an event through with a name that we aren't watching for, to make sure WatchName doesn't return that object.
In addition, it would be good to return an object which errors when you call meta.Accessor() on it, to cover that case.
Marking as a nit due to priority, timeline, and that these are backfill cases.
Signed-off-by: Silvio Moioli <silvio@moioli.net>
* bump lasso to include pull #111 Signed-off-by: Silvio Moioli <silvio@moioli.net> * Make IsListWatchable public to be reused in other packages Signed-off-by: Silvio Moioli <silvio@moioli.net> * Let lasso know whether a type is watchable upon requesting a cache Signed-off-by: Silvio Moioli <silvio@moioli.net> * Adapt existing tests Signed-off-by: Silvio Moioli <silvio@moioli.net> * Add a test to check watchability is detected correctly Signed-off-by: Silvio Moioli <silvio@moioli.net> --------- Signed-off-by: Silvio Moioli <silvio@moioli.net>
* bump lasso to include pull rancher#111 Signed-off-by: Silvio Moioli <silvio@moioli.net> * Make IsListWatchable public to be reused in other packages Signed-off-by: Silvio Moioli <silvio@moioli.net> * Let lasso know whether a type is watchable upon requesting a cache Signed-off-by: Silvio Moioli <silvio@moioli.net> * Adapt existing tests Signed-off-by: Silvio Moioli <silvio@moioli.net> * Add a test to check watchability is detected correctly Signed-off-by: Silvio Moioli <silvio@moioli.net> --------- Signed-off-by: Silvio Moioli <silvio@moioli.net>
* bump lasso to include pull rancher#111 Signed-off-by: Silvio Moioli <silvio@moioli.net> * Make IsListWatchable public to be reused in other packages Signed-off-by: Silvio Moioli <silvio@moioli.net> * Let lasso know whether a type is watchable upon requesting a cache Signed-off-by: Silvio Moioli <silvio@moioli.net> * Adapt existing tests Signed-off-by: Silvio Moioli <silvio@moioli.net> * Add a test to check watchability is detected correctly Signed-off-by: Silvio Moioli <silvio@moioli.net> --------- Signed-off-by: Silvio Moioli <silvio@moioli.net>
This test is expected to fail until #110 is merged.