Unit tests for waitForOperationToComplete, isOperationComplete (#688) #614

keithchong · 2023-08-17T18:15:00Z

Description:

This PR does not add the unit tests for service_account.go since it is separate
The first test tests directly the two functions waitForOperationToComplete and isOperationComplete
CreateOperation is not tested thoroughly as well, in particular, there is no test that sets true for waitForOperation.
- This PR makes some slight tweaks in product code to accommodate this.
- The second tests tests the 'old' CreateOperation via the new doCreateOperation. I added two operations but only one is probably sufficient. Let me know if you want to remove one of them

Link to JIRA Story (if applicable):

https://issues.redhat.com/browse/GITOPSRVCE-688

codecov · 2023-08-17T20:01:19Z

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (4ebc3f9) 60.94% compared to head (9d779e9) 61.14%.
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #614      +/-   ##
==========================================
+ Coverage   60.94%   61.14%   +0.19%     
==========================================
  Files          99       99              
  Lines       17865    17869       +4     
==========================================
+ Hits        10888    10926      +38     
+ Misses       5755     5720      -35     
- Partials     1222     1223       +1

Files	Coverage Δ
backend-shared/util/operations/types.go	`65.30% <87.50%> (+15.30%)`	⬆️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

backend-shared/util/operations/types_test.go

Rizwana777 · 2023-08-23T07:37:57Z

@keithchong Left few comments, other than that everything LGTM , thank you

Rizwana777 · 2023-08-24T11:50:04Z

/lgtm

jgwest

Looks great, thanks @keithchong and thanks @Rizwana777 for reviewing! Some small tweaks then good to go!

Re: service_account.go, I was confused, but I understand this now: I'm fine with leaving service_account.go out of this PR/story.

I figured out why it wasn't running in some cases: it requires a valid cluster to be defined in kubeconfig, otherwise it gets skipped (I had forgotten we had added that logic).

And one final request:
It looks like the following block in IsOperationComplete is not covered:

		// Either the operation couldn't be found (which shouldn't happen here), or some other issue, so return it
		return false, err

It should be a quick fix to just call IsOperationComplete with a fake db.Operation that doesn't actually exist in the database.

backend-shared/util/operations/types_test.go

jgwest · 2023-08-29T22:59:45Z

backend-shared/util/operations/types_test.go

+				Expect(operationget.State).To(BeIdenticalTo(db.OperationState_Waiting))
+
+				// Update the operation state to Completed
+				operationget.State = db.OperationState_Completed
+				err = dbq.UpdateOperation(ctx, &operationget)
+				Expect(err).ToNot(HaveOccurred())
+
+				// Get the operation again and check state
+				err = dbq.GetOperationById(ctx, &operationget)
+				Expect(err).ToNot(HaveOccurred())
+				Expect(operationget.State).To(BeIdenticalTo(db.OperationState_Completed))
+


I'm not 100% sure, but I think these Expect functions should instead be return calls: I'm not sure if Expect is supported inside an Eventually (I had presumed that it is not, but if you want to test it to prove me wrong, feel free!)

With the existing code that runs/passes successfully, I simply changed a ToNot to a To, and reran it again, and a failure was reported.

@keithchong Ah, I guess one more question: if one of the Expect calls does fail, does it permanently fail the test (I would assume so), or does it keep trying via the loop?

My guess is that we don't want any of those to be Expect calls to be permanent fails (e.g. a failed Expect will immediately fail the full test), we want the loop to keep trying over and over until it succeeds (or times out and thus fails).

It will fail immediately and will not retry. I think that if it cannot set the operation state to Completed, for example, then it probably will eventually fail repeatedly as well and it will 'eventually' time out. Perhaps it's similar to testing these conditions in the main thread as the primary test, and if it fails, then there is no point in continuing.

This is why the only situation where I return false is when checking if the operation exists. This is because the main thread creates the operation. So 'eventually', it will be created, so it's ok to loop until it exists. The other checks are a bit like 'primary' tests.

I have changed it all to return false. Either way, I think we will know the condition that failed, whether it is directly on the Expect or whether it is repeatedly called and the condition isn't satisfied and it will time out.

Regarding the loop, I moved the location of the loop to better show why we need it. I want to defer setting the operation state. It does make a difference in the behaviour of the test. If we want to allow the for loop in waitForOperationToComplete to loop several times, and allow isOperationComplete to be called several times as well, then we need it. I suppose this test behaviour goes beyond code coverage.

backend-shared/util/operations/types_test.go

jgwest · 2023-08-29T23:06:25Z

backend-shared/util/operations/types_test.go

+				// Allow waitForOperationToComplete to iterate/wait
+				time.Sleep(2 * time.Second)


Would it be possible to remove the time.Sleep? I ask because Sleep statements in test make me very wary (as a source for race conditions)... I checked the logic of the test and I THINK this time.Sleep isn't actually doing anything, so it should be easy to remove.

The purpose of the delay is to give time for the for loop in waitForOperationToComplete in types.go to iterate a few times. Removing the delay does seems to work ok, and the for loop gets executed once.

Try adding a Println just inside the for loop and try adjusting the delay to 2-5 seconds. Your println statement will get executed more than once. Thus, IsOperationComplete also gets called multiple times.

Instead of sleep, I've tried the following, and it simulates the delay too.

currentTime := time.Now() for time.Now().Before(currentTime.Add(2 * time.Second)) { }

If you prefer, we can just remove this.

I believe you can get rid of the delay as long as you convert the Expects to return bools, as above.

See my other comment. #614 (comment)

keithchong · 2023-08-30T21:10:33Z

And one final request: It looks like the following block in IsOperationComplete is not covered:
		// Either the operation couldn't be found (which shouldn't happen here), or some other issue, so return it
		return false, err
It should be a quick fix to just call IsOperationComplete with a fake db.Operation that doesn't actually exist in the database.

Done. Added a new testcase. See latest commit.

…t-appstudio#688) Signed-off-by: Keith Chong <kykchong@redhat.com>

…pstudio#688) Signed-off-by: Keith Chong <kykchong@redhat.com>

jgwest · 2023-10-11T02:33:12Z

I've rebased the PR, and added some additional code/test tweaks. The PR has been open for long enough that I would say lets just go ahead with testing within the original code coverage parameters of the story, just so we can get this one merged and off our plates 😅. I've updated the PR to that effect.

jgwest

LGTM, thanks @keithchong!

openshift-ci · 2023-10-11T12:26:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jgwest, keithchong

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [jgwest]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

keithchong requested a review from jgwest August 17, 2023 18:15

openshift-ci bot requested a review from jopit August 17, 2023 18:15

jgwest requested review from Rizwana777 and removed request for jgwest and jopit August 19, 2023 20:45

Rizwana777 suggested changes Aug 23, 2023

View reviewed changes

openshift-ci bot assigned Rizwana777 Aug 23, 2023

openshift-ci bot added the lgtm label Aug 24, 2023

jgwest requested changes Aug 29, 2023

View reviewed changes

openshift-ci bot assigned jgwest Aug 29, 2023

openshift-ci bot removed the lgtm label Aug 29, 2023

keithchong requested a review from jgwest August 30, 2023 21:10

keithchong and others added 6 commits October 10, 2023 22:24

Unit tests for waitForOperationToComplete, isOperationComplete (redha…

43e5ec8

…t-appstudio#688) Signed-off-by: Keith Chong <kykchong@redhat.com>

Unit tests for waitForOperationToComplete, isOperationComplete (redha…

4cd1a22

…t-appstudio#688) Signed-off-by: Keith Chong <kykchong@redhat.com>

Unit tests for waitForOperationToComplete, isOperationComplete (redha…

d0970ac

…t-appstudio#688) Signed-off-by: Keith Chong <kykchong@redhat.com>

Unit tests for waitForOperationToComplete, isOperationComplete (redha…

126bb30

…t-appstudio#688) Signed-off-by: Keith Chong <kykchong@redhat.com>

Return false instead, move loop out to show why we need it (redhat-ap…

38a1d7a

…pstudio#688) Signed-off-by: Keith Chong <kykchong@redhat.com>

Misc non-functional test and code tweaks

9d779e9

jgwest force-pushed the 688-WaitForOperationToComplete branch from 907fb9c to 9d779e9 Compare October 11, 2023 02:31

jgwest approved these changes Oct 11, 2023

View reviewed changes

openshift-ci bot added the lgtm label Oct 11, 2023

openshift-ci bot added the approved label Oct 11, 2023

jgwest merged commit 83037c5 into redhat-appstudio:main Oct 11, 2023

rh-tap-build-team bot mentioned this pull request Oct 11, 2023

managed-gitops update redhat-appstudio/infra-deployments#2529

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unit tests for waitForOperationToComplete, isOperationComplete (#688) #614

Unit tests for waitForOperationToComplete, isOperationComplete (#688) #614

keithchong commented Aug 17, 2023

codecov bot commented Aug 17, 2023 •

edited

Loading

Rizwana777 commented Aug 23, 2023

Rizwana777 commented Aug 24, 2023

jgwest left a comment

jgwest Aug 29, 2023

keithchong Aug 30, 2023

jgwest Aug 31, 2023

keithchong Sep 1, 2023

keithchong Sep 1, 2023

jgwest Aug 29, 2023

keithchong Aug 30, 2023

jgwest Aug 31, 2023

keithchong Sep 1, 2023

keithchong commented Aug 30, 2023

jgwest commented Oct 11, 2023

jgwest left a comment

openshift-ci bot commented Oct 11, 2023

		// Allow waitForOperationToComplete to iterate/wait
		time.Sleep(2 * time.Second)

Unit tests for waitForOperationToComplete, isOperationComplete (#688) #614

Unit tests for waitForOperationToComplete, isOperationComplete (#688) #614

Conversation

keithchong commented Aug 17, 2023

Description:

Link to JIRA Story (if applicable):

codecov bot commented Aug 17, 2023 • edited Loading

Codecov Report

Rizwana777 commented Aug 23, 2023

Rizwana777 commented Aug 24, 2023

jgwest left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keithchong commented Aug 30, 2023

jgwest commented Oct 11, 2023

jgwest left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Oct 11, 2023

codecov bot commented Aug 17, 2023 •

edited

Loading