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

Adding unit-tests and json schema validation #14

Merged
merged 2 commits into from
May 20, 2024

Conversation

fabi200123
Copy link
Contributor

  • Adding json schema validation for extra specs
  • Adding unit-tests

},
"allow_image_owners": {
"allowed_image_owners": {
Copy link
Member

Choose a reason for hiding this comment

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

I get why this change is made, but keep in mind that people are already using the old naming. Changing this will break their pools when they update.

Copy link
Contributor Author

@fabi200123 fabi200123 May 20, 2024

Choose a reason for hiding this comment

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

In the code itself it's allowed_image_owners, but in the Readme was allow_image_owners. Which one should we stick to then?

Copy link
Member

Choose a reason for hiding this comment

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

ohh! The one in the code. Aesthetics matter less. We don't want to change functionality for the sake of aesthetics, but if the code is okay and the README has a typo, ignore me.

client/client.go Outdated
@@ -99,6 +99,14 @@ type OpenstackClient struct {
controllerID string
}

func SetOpenstackClient(client *OpenstackClient, compute, image, network, volume *gophercloud.ServiceClient, controllerID string) {
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 this can be a NewTestOpenStackClient() that creates a proper testing client instead of replacing individual service clients inside the already instantiated one.

You can then replace the old one with the one returned here:

func NewTestOpenStackClient(mockClient *gophercloud.ServiceClient, controllerID string) *OpenstackClient {
    return &OpenstackClient{
		compute:      mockClient,
		image:        mockClient,
		network:      mockClient,
		volume:       mockClient,
		controllerID: controllerID,
	}
}

And then later in your code, you can just:

serviceClient := thclient.ServiceClient()
mockCli := NewTestOpenStackClient(serviceClient, "my-controller-id")
provider.cli = mockCli

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is indeed better! Updated it!

client/client.go Outdated
@@ -99,6 +99,16 @@ type OpenstackClient struct {
controllerID string
}

func NewTestOpenStackClient(mockClient *gophercloud.ServiceClient, controllerID string) *OpenstackClient {
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be moved to the _test.go file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I have tried before, the functions under file_test.go will not be seen by go as part of that package. So, if I move that function to client_test.go, it wont be seen by the provider.go (where it is used) as part of the client package

Copy link
Member

@gabriel-samfira gabriel-samfira May 20, 2024

Choose a reason for hiding this comment

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

you can use something like this:

https://github.com/cloudbase/garm/blob/6e416bb4947380735771bd1234b90fe63c6e46d4/internal/testing/testing.go#L15-L16

and pass that tag when running the tests:

https://github.com/cloudbase/garm/blob/6e416bb4947380735771bd1234b90fe63c6e46d4/Makefile#L91

The idea is to only make that function available during testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created a new file testing.go in the client package that is accessed only by the flag testing.

@gabriel-samfira gabriel-samfira merged commit 63cce58 into cloudbase:main May 20, 2024
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.

2 participants