Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test fixes #5415

Merged
merged 6 commits into from
May 8, 2024
Merged

Test fixes #5415

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
443e7ad
chore(tests): bumped tests timeouts
valdar Apr 22, 2024
6d837f5
chore(tests): made build timeout configurable in tests and defaulted …
valdar May 8, 2024
cd88d14
fix(tests): Chang assertions to adhere with Eventually() gomega requi…
valdar Apr 22, 2024
9c65f0f
fix(tests): Change CopyIntegrationKits test support function to prope…
valdar Apr 22, 2024
ecf289a
chore(pkg/cmd/run.go): refactor usag of util.StringSliceContainsAnyOf…
valdar Feb 15, 2024
9ec6cfd
fix(pkg/util/kubernetes/portforward.go): corner case scenario in port…
valdar Feb 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion e2e/advanced/build_order_strategy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ func TestRunBuildOrderStrategyMatchingDependencies(t *testing.T) {
WithNewTestNamespace(t, func(ctx context.Context, g *WithT, ns string) {
operatorID := "camel-k-build-order-deps"
g.Expect(CopyCamelCatalog(t, ctx, ns, operatorID)).To(Succeed())
g.Expect(CopyIntegrationKits(t, ctx, ns, operatorID)).To(Succeed())
g.Expect(KamelInstallWithID(t, ctx, operatorID, ns, "--max-running-pipelines", "4", "--build-order-strategy", string(v1.BuildOrderStrategyDependencies))).To(Succeed())
g.Eventually(PlatformPhase(t, ctx, ns), TestTimeoutMedium).Should(Equal(v1.IntegrationPlatformPhaseReady))

Expand Down
3 changes: 1 addition & 2 deletions e2e/advanced/builder_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@ func TestBuilderTimeout(t *testing.T) {
WithNewTestNamespace(t, func(ctx context.Context, g *WithT, ns string) {
operatorID := fmt.Sprintf("camel-k-%s", ns)
g.Expect(CopyCamelCatalog(t, ctx, ns, operatorID)).To(Succeed())
g.Expect(CopyIntegrationKits(t, ctx, ns, operatorID)).To(Succeed())
g.Expect(KamelInstallWithID(t, ctx, operatorID, ns)).To(Succeed())
g.Eventually(OperatorPod(t, ctx, ns)).ShouldNot(BeNil())
g.Eventually(Platform(t, ctx, ns)).ShouldNot(BeNil())
Expand Down Expand Up @@ -79,7 +78,7 @@ func TestBuilderTimeout(t *testing.T) {
// After a few minutes (5 max retries), this has to be in error state
g.Eventually(BuildPhase(t, ctx, ns, integrationKitName), TestTimeoutMedium).Should(Equal(v1.BuildPhaseError))
g.Eventually(IntegrationPhase(t, ctx, ns, name), TestTimeoutMedium).Should(Equal(v1.IntegrationPhaseError))
g.Eventually(BuildFailureRecovery(t, ctx, ns, integrationKitName), TestTimeoutMedium).Should(Equal(5))
g.Eventually(BuildFailureRecoveryAttempt(t, ctx, ns, integrationKitName), TestTimeoutMedium).Should(Equal(5))
g.Eventually(BuilderPodPhase(t, ctx, ns, builderKitName), TestTimeoutMedium).Should(Equal(corev1.PodFailed))
})
})
Expand Down
5 changes: 5 additions & 0 deletions e2e/advanced/debug_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ func TestKamelCLIDebug(t *testing.T) {

g.Eventually(portIsInUse("127.0.0.1", "5005"), TestTimeoutMedium, 5*time.Second).Should(BeTrue())
g.Expect(Kamel(t, ctx, "delete", "--all", "-n", ns).Execute()).To(Succeed())
g.Eventually(IntegrationPods(t, ctx, ns, "yaml"), TestTimeoutMedium, 5*time.Second).Should(HaveLen(0))
})

t.Run("debug local port check", func(t *testing.T) {
Expand All @@ -71,6 +72,7 @@ func TestKamelCLIDebug(t *testing.T) {

g.Eventually(portIsInUse("127.0.0.1", "5006"), TestTimeoutMedium, 5*time.Second).Should(BeTrue())
g.Expect(Kamel(t, ctx, "delete", "--all", "-n", ns).Execute()).To(Succeed())
g.Eventually(IntegrationPods(t, ctx, ns, "yaml"), TestTimeoutMedium, 5*time.Second).Should(HaveLen(0))
})

t.Run("debug logs check", func(t *testing.T) {
Expand All @@ -83,6 +85,7 @@ func TestKamelCLIDebug(t *testing.T) {

g.Eventually(IntegrationLogs(t, ctx, ns, "yaml"), TestTimeoutMedium).Should(ContainSubstring("Listening for transport dt_socket at address: 5005"))
g.Expect(Kamel(t, ctx, "delete", "--all", "-n", ns).Execute()).To(Succeed())
g.Eventually(IntegrationPods(t, ctx, ns, "yaml"), TestTimeoutMedium, 5*time.Second).Should(HaveLen(0))
})

t.Run("Pod config test", func(t *testing.T) {
Expand All @@ -98,9 +101,11 @@ func TestKamelCLIDebug(t *testing.T) {
}).Should(ContainSubstring("-agentlib:jdwp=transport=dt_socket,server=y,suspend=y,address=*:5005"))
g.Expect(IntegrationPod(t, ctx, ns, "yaml")().GetLabels()["camel.apache.org/debug"]).To(Not(BeNil()))
g.Expect(Kamel(t, ctx, "delete", "--all", "-n", ns).Execute()).To(Succeed())
g.Eventually(IntegrationPods(t, ctx, ns, "yaml"), TestTimeoutMedium, 5*time.Second).Should(HaveLen(0))
})

g.Expect(Kamel(t, ctx, "delete", "--all", "-n", ns).Execute()).To(Succeed())
g.Eventually(IntegrationPods(t, ctx, ns, "yaml"), TestTimeoutMedium, 5*time.Second).Should(HaveLen(0))
})
}

