Skip to content
This repository was archived by the owner on Nov 27, 2024. It is now read-only.

rest: add tests for read and list in readclient pkg#231

Merged
openshift-merge-bot[bot] merged 4 commits intokonflux-workspaces:mainfrom
filariow:utest-rest-persistence-readclient
Jul 17, 2024
Merged

rest: add tests for read and list in readclient pkg#231
openshift-merge-bot[bot] merged 4 commits intokonflux-workspaces:mainfrom
filariow:utest-rest-persistence-readclient

Conversation

@filariow
Copy link
Member

@filariow filariow commented Jul 2, 2024

Signed-off-by: Francesco Ilario filario@redhat.com

@filariow filariow requested a review from dperaza4dustbit July 2, 2024 14:39
@openshift-ci openshift-ci bot requested a review from sadlerap July 2, 2024 14:39
@openshift-ci openshift-ci bot added the approved label Jul 2, 2024
@filariow filariow force-pushed the utest-rest-persistence-readclient branch 3 times, most recently from 6eed322 to 13c769f Compare July 2, 2024 16:22
Comment on lines +75 to +76
// mapper expects to be called once.
// It returns the list of mapped workspaces (just objectmeta)
Copy link
Member

Choose a reason for hiding this comment

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

These two lines are over-indented.

Copy link
Member Author

Choose a reason for hiding this comment

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

this also happens below, but it's done by go fmt itself

Expect(err).ToNot(HaveOccurred())
Expect(actualWorkspaces.Items).To(HaveLen(1))
Expect(actualWorkspaces.Items[0].ObjectMeta).To(Equal(expectedObjectMeta))
})
Copy link
Member

Choose a reason for hiding this comment

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

I think we might need two more happy-path tests:

  1. More than one workspace matches the label selector.
  2. No workspaces match the label selector.

Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

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

good idea, let me work on this two test cases

Copy link
Member Author

@filariow filariow Jul 17, 2024

Choose a reason for hiding this comment

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

added in ec80332

Expect(err).NotTo(HaveOccurred())
Expect(returnedWorkspace).To(Equal(mappedWorkspace))
// test data is deepcopied
Expect(&returnedWorkspace).NotTo(BeIdenticalTo(&mappedWorkspace))
Copy link
Member

Choose a reason for hiding this comment

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

I'm not convinced this check is correct. Both returnedWorkspace and mappedWorkspace are distinct variables defined in this scope, so taking their addresses and comparing them will always be a false comparison (so this check will always pass).

Copy link
Member Author

Choose a reason for hiding this comment

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

great catch! fixed in 4b9ee08

@sadlerap
Copy link
Member

Also looks like it's got a merge conflict that needs to be resolved.

Signed-off-by: Francesco Ilario <filario@redhat.com>
@filariow filariow force-pushed the utest-rest-persistence-readclient branch from f3fc815 to f52f7a1 Compare July 17, 2024 13:27
filariow added 2 commits July 17, 2024 15:34
Signed-off-by: Francesco Ilario <filario@redhat.com>
Signed-off-by: Francesco Ilario <filario@redhat.com>
Comment on lines +76 to +80
oww := make([]metav1.ObjectMeta, len(actualWorkspaces.Items))
for i, w := range actualWorkspaces.Items {
oww[i] = w.ObjectMeta
}
Expect(oww).To(BeEquivalentTo(expectedObjectMetas))
Copy link
Member Author

Choose a reason for hiding this comment

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

@sadlerap do you have any suggestions here?

Copy link
Member

Choose a reason for hiding this comment

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

I have a way of doing this without copying the ObjectMeta fields out, but it's longer than this. This is more than fine for now.

Signed-off-by: Francesco Ilario <filario@redhat.com>
@openshift-ci
Copy link

openshift-ci bot commented Jul 17, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: filariow, sadlerap

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

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

@openshift-merge-bot openshift-merge-bot bot merged commit 782a867 into konflux-workspaces:main Jul 17, 2024
@filariow filariow deleted the utest-rest-persistence-readclient branch July 17, 2024 17:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants