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

Add organizations.go unit tests and PoolManagerController mocks #8

Merged

Conversation

mihaelabalutoiu
Copy link
Contributor

@mihaelabalutoiu mihaelabalutoiu commented Aug 19, 2022

  • Add organizations.go unit tests
    • Also, update vendor with the github.com/stretchr/testify/suite.
  • Add PoolManagerController mocks

Copy link
Member

@gabriel-samfira gabriel-samfira left a comment

Choose a reason for hiding this comment

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

Before doing an in-depth review, it would help to deduplicate some code. Left a couple of comments that should be addressed first.

The rest of the changes are similar to the one I left the comments on, so reviewing those changes will be faster once the two comments are addressed.

Thanks for the PR!

runner/organizations_test.go Outdated Show resolved Hide resolved
runner/organizations_test.go Outdated Show resolved Hide resolved
Copy link
Member

@gabriel-samfira gabriel-samfira left a comment

Choose a reason for hiding this comment

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

Looks a lot better. I know this may be a pain, but I think it's worth removing the store mock and simply using the sql database backend with the sqlite driver. It would spare you from a lot of mocks, and would yield better data you can test against (ie: objects that include all fields, including IDs)

runner/organizations_test.go Outdated Show resolved Hide resolved
runner/organizations_test.go Outdated Show resolved Hide resolved
runner/mocks/poolManagerController.go Show resolved Hide resolved
@mihaelabalutoiu
Copy link
Contributor Author

Looks a lot better. I know this may be a pain, but I think it's worth removing the store mock and simply using the sql database backend with the sqlite driver. It would spare you from a lot of mocks, and would yield better data you can test against (ie: objects that include all fields, including IDs)

I made this changes. Please take a look now.

Copy link
Member

@gabriel-samfira gabriel-samfira left a comment

Choose a reason for hiding this comment

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

Looks much better. Just one small nit and we can merge this.

runner/organizations_test.go Outdated Show resolved Hide resolved
runner/organizations_test.go Outdated Show resolved Hide resolved
Also, update vendor with the `github.com/stretchr/testify/suite`.
@mihaelabalutoiu
Copy link
Contributor Author

Looks much better. Just one small nit and we can merge this.

I made this change, please take a look now.

@gabriel-samfira gabriel-samfira merged commit c0df364 into cloudbase:main Sep 6, 2022
@mihaelabalutoiu mihaelabalutoiu deleted the add-organizations-unit-tests branch September 7, 2022 12:23
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