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

Jump to bottom

Refactor applicationEventQueueLoop so it can be unit tested, and write new unit tests for it #502

Merged

jgwest merged 3 commits into redhat-appstudio:main from jgwest:improve-test-coverage-feb-2023

Aug 1, 2023

Member

jgwest commented Jul 14, 2023

Description:

Move the contents of applicationEventQueueLoop into a separate struct, so that the state can be verified by unit tests
Move the existing logic (which processesindividual application event messages) into a new function processApplicationEventQueueLoopMessage
Call processApplicationEventQueueLoopMessage via new unit tests

Link to JIRA Story (if applicable): N/A

jgwest added 2 commits

July 14, 2023 04:26


          Add syncoperation test to application event loop tests

2cce9e4


          Add deployment, managed env, status tick, and status check app event …

…loop tests

openshift-ci bot added the do-not-merge/work-in-progress label

Contributor

openshift-ci bot commented Jul 14, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Contributor

openshift-ci bot commented Jul 14, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jgwest

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

openshift-ci bot added the approved label

jgwest changed the title ~~Refactor applicationEventQueueLoop so it can be unit testsed and write new unit tests~~ Refactor applicationEventQueueLoop so it can be unit tested, and write new unit tests for it

Member Author

jgwest commented Jul 17, 2023

/test all

jgwest commented

View reviewed changes

