proof of concept of the more type-safe way of defining the predicates. #1073
Conversation
|
Skipping CI for Draft Pull Request. |
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: metlos The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@metlos: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
|
Thanks for the proposal 👍 We can have our own implementation (which can be very simple) and use that in the poll functions. With this implementation of the type Eventually struct {
t *testing.T
errors []assertError
shouldFail bool
}
type assertError struct {
format string
args []interface{}
}
func (e *Eventually) Errorf(format string, args ...interface{}) {
e.errors = append(e.errors, assertError{
format: format,
args: args,
})
}
func (e *Eventually) FailNow() {
e.shouldFail = true
}
// WithNameMatching waits for a single object with the provided name in the namespace of the awaitality that additionally
// matches the provided predicate functions.
func (w *Waiter[T]) WithNameMatching(name string, predicates ...func(*Eventually, T)) (T, error) {
w.t.Logf("waiting for object of GVK '%s' with name '%s' in namespace '%s' to match additional criteria", w.gvk, name, w.await.Namespace)
var returnedObject T
ev := &Eventually{
t: w.t,
}
err := wait.PollUntilContextTimeout(context.TODO(), w.await.RetryInterval, w.await.Timeout, true, func(ctx context.Context) (done bool, err error) {
obj := &unstructured.Unstructured{}
obj.SetGroupVersionKind(w.gvk)
if err := w.await.Client.Get(context.TODO(), client.ObjectKey{Name: name, Namespace: w.await.Namespace}, obj); err != nil {
if apierrors.IsNotFound(err) {
return false, nil
}
return false, err
}
object, err := w.cast(obj)
if err != nil {
return false, fmt.Errorf("failed to cast the object to GVK %v: %w", w.gvk, err)
}
ev.errors = nil
for _, predicate := range predicates {
predicate(ev, object)
}
if len(ev.errors) == 0 {
returnedObject = object
return true, nil
}
if ev.shouldFail {
return false, fmt.Errorf("wait function was stopped")
}
return false, nil
})
if err != nil {
sb := strings.Builder{}
sb.WriteString("couldn't match the object (GVK '%s') called '%s' in namespace '%s' with the criteria")
args := []any{w.gvk, name, w.await.Namespace}
obj := &unstructured.Unstructured{}
obj.SetGroupVersionKind(w.gvk)
if err := w.await.Client.Get(context.TODO(), client.ObjectKey{Name: name, Namespace: w.await.Namespace}, obj); err != nil {
sb.WriteString(" and also failed to retrieve the object at all with error: %s")
args = append(args, err)
w.t.Logf(sb.String(), args...)
} else {
o, _ := w.cast(obj)
sb.WriteString("\n the actual object: \n")
y, _ := StringifyObject(o)
sb.Write(y)
sb.WriteString("\n----\n")
sb.WriteString("erros:\n")
w.t.Logf(sb.String(), args...)
for _, testErr := range ev.errors {
w.t.Errorf(testErr.format, testErr.args...)
}
}
}
return returnedObject, err
}and used it here: toolchain-e2e/testsupport/wait/awaitility.go Lines 421 to 423 in 21e81c0 where instead of comparing the values and returning the boolean, I used assert.Equal: _, err = For(t, a, &corev1.Namespace{}).WithNameMatching(name, func(ev *Eventually, c *corev1.Namespace) {
assert.Equal(ev, ns.Status.Phase, corev1.NamespaceActive)
})When changing it to something that fails _, err = For(t, a, &corev1.Namespace{}).WithNameMatching(name, func(ev *Eventually, c *corev1.Namespace) {
assert.Equal(ev, ns.Status.Phase, "something")
})it fails as expected and also prints the diff created by the assertion function with the whole error trace: With that approach, there is no need for the I believe that this is the way we should go - try to simplify the code and reuse the existing logic & libraries as much as possible. WDYT? |
|
Playing a bit with the API - here is my type-safety fluent API proposal: type Predicate[T client.Object] interface {
Matches(e *Eventually, object T)
}
type ObjectPredicate[Self any, T client.Object] struct {
predicates []func(*Eventually, T)
self *Self
}
func (p *ObjectPredicate[Self, T]) WithLabel(key, value string) *Self {
p.AddPredicate(func(ev *Eventually, c T) {
if c.GetLabels() == nil {
assert.Fail(ev, "labels should not be nil")
return
}
assert.Contains(ev, c.GetLabels(), key)
assert.Equal(ev, value, c.GetLabels()[key])
})
return p.self
}
func (p *ObjectPredicate[Self, T]) WithAnnotation(key, value string) *Self {
p.AddPredicate(func(ev *Eventually, c T) {
if c.GetAnnotations() == nil {
assert.Fail(ev, "annotations should not be nil")
return
}
assert.Contains(ev, c.GetAnnotations(), key)
assert.Equal(ev, value, c.GetAnnotations()[key])
})
return p.self
}
func (p *ObjectPredicate[Self, T]) Matches(e *Eventually, object T) {
for _, match := range p.predicates {
match(e, object)
}
}
func (p *ObjectPredicate[Self, T]) AddPredicate(predicate func(*Eventually, T)) {
p.predicates = append(p.predicates, predicate)
}
type NamespacePreds struct {
*ObjectPredicate[NamespacePreds, *corev1.Namespace]
}
func NsPredicates() *NamespacePreds {
predicate := &NamespacePreds{
ObjectPredicate: &ObjectPredicate[NamespacePreds, *corev1.Namespace]{},
}
predicate.self = predicate
return predicate
}
func (p *NamespacePreds) WithNsActive() *NamespacePreds {
p.AddPredicate(func(ev *Eventually, ns *corev1.Namespace) {
assert.Equal(ev, ns.Status.Phase, corev1.NamespaceActive)
})
return p
}
type SpcPreds struct {
*ObjectPredicate[SpcPreds, *toolchainv1alpha1.SpaceProvisionerConfig]
}
func SpcPredicates() *SpcPreds {
predicate := &SpcPreds{
ObjectPredicate: &ObjectPredicate[SpcPreds, *toolchainv1alpha1.SpaceProvisionerConfig]{},
}
predicate.self = predicate
return predicate
}
func (p *SpcPreds) WithToolchainClusterRef(ref string) *SpcPreds {
p.AddPredicate(func(ev *Eventually, spc *toolchainv1alpha1.SpaceProvisionerConfig) {
assert.Equal(ev, ref, spc.Spec.ToolchainCluster, "toolchain cluster reference should match")
})
return p
}the usage: _, err = For(t, a, &corev1.Namespace{}).WithNameMatching(name, NsPredicates().WithNsActive().WithLabel("some", "thing").WithAnnotation("another", "thing"))
require.NoError(t, err)
_, err = For(t, a, &toolchainv1alpha1.SpaceProvisionerConfig{}).WithNameMatching(a.ClusterName, SpcPredicates().WithLabel("some", "thing").WithToolchainClusterRef("ref"))
require.NoError(t, err)and small modification in the func (w *Waiter[T]) WithNameMatching(name string, predicates ...Predicate[T]) (T, error) {
...
for _, predicate := range predicates {
predicate.Matches(ev, object)
}
... |
…ds to perform tests.
|
I added diffing and a whole bunch of logging to the |
|
…ertion conversion instead of the elaborate setup to compose the assertions together.
metlos
force-pushed
the
new-predicates
branch
2 times, most recently
from
8e4ce18 to
5457ee2
Compare
|
MatousJobanek
left a comment
MatousJobanek
left a comment
There was a problem hiding this comment.
Please read all the comments in the review before you start replying to any of them
| Await(cl). | ||
| WithTimeout(1*time.Second). |
There was a problem hiding this comment.
why not using the already existing awaitility object? It already provides the needed methods to modify the timeout,etc...?
Also, providing the awaitility object (or client) is mandatory, so it should be part of either the initial function or the last one that executes the logic.
There was a problem hiding this comment.
I think of the Await[T client.Object] that is the result value of the Await(client.Client) call as a stripped down version of the Awaitility (that takes along some baggage along with it).
The WithTimeout() call is optional the same way as is Awaitility.WithRetryOptions().
There was a problem hiding this comment.
My point was that you can delegate this to the already available API in the awaitility object (as it is done now) and keep the new API as simple as possible. We don't face any issues with the code-duplication that we want to solve in this PR.
I would focus on providing API for the assertions/criteria, any additional changes can be done later if needed.
| cl := fake.NewClientBuilder().WithScheme(scheme).WithRuntimeObjects(spcUnderTest).Build() | ||
|
|
||
| // this is the new WaitFor | ||
| spaceprovisionerconfig.Asserting(). |
There was a problem hiding this comment.
I would still keep the "Wait", "WaitFor", or "Eventually" somewhere present in the whole API to make it clear that it's waiting and not running the immediate assertions.
There was a problem hiding this comment.
The Asserting part is for immediate assertions. You can do spc.Asserting()...Test(t, obj).
You can convert the immediate assertion into a waiting one by calling spc.Asserting()...Await(cl).... The name Await is meant to suggest that you're going to await an object matching the assertions.
There was a problem hiding this comment.
Ok, I'm confused now. In one of our sync calls we discussed the scope of the API and we agreed on supporting only the "wait" logic/part in toolchain-e2e.
Our goal is to replace the code-duplication in the awaitilities & simplify the code, we don't want to create an API for asserting any object.
In case of toolchain-e2e, the wast majority of all calls are with the wait logic, the immediate assertion is minimal (compared to waits).
| Conditions(conditions.With().Type(toolchainv1aplha1.ConditionReady)). | ||
| Await(cl). | ||
| WithTimeout(1*time.Second). | ||
| Matching(context.TODO(), t) |
There was a problem hiding this comment.
I think that the context can be hidden (either fall back to context.TODO() all over the place, or include a context instance into the awaitility objejct.
There was a problem hiding this comment.
It is my personal preference to have explicit context where ever a cancellable call is being made or at least can be made. I got a confirmation of that preference in controller-runtime 0.15 where they actually broke a lot of user code by changing the API just to be able to pass the context around also in event handler mappers.
There was a problem hiding this comment.
Two things:
- if this was a production code, then I wouldn't say a single word, but it's an e2e tests where we can easily risk any unfinished cleanup
- As you also used in the example above, we would end up with providing
context.TODO()orcontext.Background()all over the place anyway, unless we introduce creating a context listening on exit code for graceful shutdown for every test, which seems to be a bit overkill to me. In other words, why not movingcontext.TODO()down into the API?
| spaceprovisionerconfig.Asserting(). | ||
| ObjectMeta(metadata.With().Name("myobj").Namespace("default").Label("requiredLabel")). |
There was a problem hiding this comment.
in most of the cases, we always provide a name or namespace, so it would be completely fine to include it into the API as mandatory parameters. The rest of the functions can be part of the API, right?
Something like this:
spaceprovisionerconfig.Asserting("myobj", "default").
Label("requiredLabel").
...There was a problem hiding this comment.
I think that's a wrong place to optimize, because that assumption fails if you want to find the first or all objects matching some conditions.
There was a problem hiding this comment.
Good point. IIRC, we have a function called FirstThat, so we could have AssertingFirstThat. I pretty like the format that is in the generic function now.
There was a problem hiding this comment.
Actually, your PR also contains the "First logic" that lists the objects, so it's already there
| spaceprovisionerconfig.Asserting(). | ||
| ObjectMeta(metadata.With().Name("myobj").Namespace("default").Label("requiredLabel")). | ||
| Conditions(conditions.With().Type(toolchainv1aplha1.ConditionReady)). | ||
| Await(cl). | ||
| WithTimeout(1*time.Second). | ||
| Matching(context.TODO(), t) |
There was a problem hiding this comment.
how about something like this:
spaceprovisionerconfig.With(awaitility).
AssertThat("myobj", "default").
Matching().
Label("requiredLabel").
Conditions(spaceprovisionerconfig.IsReady()).
IsEventuallyPresent(t)It's easy to read and it should provide us enough flexibility for adding additional functionalities & object specifics.
we could do:
spaceprovisionerconfig.With(awaitility).
AssertFirstMatching().or
spaceprovisionerconfig.With(awaitility).
AssertThat("myobj", "default").
Matching().
Label("requiredLabel").
Conditions(spaceprovisionerconfig.IsReady()).
IsDeleted(t)There was a problem hiding this comment.
While your proposal reads quite nice, it forgoes the possibility of doing immediate tests. I like that about the API because it opens up the ability to e.g. have two different assertion objects and use them on a list of objects in a single loop, etc. I'm not saying that's super important but I do think that less restrictions is a good thing here, even if we didn't do things that way atm.
As for the readabilty, I was going back and forth between:
spaceprovisionerConfig.Asserting().
Label("requiredLabel").
Name("myObj").
Namespace("default").
Annotation("expectedAnnotation").
Conditions(conditions.With().Type(toolchainv1alpha1.ConditionReady)).
ToolchainClusterName("tc").
...and
spaceprovisionerConfig.Asserting().
ObjectMeta(metadata.With().
Label("requiredLabel").
Name("myObj").
Namespace("default").
Annotation("expectedAnnotation")).
Conditions(conditions.With().
Type(toolchainv1alpha1.ConditionReady).
Message("hey"))
ToolchainClusterName("tc").
...In the end I ended up on the latter because it IMHO better visually distinguishes between the different "parts" of the object. We could go even further with that:
spaceprovisionerconfig.Asserting().
ObjectMeta(metadata.With().
Label("requiredLabel").
Name("myObj").
Namespace("default").
Annotation("expectedAnnotation")).
Spec(spaceprovisionerconfig.Spec().
ToolchainClusterName("tc"))
Status(spaceprovisionconfig.Status().
Conditions(conditions.With().
Type(toolchainv1alpha1.ConditionReady).
Message("hey")))
...Also your Conditions(spaceprovisionerconfig.IsReady()) doesn't look composable. How would you add other requirements on the conditions? conditions.IsReady().WithMessage("") could possibly work, but what if you wanted some different criteria first? like masteruserrecord.UserProvisionedNotificationCreated(true).IsReady().WithMessage("")? How would you be able to swap around the order of the conditions (or leave some out if you didn't need them?)
I'm not saying that my design is perfect in that regard either - conditions are hard :)
There was a problem hiding this comment.
Yes, conditions are hard, and I don't think that we should be trying to solve them here, in this API.
To your comment, I didn't try to make the condition API composable, because this wasn't the problem I focused on. I focused on the wait API only, nothing else.
I would say that implementing composable API for conditions can be completely separate topic, effort, and also PR. This could even go into toolchain-common, or could work together with some changes in the api repository that would abstract all our types to some interface that supports "toolchain conditions".
In other words, accepting the whole condition object in the API is completely sufficient for now.
| type Assertion[T any] interface { | ||
| Test(t AssertT, obj T) | ||
| } | ||
|
|
||
| // Assertions is just a list of assertions provided for convenience. It is meant to be embedded into structs. | ||
| type Assertions[T any] []Assertion[T] | ||
|
|
||
| // AssertionFunc converts a function into an assertion. | ||
| type AssertionFunc[T any] func(t AssertT, obj T) | ||
|
|
||
| // Append is just an alias for Go's built-in append. | ||
| func Append[Type any](assertionList Assertions[Type], assertions ...Assertion[Type]) Assertions[Type] { |
There was a problem hiding this comment.
using any looks to complicate the things a bit more (there is the need to cast the assertions).
I think that we can rely on the fact that this API should be used only for client.Object type - this assumption could simplify the code a bit.
Let's keep in mind that we want to improve the wait functions for k8s resources, we don't want to implement a new assertion library for any object type.
| type Assertion[T any] interface { | |
| Test(t AssertT, obj T) | |
| } | |
| // Assertions is just a list of assertions provided for convenience. It is meant to be embedded into structs. | |
| type Assertions[T any] []Assertion[T] | |
| // AssertionFunc converts a function into an assertion. | |
| type AssertionFunc[T any] func(t AssertT, obj T) | |
| // Append is just an alias for Go's built-in append. | |
| func Append[Type any](assertionList Assertions[Type], assertions ...Assertion[Type]) Assertions[Type] { | |
| type Assertion[T client.Object] interface { | |
| Test(t AssertT, obj T) | |
| } | |
| // Assertions is just a list of assertions provided for convenience. It is meant to be embedded into structs. | |
| type Assertions[T client.Object] []Assertion[T] | |
| // AssertionFunc converts a function into an assertion. | |
| type AssertionFunc[T client.Object] func(t AssertT, obj T) | |
| // Append is just an alias for Go's built-in append. | |
| func Append[Type client.Object](assertionList Assertions[Type], assertions ...Assertion[Type]) Assertions[Type] { |
There was a problem hiding this comment.
I think the restriction to client.Object here is completely arbitrary - the API as it is doesn't need the T to be a client.Object so we shouldn't restrict it.
There was a problem hiding this comment.
I wouldn't say that it's arbitrary. The API is gonna be used for resources from OpenShift clusters and all of them should implement client.Object interface. Restricting the API to this type makes the logic simpler.
| name, found := findNameFromAssertions(f.assertions) | ||
| require.True(t, found, "ObjectNameAssertion not found in the list of assertions but one is required") | ||
|
|
||
| namespace, found := findNamespaceFromAssertions(f.assertions) | ||
| require.True(t, found, "ObjectNamespaceAssertion not found in the list of assertions but one is required") |
There was a problem hiding this comment.
related to the other comments - making name & namespace mandatory would simplify also this part.
| type ConditionAssertions struct { | ||
| assertions.Assertions[[]toolchainv1alpha1.Condition] | ||
| } |
There was a problem hiding this comment.
ok, now I understand why you used the any - it's to support the conditions. Well, we really don't need to store that in the same list of assertions, correct? Each custom-assertion implementation needs to provide the Conditions method anyway, so it's only enabling these implementations calling the API when adding the type-specific assertion function.
There was a problem hiding this comment.
Having the Assertion[T any] instead of Assertion[client.Object] also enables a generic implementation of AppendConverted[From any, To any](conversionFn func(To) (From, bool), assertions[To], assertions[From]) Assertion[To].
This function then enables a nice "traversal" in the spaceprovisionerconfig assertions:
func (spc *SpaceProvisionerConfigAssertions) Conditions(cas *conditions.ConditionAssertions) *SpaceProvisionerConfigAssertions {
spc.Assertions = assertions.AppendConverted(getConditions, spc.Assertions, cas.Assertions...)
return spc
}
func getConditions(spc *toolchainv1alpha1.SpaceProvisionerConfig) ([]toolchainv1alpha1.Condition, bool) {
return spc.Status.Conditions, true
}Of course, we could implement something similar if we had specialized assertions for conditions that wouldn't share anything with the Assertion[T client.Object] but it would be a bit arbitrary.
I like the cohesion that Assertion[T any] enables here.
There was a problem hiding this comment.
As I mentioned above - we already talked about it in one of our sync calls and we agreed on focusing only on the wait logic in toolchain-e2e repo.
We don't want to create a new testing framework, we don't want to solve all the problems of the universe with the new API, we only want to reduce the code-duplication in the wait package and simplify it.
The SPC assertions were one of the first POC proposals to provide a generic API for everything, but we agreed on not expanding that as it brings additional complication and rather focus only on the wait logic instead.
| // CastAssertion can be used to convert a generic assertion on, say, client.Object, into | ||
| // an assertion on a concrete subtype. Note that the conversion is not guaranteed to | ||
| // pass by the type system and can fail at runtime. | ||
| func CastAssertion[SuperType any, Type any](a Assertion[SuperType]) Assertion[Type] { |
There was a problem hiding this comment.
Related to the other comments, by replacing any with client.Object we could technically drop all this conversion logic.
There was a problem hiding this comment.
I think the my comment from above also applies here. The simplistic generics of Go force a lot of this "cruft", too.
This function is only used in AppendGeneric() which incidentally converts the generic assertions on the client.Object.ObjectMeta to T in ObjectAssertions.ObjectMeta(). So something like this would be needed even if we restricted T to client.Object. We could also forgo type safety (as we do with the current incarnation of the predicates used in WaitFor but that lack of type safety was actually one of the drivers for me to have another stab at the assertions.
| ft := &errorCollectingT{logger: t} | ||
|
|
||
| err := kwait.PollUntilContextTimeout(ctx, f.tick, f.timeout, true, func(ctx context.Context) (done bool, err error) { | ||
| t.Helper() | ||
| ft.errors = nil |
There was a problem hiding this comment.
do you know if we could replace this with either assert.Eventually or assert.EventuallyWithT ?
There was a problem hiding this comment.
That's a good point. I'm not sure. We'd need to compare the impl of kwait.PollUntilContextTimeout with assert.Eventually. The main difference I see here is that assert.Eventually doesn't accept a context and therefore is not cancellable. The question is whether that is problem or not. I assume there are also going to be slight behavioral differences between the two - but I haven't looked at that yet.
3 participants
We were recently discussing the state of our assertions across the codebase and came to a couple of conclusions:
wait.Foret al. for all our assertions. This means replacing the large number of ad-hoc assertions inAwaitilityclass as well as a number of*Criterionstructs. We hope this will consolidate the different kinds of reporting employed in various places, will simplify the codebase and reduce code duplication.toolchain-e2e(at least for now). We use the predicates and various utility methods the most in here so converging them will bring the biggest benefits.The problem with the current approach
In this PR I'm trying out a slightly different approach to composing the list of predicates to be used for the test in
wait.For.The main problem I'm trying to solve here is the loss of type safety that we currently suffer when using
This method accepts predicates on any
client.Objectregardless of the type it actually tests. This can lead to subtle bugs in the tests that ideally the compiler would have caught were the method signature more typesafe. The reason for this is the limited type-inference the go compiler is trying to make.We define a couple of generic predicates like
that checks for the presence of certain labels on an object. This by definition can be done on any
client.Object. The reason for the predicate constructor function to look like above and NOT like:is that if we wanted to use such predicate constructor function, we would need to explicitly name the
Ton every use-site - its type would not be inferred. E.g.:Not nice. The solution that currently exists in the codebase is to use "type erasing" function:
so that the above call would read:
Nicer, but at the cost of lost type safety.
The proposed solution
The proposed solution takes inspiration in the
*Assertionclasses that we use in the unit tests that basically compile together all the predicates that can be asserted on given object. It takes the idea of going "down and right", i.e. the properties that should be tested on a given object are represented as functions on the object type's Assertion (we're going "down" by having these chained invocations each on a new line) and these functions accept an assertion object to test the property (we're going "right" by providing the args to the function).E.g.:
In our current codebase, we have a
MasterUserRecordAssertionthat is being used like this:This style executes the tests eagerly, i.e. as soon as the
Exists()function is invoked, the method checks for the existance of the MUR.In the new "framework", this might be expressed as:
While this might not look like much of an improvement, I think there are some important differences that make this worth taking look at:
The new implementation also retains the use of
assertfor testing and care is taken to retain the test line numbers for easy failure identification.I.e. given a test like this:
we'd get output like this in case of the test failure:
You see the exact line in the test that failed and you can also get to the exact assertion that failed by looking at the first line of the error trace. The
context deadline exceededis a telltale sign of this method actually waiting for the object to appear in the cluster instead of just checking once and failing immediately.The impl proposed in here concentrates on the e2e usecase and therefore only implements the waiting methods using "Await(cl)" call-chain but one can imagine we could extend this to use the immediate tests and for example write (again, this doesn't exist in the PR atm):
The proposed implementation strives to support implementation of new assertion objects as easily as possible. It doesn't try to come up with some complex composition of structs but rather provides a number of utility functions to compose the assertions on different types together (the exception to this rule is the "base" struct for testing object assertions that is meant to be embedded in the vast majority of the
*Assertiontypes).E.g. adding support for asserting on conditions of some object is fairly simple (compared to the copypasting and adapting a large(-ish) number of methods in the existing
*Assertionor*Criterionstructs):The
conditions.ConditionAssertionsis a type that can produce assertions on a list of condition objects and as such can be used on any type that has them. All we need to do is to adapt the assertions on the conditions to be assertions on the objects themselves by providing an "accessor" to conditions and usingassertions.AppendConverted(...)to modify the set of assertions on a condition list to become a set of assertions on the SpaceProvisionerConfig.