Expand Down
1 change: 0 additions & 1 deletion e2e/advanced/operator_id_filtering_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@ func TestOperatorIDFiltering(t *testing.T) {
WithNewTestNamespace(t, func(ctx context.Context, g *WithT, nsop1 string) {
operator1 := "operator-1"
g.Expect(CopyCamelCatalog(t, ctx, nsop1, operator1)).To(Succeed())
g.Expect(CopyIntegrationKits(t, ctx, nsop1, operator1)).To(Succeed())
g.Expect(KamelInstallWithIDAndKameletCatalog(t, ctx, operator1, nsop1, "--global", "--force")).To(Succeed())
g.Eventually(PlatformPhase(t, ctx, nsop1), TestTimeoutMedium).Should(Equal(v1.IntegrationPlatformPhaseReady))

Expand Down
14 changes: 5 additions & 9 deletions e2e/common/misc/integration_fail_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,9 +86,8 @@ func TestBadRouteIntegration(t *testing.T) {
integrationKitNamespace := IntegrationKitNamespace(t, ctx, ns, name)()
g.Eventually(KitPhase(t, ctx, integrationKitNamespace, kitName), TestTimeoutShort).Should(Equal(v1.IntegrationKitPhaseError))
//Build in error with 5 attempts
build := Build(t, ctx, integrationKitNamespace, kitName)()
g.Eventually(build.Status.Phase, TestTimeoutShort).Should(Equal(v1.BuildPhaseError))
g.Eventually(build.Status.Failure.Recovery.Attempt, TestTimeoutShort).Should(Equal(5))
g.Eventually(BuildPhase(t, ctx, integrationKitNamespace, kitName), TestTimeoutShort).Should(Equal(v1.BuildPhaseError))
g.Eventually(BuildFailureRecoveryAttempt(t, ctx, integrationKitNamespace, kitName), TestTimeoutShort).Should(Equal(5))

// Fixing the route should reconcile the Integration
g.Expect(KamelRunWithID(t, ctx, operatorID, ns, "files/Java.java", "--name", name).Execute()).To(Succeed())
Expand All @@ -99,8 +98,7 @@ func TestBadRouteIntegration(t *testing.T) {
g.Eventually(KitPhase(t, ctx, integrationKitRecoveryNamespace, kitRecoveryName), TestTimeoutShort).Should(Equal(v1.IntegrationKitPhaseReady))
g.Expect(kitRecoveryName).NotTo(Equal(kitName))
// New Build success
buildRecovery := Build(t, ctx, integrationKitRecoveryNamespace, kitRecoveryName)()
g.Eventually(buildRecovery.Status.Phase, TestTimeoutShort).Should(Equal(v1.BuildPhaseSucceeded))
g.Eventually(BuildPhase(t, ctx, integrationKitRecoveryNamespace, kitRecoveryName), TestTimeoutShort).Should(Equal(v1.BuildPhaseSucceeded))

})

Expand Down Expand Up @@ -129,8 +127,7 @@ func TestBadRouteIntegration(t *testing.T) {
integrationKitRecoveryNamespace := IntegrationKitNamespace(t, ctx, ns, name)()
g.Eventually(KitPhase(t, ctx, integrationKitRecoveryNamespace, kitRecoveryName), TestTimeoutShort).Should(Equal(v1.IntegrationKitPhaseReady))
// New Build success
buildRecovery := Build(t, ctx, integrationKitRecoveryNamespace, kitRecoveryName)()
g.Eventually(buildRecovery.Status.Phase, TestTimeoutShort).Should(Equal(v1.BuildPhaseSucceeded))
g.Eventually(BuildPhase(t, ctx, integrationKitRecoveryNamespace, kitRecoveryName), TestTimeoutShort).Should(Equal(v1.BuildPhaseSucceeded))
})

t.Run("run unresolvable component java route", func(t *testing.T) {
Expand Down Expand Up @@ -158,8 +155,7 @@ func TestBadRouteIntegration(t *testing.T) {
integrationKitRecoveryNamespace := IntegrationKitNamespace(t, ctx, ns, name)()
g.Eventually(KitPhase(t, ctx, integrationKitRecoveryNamespace, kitRecoveryName), TestTimeoutShort).Should(Equal(v1.IntegrationKitPhaseReady))
// New Build success
buildRecovery := Build(t, ctx, integrationKitRecoveryNamespace, kitRecoveryName)()
g.Eventually(buildRecovery.Status.Phase, TestTimeoutShort).Should(Equal(v1.BuildPhaseSucceeded))
g.Eventually(BuildPhase(t, ctx, integrationKitRecoveryNamespace, kitRecoveryName), TestTimeoutShort).Should(Equal(v1.BuildPhaseSucceeded))
})

t.Run("run invalid java route", func(t *testing.T) {
Expand Down
33 changes: 20 additions & 13 deletions e2e/support/test_support.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ import (
"github.com/apache/camel-k/v2/pkg/cmd"
"github.com/apache/camel-k/v2/pkg/install"
"github.com/apache/camel-k/v2/pkg/platform"
pkgutil "github.com/apache/camel-k/v2/pkg/util"
v2util "github.com/apache/camel-k/v2/pkg/util"
"github.com/apache/camel-k/v2/pkg/util/defaults"
"github.com/apache/camel-k/v2/pkg/util/kubernetes"
Expand Down Expand Up @@ -128,9 +129,9 @@ const ExpectedOSClusterRoles = 1

var TestDefaultNamespace = "default"

var TestTimeoutShort = 1 * time.Minute
var TestTimeoutMedium = 5 * time.Minute
var TestTimeoutLong = 15 * time.Minute
var TestTimeoutShort = 5 * time.Minute
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these values are too high. If we need to change them specifically for certain tests, then, we should change the test and include the specific timeout instead.

Copy link
Member Author

@valdar valdar Apr 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to my empirical tests these are the values that minimize test runs failing due to small perturbations in load or network speed. Honestly I don't see may cons in bumping them, the only one is that actually failing tests will take longer. But that is a condition that should happens rarely since most of the runs should be of passing tests.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's exactly the problem. In case of an unstable change we may introduce, the time to complete a test would be at least duplicated, having to wait for a feedback too much time. Imho, the goal should be to have faster timeouts. If some test is not stable, then, I don't think the culprit is the timeout. And if it's the timeout, likely it's in the specific flaky test.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see both of you having a point here. IMO if the increased timeout really fixes the flaky tests I tend to accept the fact that we need to wait some more time when a test really is broken by the changes in a PR. The feedback loop for a successful test is over 60 minutes anyways and having a flaky test can be really exhausting.

var TestTimeoutMedium = 20 * time.Minute
var TestTimeoutLong = 30 * time.Minute

// TestTimeoutVeryLong should be used only for testing native builds.
var TestTimeoutVeryLong = 60 * time.Minute
Expand Down Expand Up @@ -283,6 +284,17 @@ func kamelInstallWithContext(t *testing.T, ctx context.Context, operatorID strin

installArgs = []string{"install", "-n", namespace, "--operator-id", operatorID, "--skip-cluster-setup"}

if !pkgutil.StringSliceExists(args, "--build-timeout") {
//if --build-timeout is not explicitly passed as an argument, try to configure it
buildTimeout := os.Getenv("CAMEL_K_TEST_BUILD_TIMEOUT")
if buildTimeout == "" {
//default Build Timeout for tests
buildTimeout = "10m"
}
fmt.Printf("Setting build timeout to %s\n", buildTimeout)
installArgs = append(installArgs, "--build-timeout", buildTimeout)
}

if skipKameletCatalog {
installArgs = append(installArgs, "--skip-default-kamelets-setup")
}
Expand Down Expand Up @@ -1935,7 +1947,7 @@ func BuildCondition(t *testing.T, ctx context.Context, ns string, name string, c
}
}

func BuildFailureRecovery(t *testing.T, ctx context.Context, ns, name string) func() int {
func BuildFailureRecoveryAttempt(t *testing.T, ctx context.Context, ns, name string) func() int {
return func() int {
build := Build(t, ctx, ns, name)()
if build != nil {
Expand Down Expand Up @@ -2011,22 +2023,17 @@ func CopyIntegrationKits(t *testing.T, ctx context.Context, ns, operatorID strin
failTest(t, err)
}
for _, kit := range lst.Items {
if kit.Status.Image != "" {
if kit.Status.Image != "" && kit.Status.Phase == v1.IntegrationKitPhaseReady {
copyKit := v1.IntegrationKit{
ObjectMeta: metav1.ObjectMeta{
Namespace: ns,
Name: kit.Name,
Labels: kit.Labels,
},
Spec: *kit.Spec.DeepCopy(),
Spec: *kit.Spec.DeepCopy(),
Status: *kit.Status.DeepCopy(),
}

if copyKit.Labels == nil {
copyKit.Labels = make(map[string]string)
}

copyKit.Labels[v1.IntegrationKitTypeLabel] = v1.IntegrationKitTypeExternal
copyKit.Spec.Image = kit.Status.Image

v1.SetAnnotation(&copyKit.ObjectMeta, v1.OperatorIDAnnotation, operatorID)
fmt.Printf("Copy integration kit %s from namespace %s\n", kit.Name, opns)
if err := CreateIntegrationKit(t, ctx, &copyKit)(); err != nil {
Expand Down
12 changes: 1 addition & 11 deletions pkg/cmd/run.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -876,7 +876,7 @@ func (o *runCmdOptions) applyDependencies(cmd *cobra.Command, c client.Client, i

func (o *runCmdOptions) getPlatform(cmd *cobra.Command, c client.Client, it *v1.Integration) (*v1.IntegrationPlatform, error) {
// let's also enable the registry trait if not explicitly disabled
if !contains(o.Traits, "registry.enabled=false") {
if !util.StringSliceContainsAnyOf(o.Traits, "registry.enabled=false") {
o.Traits = append(o.Traits, "registry.enabled=true")
}
pl, err := platform.GetForResource(o.Context, c, it)
Expand Down Expand Up @@ -1398,13 +1398,3 @@ func getDirName(path string) (string, error) {
}
return parentDir, nil
}

func contains(s []string, str string) bool {
for _, v := range s {
if v == str {
return true
}
}

return false
}
43 changes: 32 additions & 11 deletions pkg/util/kubernetes/portforward.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,6 @@ import (

func PortForward(ctx context.Context, c client.Client, ns, labelSelector string, localPort, remotePort uint, stdOut, stdErr io.Writer) error {
log.InitForCmd()
list, err := c.CoreV1().Pods(ns).List(ctx, metav1.ListOptions{
LabelSelector: labelSelector,
})
if err != nil {
return err
}

var forwardPod *corev1.Pod
var forwardCtx context.Context
var forwardCtxCancel context.CancelFunc
Expand All @@ -52,19 +45,21 @@ func PortForward(ctx context.Context, c client.Client, ns, labelSelector string,
if forwardPod == nil && podReady(pod) {
forwardPod = pod
forwardCtx, forwardCtxCancel = context.WithCancel(ctx)
log.Debugf("Setting up Port Forward for pod with name: %q\n", forwardPod.Name)
if _, err := portFowardPod(forwardCtx, c.GetConfig(), ns, forwardPod.Name, localPort, remotePort, stdOut, stdErr); err != nil {
return err
}
}
return nil
}

if len(list.Items) > 0 {
if err := setupPortForward(&list.Items[0]); err != nil {
return err
}
log.Debugf("First attempt to bootstrap Port Forward with LabelSelector: %v\n", labelSelector)
list, err := bootstrapPortForward(ctx, c, ns, labelSelector, setupPortForward)
if err != nil {
return err
}

log.Debugf("Instantiating pod event watcher with LabelSelector: %v and ResourceVersion: %v in namespace: %v\n", labelSelector, list.ResourceVersion, ns)
watcher, err := c.CoreV1().Pods(ns).Watch(ctx, metav1.ListOptions{
LabelSelector: labelSelector,
ResourceVersion: list.ResourceVersion,
Expand All @@ -90,6 +85,7 @@ func PortForward(ctx context.Context, c client.Client, ns, labelSelector string,
if !ok {
return fmt.Errorf("type assertion failed: %v", e.Object)
}
log.Debugf("Handling watch.Added event for pod with name: %v\n", pod.Name)
if err := setupPortForward(pod); err != nil {
return err
}
Expand All @@ -98,27 +94,52 @@ func PortForward(ctx context.Context, c client.Client, ns, labelSelector string,
if !ok {
return fmt.Errorf("type assertion failed: %v", e.Object)
}
log.Debugf("Handling watch.Modified event for pod with name: %v\n", pod.Name)
if err := setupPortForward(pod); err != nil {
return err
}
case watch.Deleted:
log.Debugf("Handling watch.Deleted event\n")
if forwardPod != nil && e.Object != nil {
deletedPod, ok := e.Object.(*corev1.Pod)
if !ok {
return fmt.Errorf("type assertion failed: %v", e.Object)
}
log.Debugf("Handling watch.Deleted event for pod with name: %v while Port Forward was active for pod with name: %v\n", deletedPod.Name, forwardPod.Name)
if deletedPod.Name == forwardPod.Name {
forwardCtxCancel()
forwardPod = nil
forwardCtx = nil
forwardCtxCancel = nil

log.Debugf("Handling watch.Deleted event, since the pod with Port Forward enabled has been deleted we try to bootstrap Port Forward with LabelSelector: %v\n", labelSelector)
_, err := bootstrapPortForward(ctx, c, ns, labelSelector, setupPortForward)
if err != nil {
return err
}
}
}
}
}
}
}

func bootstrapPortForward(ctx context.Context, c client.Client, ns string, labelSelector string, setupPortForward func(pod *corev1.Pod) error) (*corev1.PodList, error) {
list, err := c.CoreV1().Pods(ns).List(ctx, metav1.ListOptions{
LabelSelector: labelSelector,
})
if err != nil {
return nil, err
}
if len(list.Items) > 0 {
log.Debugf("Bootstrapping Port Forward for pod with name: %v\n", list.Items[0].Name)
if err := setupPortForward(&list.Items[0]); err != nil {
return nil, err
}
}
return list, nil
}

func portFowardPod(ctx context.Context, config *restclient.Config, ns, pod string, localPort, remotePort uint, stdOut, stdErr io.Writer) (string, error) {
c, err := corev1client.NewForConfig(config)
if err != nil {
Expand Down