squakez commented on code in PR #5503:
URL: https://github.com/apache/camel-k/pull/5503#discussion_r1602756276
##########
e2e/common/traits/health_test.go:
##########
@@ -482,13 +472,15 @@ func TestHealthTrait(t *testing.T) {
WithTransform(IntegrationConditionReason,
Equal(v1.IntegrationConditionDeploymentReadyReason)),
WithTransform(IntegrationConditionMessage,
Equal("1/1 ready replicas"))))
- Satisfy(func(is *v1.IntegrationSpec) bool {
- if *is.Traits.Health.Enabled == true &&
*is.Traits.Health.StartupProbeEnabled == true &&
is.Traits.Health.StartupTimeout == 60 {
- return true
- }
- return false
- })
-
+ g.Expect(IntegrationTraitSpec(t, ctx, ns,
name)()).Should(
Review Comment:
This can be removed. It's more part of the unit test and it's already
covered.
##########
e2e/common/traits/health_test.go:
##########
@@ -363,9 +362,7 @@ func TestHealthTrait(t *testing.T) {
name := RandomizedSuffixName("never-ready")
g.Expect(KamelRunWithID(t, ctx, operatorID, ns,
"files/NeverReady.java", "--name", name, "-t", "health.enabled=true",
- // TODO remove these workaround properties when
https://issues.apache.org/jira/browse/CAMEL-20244 is fixed
- "-p",
"camel.route-controller.unhealthyOnRestarting=true",
- "-p",
"camel.route-controller.unhealthyOnExhausted=true",
+ "-p", "camel.health.routesEnabled=false",
Review Comment:
I think this is defeating the goal of the test. The idea is that the health
is enabled on Camel side and we use those endpoints to query the route about
it's health condition. I am not sure if I am understanding why this option is
needed.
##########
e2e/common/traits/health_test.go:
##########
@@ -439,7 +437,7 @@ func TestHealthTrait(t *testing.T) {
var r *v1.HealthCheckResponse
for h := range c.Pods[0].Health {
- if c.Pods[0].Health[h].Name ==
"camel-routes" && c.Pods[0].Health[h].Status == "DOWN" {
+ if c.Pods[0].Health[h].Name ==
"never-ready" && c.Pods[0].Health[h].Status == "DOWN" {
Review Comment:
This one look weird. If it is expected the Integration name, then, a
randomized name variable `name` should be provided instead. Are we sure this
condition is really existing?
##########
e2e/support/test_support.go:
##########
@@ -1079,6 +1079,16 @@ func IntegrationTraitProfile(t *testing.T, ctx
context.Context, ns string, name
}
}
+func IntegrationTraitSpec(t *testing.T, ctx context.Context, ns string, name
string) func() *v1.Traits {
Review Comment:
I'd remove this as well as it makes sense in unit test. In E2E we want to
test the behavior instead.
##########
e2e/common/traits/health_test.go:
##########
@@ -409,14 +406,15 @@ func TestHealthTrait(t *testing.T) {
return false
}
- return data["check.kind"].(string) ==
"READINESS" && data["route.status"].(string) == "Stopped" &&
data["route.id"].(string) == "never-ready"
+ return r.Status ==
v1.HealthCheckStatusDown && data["check.kind"].(string) == "READINESS"
}))
})
t.Run("Startup condition with never ready route", func(t
*testing.T) {
name :=
RandomizedSuffixName("startup-probe-never-ready-route")
- g.Expect(KamelRunWithID(t, ctx, operatorID, ns,
"files/NeverReady.java", "--name", name, "-t",
"health.startup-probe-enabled=true", "-t",
"health.startup-timeout=60").Execute()).To(Succeed())
+ g.Expect(KamelRunWithID(t, ctx, operatorID, ns,
"files/NeverReady.java", "--name", name,
+ "-t", "health.enabled=true", "-t",
"health.startup-probe-enabled=true", "-t",
"health.startup-timeout=60").Execute()).To(Succeed())
Review Comment:
I think `health.enabled` is superfluous if any probe configuration is
provided.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
