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

Jump to bottom

CB-231: Support for testing MusicBrainz-dependent code #134

Merged
merged 2 commits into from Aug 4, 2017
Merged

CB-231: Support for testing MusicBrainz-dependent code #134

merged 2 commits into from Aug 4, 2017

Conversation

ferbncode
Copy link
Member

@ferbncode ferbncode commented Jul 20, 2017 •
edited

Added tests for place entity and sample_data.py which contains all the test data. Presently, the tests take quite sometime to run (working on it).

@gentlecat gentlecat self-requested a review July 20, 2017 10:51
alastair
alastair reviewed Jul 20, 2017
View reviewed changes
docker/docker-compose.dev.yml Outdated
@@ -49,3 +50,12 @@ services:
MB_IMPORT_DUMPS: "true"
ports:
- "5430:5432"

musicbrainz_test_db:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible here to use https://hub.docker.com/r/metabrainz/musicbrainz-test-database/ ? (users already have it downloaded so that they can run the musicbrainz database)

@ferbncode ferbncode force-pushed the testdb branch 5 times, most recently from 2b38b8f to ad90374 Compare July 23, 2017 06:22
@ferbncode ferbncode changed the title [WIP]: CB-231: Add tests for musicbrainz database CB-231: Add tests for musicbrainz database Jul 23, 2017
@ferbncode
Copy link
Member Author

ferbncode commented Jul 23, 2017 •
edited

I've updated the dockerfiles for using the musicbrainz-test-database docker image. That image takes care of making the database as well as dumping the initial.sql (https://github.com/metabrainz/musicbrainz-server/blob/master/t/sql/initial.sql). For the test data, I've added the create_test_data.sql.
Also, conftest.py has been added for populating and deleting the data in musicbrainz_test db at the start and end of a test session.

@ferbncode
Copy link
Member Author

ferbncode commented Jul 23, 2017 •
edited

The following entities are present in the test data:

Release Groups: (and their corresponding releases(with media, recordings) and tags)

Artists: (with tags)

  • Linkin Park (have artist-rels, url-rels etc, also have their two release groups in test db)
  • The Spyro (Performer at event in the test db)
  • Related artists to all entities (members of LP, Jay Z)

Place

Event

gentlecat
gentlecat reviewed Jul 26, 2017
View reviewed changes
Copy link
Contributor

@gentlecat gentlecat left a comment

Choose a reason for hiding this comment

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

What's the function of the critiquebrainz.conftest module? Can you add some docstrings for not very obvious functions and modules describing what their purpose is?

gentlecat
gentlecat suggested changes Jul 27, 2017
View reviewed changes
Copy link
Contributor

@gentlecat gentlecat left a comment •
edited

Choose a reason for hiding this comment

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

It takes way too long to start the MusicBrainz test database for some reason. https://gist.github.com/anonymous/03fad617d32fff4268ae9dce59036aae

This is a big problem since tests should run as fast as possible. I'd suggest considering writing mock functions for accessing MB data since we have so many services running in the dev/test environment already.

@ferbncode ferbncode force-pushed the testdb branch 5 times, most recently from c840795 to 4908a73 Compare August 2, 2017 04:53
gentlecat
gentlecat suggested changes Aug 2, 2017
View reviewed changes
Copy link
Contributor

@gentlecat gentlecat left a comment

Choose a reason for hiding this comment

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

Can you check that the changes in critiquebrainz/frontend/external/musicbrainz_db/place.py actually need to be there?

critiquebrainz/frontend/external/musicbrainz_db/testing.py Outdated
from flask_testing import TestCase
from critiquebrainz.frontend import create_app

class MBDatabaseTestCase(TestCase):
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this base class need to be based on flask_testing's TestCase or a regular one from unittest would be enough?

Copy link
Member Author

@ferbncode ferbncode Aug 3, 2017 •
edited

Choose a reason for hiding this comment

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

Yes, flask_testing's TestCase is required as it sets up the critiquebrainz_redis container for the tests. I get the following error when using unittest.TestCase > https://gist.github.com/ferbncode/2a3f3967015747c45efedfc20b7f3608. But this can be avoided by initializing redis separately in the setUpfunctions. Please suggest which method is preferred. Thanks :)

Copy link
Contributor

Choose a reason for hiding this comment

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

If creating a whole Flask app is unnecessary then I'd say it's better to just initialise the cache module.

critiquebrainz/frontend/external/musicbrainz_db/tests/helpers_test.py Outdated
import critiquebrainz.frontend.external.musicbrainz_db as mb
from critiquebrainz.frontend.external.musicbrainz_db.test_data import linkplaceurl_1, linkplaceurl_2, place_suisto

class HelpersTestClass(MBDatabaseTestCase):
Copy link
Contributor

Choose a reason for hiding this comment

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

HelpersTestCase

gentlecat
gentlecat approved these changes Aug 4, 2017
View reviewed changes
Copy link
Contributor

@gentlecat gentlecat left a comment

Choose a reason for hiding this comment

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

There are still some improvements to be made, but feel free to submit that separately.

critiquebrainz/frontend/external/musicbrainz_db/tests/__init__.py

REDIS_HOST = 'critiquebrainz_redis'
REDIS_PORT = 6379
REDIS_NAMESPACE = 'CB'
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, this is not ideal. It would be good to get these values from the test configuration instead.

@gentlecat gentlecat changed the title CB-231: Add tests for musicbrainz database CB-231: Support for testing MusicBrainz-dependent code Aug 4, 2017
@gentlecat gentlecat merged commit 1818604 into metabrainz:master Aug 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alastair alastair alastair left review comments

@gentlecat gentlecat gentlecat approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@ferbncode @alastair @gentlecat