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

Tests should be refactored #67

Open
almet opened this issue Mar 22, 2016 · 12 comments
Open

Tests should be refactored #67

almet opened this issue Mar 22, 2016 · 12 comments
Assignees
Labels
easy-pick enhancement stale For marking issues as stale. Labeled issues will be closed soon if label is not removed.

Comments

@almet
Copy link
Member

almet commented Mar 22, 2016

Some tests would benefit from a refactor. I'm mostly thinking about the logic contained in the _get_or_create private method, which is not directly tested but via its callers.

One solution here would be to create a base class with the behaviours we want to test and then derive multiple classes with the different inputs (bucket, collection and record).

@Sayli-Karnik
Copy link
Member

Sayli-Karnik commented Apr 19, 2016

I could work on this..So BucketTest, CollectionTest ,RecordTest must extend a common base class. And are we just dealing with _get_or_create or all methods? @almet @Natim

@sahildua2305
Copy link
Member

@Sayli-Karnik Can I take this up, in case you're not?

@almet @Natim Can I have more information about this issue?

@leplatrem
Copy link
Contributor

To be honest I'm not very convinced by the proposition of a «base class», but we can try to see how it goes.

The idea is to go through every buckets/collections/groups/records client tests and see how much code is «repeated» between them. If there is a lot, let's go with the base class approach.

Let us know what you find out ;)

@zeddmaxx
Copy link

Hi all
This looks very interesting.
I would like to contribute to this, I hope I can.

@almet
Copy link
Member Author

almet commented Sep 19, 2016

Well, if you get the thumbs up from @sahildua2305 , I believe you can go ahead and take it!

@sahildua2305
Copy link
Member

Sure @zeddmaxx! Go ahead. It's all yours.

@sahildua2305
Copy link
Member

@zeddmaxx Any progress on this?

@sahildua2305
Copy link
Member

Accounting to no response from you, @zeddmaxx, I'd like to claim it back.

/cc @almet @leplatrem

@sahildua2305
Copy link
Member

sahildua2305 commented Nov 11, 2016

@glasserc thanks!

Btw, do we want to see how much code is repeated between corresponding tests for buckets/collections/records/groups? Or separately for Bucket tests and then for collection tests and so on?

@glasserc
Copy link
Contributor

I think @almet's original goal was to share code that corresponds to the same test in buckets, collections, records, and groups. But any kind of DRYing up or refactoring could potentially be beneficial.

@sahildua2305
Copy link
Member

@glasserc I noticed that there are a lot of tests in Buckets / Collections / Records / Groups which test the same functionality. For example - there are tests to verify that create_xxx and update_xxx issues put http request and that patch issues patch http request.

And that I think is intentional and necessary to make sure we have good test coverage. Making a base class and calling these tests indirectly using that class methods and just passing params necessary for new tests will make tests difficult to understand for new contributors.

What do you think?

@glasserc
Copy link
Contributor

I don't have a strong feeling about it, but I'm not as knowledgeable in this area as @leplatrem and @almet. If we think it's a bad idea, let's close the issue.

@alexcottner alexcottner added the stale For marking issues as stale. Labeled issues will be closed soon if label is not removed. label Jul 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
easy-pick enhancement stale For marking issues as stale. Labeled issues will be closed soon if label is not removed.
Projects
None yet
Development

No branches or pull requests

7 participants