jkube icon indicating copy to clipboard operation
jkube copied to clipboard

PortForwardServiceTest : Remove use of `Thread.sleep`

Open rohanKanojia opened this issue 1 year ago • 3 comments

Component

JKube Kit

Task description

Description

We have this code in PortForwardServiceTest which seems to be forwarding a port inside Kubernetes using Fabric8 Kubernetes Client

https://github.com/eclipse/jkube/blob/3afc7bf1f8dbe4452826c5ec7ec58de5c9408d53/jkube-kit/config/service/src/test/java/org/eclipse/jkube/kit/config/service/PortForwardServiceTest.java#L98-L100

However, I'm not sure why we have Thread.sleep inside the try block after initializing port forward. We don't seem to be testing anything after this wait.

It's better to just remove this unnecessary waiting, asserting whether value returned from forwardPortAsync is not null should suffice.

Expected Behavior

Thread.sleep is removed from PortForwardServiceTest.simpleScenario

Acceptance Criteria

  • [ ] Thread.sleepis removed fromPortForwardServiceTest.simpleScenario`
  • [ ] PortForwardServiceTest.simpleScenario should still pass after making these changes

Before you start :red_circle:

:point_down: :point_down: :point_down: :point_down: :point_down: :point_down: :point_down: :point_down: :point_down: :point_down::point_down::point_down::point_down::point_down::point_down::point_down: Make sure you read the contributing guide first. Pay special attention to the ECA agreement section and the requirement to sign-off your commit.

How to manually test my changes

Kubernetes

If you don't have a real Kubernetes cluster available (most probably), you can use Minikube or Kind to test with a local cluster.

OpenShift

If you don't have a real OpenShift cluster available (most probably), you can use Red Hat's developer Sandbox for Red Hat OpenShift. The only requirement is to have a Red Hat account.

Once you have your Sandbox environment, you'll need to download the oc tool from the cluster console. (Press the ? icon and from the context menu select Command line tools, you'll be redirected to https://$subdomain.openshiftapps.com/command-lines-tools where you'll be able to download the CLI for your platform)

rohanKanojia avatar Feb 28 '24 16:02 rohanKanojia

The sleep is probably there to force the code to pass through the asynchronous block, the test should be refactored to assert something (an open connection, a log message, etc.)

manusa avatar Feb 29 '24 06:02 manusa

When I was trying this, test was passing even when I removed Thread.sleep ( probably because we are not checking anything after opening port forward)

rohanKanojia avatar Feb 29 '24 06:02 rohanKanojia

test was passing because the test is not asserting anything (just like now).

manusa avatar Feb 29 '24 08:02 manusa