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

Test: Delete Collection #484

Merged

Conversation

prabalsingh24
Copy link
Contributor

Problem

Tests for POST collection/:collectionID/delete/handler

@coveralls
Copy link

coveralls commented Aug 5, 2020

Coverage Status

Coverage increased (+0.2%) to 58.1% when pulling add32a7 on prabalsingh24:test-deleteCollection into 0de3fc5 on bookbrainz:UserCollection.

Copy link
Member

@MonkeyDo MonkeyDo left a comment

Choose a reason for hiding this comment

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

Nice and clean, well done !

I only have one request: wherever you expect an error status response, let's check that we're getting the error Same here, let's check statusMessage we're expecting.

Copy link
Member

@MonkeyDo MonkeyDo left a comment

Choose a reason for hiding this comment

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

That looks much better!
With the error message checks in place, we're sure that the route/tests aren't failing for some other unrelated reason, making them much more solid !

@MonkeyDo MonkeyDo merged commit d1fd35b into metabrainz:UserCollection Aug 6, 2020
@prabalsingh24
Copy link
Contributor Author

I just realized I missed a test it('should remove remove the collection from the ES index')
I have to implement this feature as well :)

@prabalsingh24
Copy link
Contributor Author

In your words, "that's why we love tests!" haha

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.

3 participants