backend/eventloop/application_event_loop/application_event_loop.go

		@@ -114,8 +114,34 @@ func startApplicationEventQueueLoopWithFactory(ctx context.Context, aeqlParam Ap

		}

		type applicationEventQueueLoopState struct {

Member Author

jgwest Jul 22, 2023

Note for reviewer: I moved these out of applicationEventQueueLoop, and added some comments, they are otherwise the same as before.

Contributor

keithchong Jul 25, 2023

Ok, I don't see where these fields are moved out from applicationEventQueueLoop at line 65. You'd think that there would be a git diff on this?

Member Author

jgwest Jul 31, 2023 •

edited

Loading

Yah, GitHub diff isn't very good here, in this case I often find I have to copy/paste the change into a text file, and compare them using another tool like VS Code, Meld Merge, or BeyondCompare.

jgwest commented

View reviewed changes

backend/eventloop/application_event_loop/application_event_loop.go

-              				log.V(logutil.LogLevel_Debug).Info("applicationEventQueueLoop received event",
-              					"event", eventlooptypes.StringEventLoopEvent(eventLoopMessage))
+              // returns true if the event loop should be terminated, false otherwise.
+              func processApplicationEventQueueLoopMessage(ctx context.Context, newEvent RequestMessage, state *applicationEventQueueLoopState, input chan RequestMessage, k8sClient client.Client, log logr.Logger) bool {

Member Author

jgwest Jul 22, 2023

Note for reviewer: I moved the contents of applicationEventQueueLoop into its own function, so it was easier to unit test. The code is otherwise the same (the GitHub compare is mangling it 😄 )

Contributor

keithchong Jul 25, 2023

Ok, looks like you did move the contents of that loop out to the new function.

I think you added the two new const booleans.

jgwest marked this pull request as ready for review

July 22, 2023 09:59

openshift-ci bot removed the do-not-merge/work-in-progress label

openshift-ci bot requested review from jopit and Rizwana777

July 22, 2023 09:59

jgwest requested review from keithchong and removed request for jopit and Rizwana777

July 24, 2023 11:40

keithchong reviewed

View reviewed changes

backend/eventloop/application_event_loop/application_event_loop_test.go Outdated

+              							Event: &eventlooptypes.EventLoopEvent{
+              								EventType:   eventlooptypes.SyncRunModified,
+              								ReqResource: eventlooptypes.GitOpsDeploymentSyncRunTypeName,
+              								WorkspaceID: string(workspaceUID),

Contributor

keithchong Jul 25, 2023

Replace with: WorkspaceID: workspaceUID,

keithchong reviewed

View reviewed changes

backend/eventloop/application_event_loop/application_event_loop_test.go Outdated

+              							Event: &eventlooptypes.EventLoopEvent{
+              								EventType:   eventlooptypes.SyncRunModified,
+              								ReqResource: eventlooptypes.GitOpsDeploymentSyncRunTypeName,
+              								WorkspaceID: string(workspaceUID),

Contributor

keithchong Jul 25, 2023

Replace with: WorkspaceID: workspaceUID,

keithchong reviewed

View reviewed changes

backend/eventloop/application_event_loop/application_event_loop_test.go Outdated

+              							Event: &eventlooptypes.EventLoopEvent{
+              								EventType:   eventlooptypes.DeploymentModified,
+              								ReqResource: eventlooptypes.GitOpsDeploymentTypeName,
+              								WorkspaceID: string(workspaceUID),

Contributor

keithchong Jul 25, 2023

Replace with: WorkspaceID: workspaceUID,

keithchong reviewed

View reviewed changes

backend/eventloop/application_event_loop/application_event_loop_test.go Outdated

+              							Event: &eventlooptypes.EventLoopEvent{
+              								EventType:   eventlooptypes.DeploymentModified,
+              								ReqResource: eventlooptypes.GitOpsDeploymentTypeName,
+              								WorkspaceID: string(workspaceUID),

Contributor

keithchong Jul 25, 2023

Replace with: WorkspaceID: workspaceUID,

keithchong reviewed

View reviewed changes

backend/eventloop/application_event_loop/application_event_loop_test.go Outdated

+              							Event: &eventlooptypes.EventLoopEvent{
+              								EventType:   eventlooptypes.ManagedEnvironmentModified,
+              								ReqResource: eventlooptypes.GitOpsDeploymentManagedEnvironmentTypeName,
+              								WorkspaceID: string(workspaceUID),

Contributor

keithchong Jul 25, 2023

Replace with: WorkspaceID: workspaceUID,

keithchong reviewed

View reviewed changes

backend/eventloop/application_event_loop/application_event_loop_test.go Outdated

+              							MessageType: eventlooptypes.ApplicationEventLoopMessageType_Event,
+              							Event: &eventlooptypes.EventLoopEvent{
+              								EventType:   eventlooptypes.UpdateDeploymentStatusTick,
+              								WorkspaceID: string(workspaceUID),

Contributor

keithchong Jul 25, 2023

Replace with: WorkspaceID: workspaceUID,

keithchong reviewed

View reviewed changes

backend/eventloop/application_event_loop/application_event_loop_test.go Outdated

+              							MessageType: eventlooptypes.ApplicationEventLoopMessageType_WorkComplete,
+              							Event: &eventlooptypes.EventLoopEvent{
+              								EventType:   eventlooptypes.UpdateDeploymentStatusTick,
+              								WorkspaceID: string(workspaceUID),

Contributor

keithchong Jul 25, 2023

Replace with: WorkspaceID: workspaceUID,


          Respond to review comments: remove cast to string

b9898e9

codecov bot commented Jul 29, 2023

Codecov Report

Patch coverage: 73.58% and project coverage change: +0.87% 🎉

Comparison is base (5580df0) 51.60% compared to head (b9898e9) 52.48%.
Report is 110 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #502      +/-   ##
==========================================
+ Coverage   51.60%   52.48%   +0.87%     
==========================================
  Files         109      108       -1     
  Lines       19041    19091      +50     
==========================================
+ Hits         9827    10020     +193     
+ Misses       8029     7858     -171     
- Partials     1185     1213      +28

Files Changed	Coverage Δ
...p/application_event_loop/application_event_loop.go	`76.51% <73.07%> (+18.97%)`	⬆️
...io.redhat.com/deploymenttargetbinder_controller.go	`70.38% <100.00%> (+0.45%)`	⬆️

... and 32 files with indirect coverage changes

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

jgwest merged commit b510bcb into redhat-appstudio:main

13 checks passed

rh-tap-build-team bot mentioned this pull request

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

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels