Assertions and predicates for the SpaceProvisionerConfig CRD #357
Conversation
metlos
requested review from
MatousJobanek,
xcoulon,
alexeykazakov,
rajivnathan,
ranakan19,
sbryzak and
mfrancisc
as code owners
metlos
changed the title
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #357 +/- ##
=======================================
Coverage 76.42% 76.42%
=======================================
Files 43 43
Lines 1998 1998
=======================================
Hits 1527 1527
Misses 424 424
Partials 47 47 |
|
Let's add also the options to set the fields in the spec - what you've done in your e2e PR. But you can expand it by adding options to set "enabled" "thresholds" and other fields |
|
my point is to have functions that will help you to create the SPC objects: spc := testSpc.NewSpaceProvisionerConfig("spc", test.HostOperatorNs, "cluster1",
testSpc.Enabled(), testSpc.WithReadyCondition(), testSpc.MaxNumberOfSpaces(1000), testSpc.MaxMemoryUtilizationPercent(85)) |
|
just a side note: I would never ever ask you to move one simple function to toolchain-common 😄 |
|
but, if you feel that you won't use it in the next changes in both repos and you feel that it's not worth moving it here, then just say - it's fine ;-) |
be more flexible by using the option functions.
…tThat function to work on any client.Object. This enables the reuse of the predicates both in the helper functions for unit tests (that can use the AssertThat function) as well as in end-to-end tests (which can use wait.For().FirstThat()). This enables us to reuse the same predicates across the whole testsuite instead of having one set for unit tests and another set for the end-to-end tests.
metlos
changed the title
be more readable with the ability to also use Has as the "verb" in the expression.
|
@MatousJobanek the upload to codecov failed in this PR a couple of times in a row. Would you have an idea what might be causing this? |
Yes, it happens from time to time. Codecov uploads are not very reliable on the codecov side. We just have to keep re-running the failed job to get it through. |
pkg/test/spaceprovisionerconfig/spaceprovisionerconfig_assertions.go
Outdated
Show resolved
Hide resolved
pkg/test/spaceprovisionerconfig/spaceprovisionerconfig_assertions.go
Outdated
Show resolved
Hide resolved
| func (p predicate) Matches(obj *toolchainv1alpha1.SpaceProvisionerConfig) bool { | ||
| return p(obj) | ||
| } |
There was a problem hiding this comment.
btw, there is one disadvantage of using the same type predicate that returns just a boolean at both places - it's hard to understand why the unit test failed - there is no assertion message, that something was expected but the actual value is different
There was a problem hiding this comment.
Anyway, let's not try to solve all the problems of the universe in one PR - let's merge it as it is and let's see how this will evolve and is gonna be used
There was a problem hiding this comment.
That is true... There are 2 ways of addressing it:
- Change
AssertThatto acceptmsgAndArgs ...anyand pass it to the innerassert.Trueso that the caller has some control over the output of the asserts. - Change the
Predicate.Matchesto return(bool, string)where the second parameter could contain a description of why the predicate didn't match. This could then somehow be incorporated intoAssertThatandwait.For().FirstThat().
I will update this PR with the former, but I would wait with the latter until we find we actually need it. WDYT?
…t for better control over the test output.
|
A generic way of declaring predicates that can be used both as assertions (through
AssertThatfunction) and as criterions in e2e tests (inwait.For().FirstThat()introduced in the associated PR codeready-toolchain/toolchain-e2e#883).Additionally, also as an example how to use it, this introduces support functions and predicates for
SpaceProvisionerConfigunit tests inhost-operator(codeready-toolchain/host-operator#963) and e2e tests intoolchain-e2e(codeready-toolchain/toolchain-e2e#883).