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.

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

Update tests for installed package to test call to client. #4225

Merged
merged 2 commits into from
Feb 8, 2022

Conversation

absoludity
Copy link
Contributor

Signed-off-by: Michael Nelson minelson@vmware.com

Description of the change

I was getting ready to update the GetInstalledPackageResourceRefs shared util, went to write a test for the existing code, but found that the existing tests for that module were mocking the actual function being tested, rather than mocking the underlying client so that we can verify that the client is called with the arguments.

This PR just updates those tests so that we are not mocking the actual functions that we're testing (guess we did this when rushing to switch to the new grpc client). The tests aren't particularly valuable here, since the util is just calling the client with the same args, but the test for GetInstalledPackageResourceRefs will be extended to be more useful in the following PR (where I'll add retries).

Only stopping and pushing this now because I don't have time to do the second change today.

Benefits

Just preparing for the next PR which will add the retries for GetInstalledPackageResourceRefs.

Possible drawbacks

Applicable issues

Additional information

Copy link
Contributor

@antgamdia antgamdia left a comment

Choose a reason for hiding this comment

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

Thanks for this change! Yep, we were in a rush and didn't properly mock the client.
In fact, I had a look (trying to mock the grpc client itself by intercepting the transport with a library), but I finally wasn't able to do it. This neat approach of setMockCoreClient is simple and effective :)

Base automatically changed from 3695-check-installed-pkg-details-4 to main February 4, 2022 03:10
@absoludity absoludity force-pushed the 3695-check-installed-pkg-details-5 branch from 28c7f6d to d7437d4 Compare February 4, 2022 03:14
Signed-off-by: Michael Nelson <minelson@vmware.com>
Signed-off-by: Michael Nelson <minelson@vmware.com>
@absoludity absoludity force-pushed the 3695-check-installed-pkg-details-5 branch from d7437d4 to 660e74e Compare February 8, 2022 10:31
@absoludity absoludity merged commit 6b48550 into main Feb 8, 2022
@absoludity absoludity deleted the 3695-check-installed-pkg-details-5 branch February 8, 2022 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants