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

CB-355: Implemented the retrieval of entities using mbdata #262

Merged
merged 15 commits into from Jul 11, 2019

Conversation

@spellew
Copy link
Contributor

commented Jun 15, 2019

No description provided.

@spellew spellew changed the title Implemented the retrieval of entities using mbdata CB-355: Implemented the retrieval of entities using mbdata Jun 15, 2019

@paramsingh

This comment has been minimized.

Copy link
Member

commented Jun 19, 2019

Hi @spellew, tests seem to be failing, can you take a look?

@ferbncode

This comment has been minimized.

Copy link
Collaborator

commented Jun 21, 2019

@spellew Did you get a chance to look at the tests?

@spellew

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2019

I'm still looking into it.

@ferbncode

This comment has been minimized.

Copy link
Collaborator

commented Jun 21, 2019

@spellew awesome, I'll try to find and direct some points that I think might be the reason of the tests failing.

@ferbncode

This comment has been minimized.

Copy link
Collaborator

commented Jun 27, 2019

@spellew Please modify the docker-compose.test.yml and add the musicbrainz_db container:

musicbrainz_db:
    image: metabrainz/musicbrainz-test-database:beta
    environment:
      PGDATA: /var/lib/postgresql/data/pgdata
      MB_IMPORT_DUMPS: "false"
    ports:
      - "5430:5432"
    network_mode: bridge

Also, modify critiquebrainz/test_config.py to add the MB_DATABASE_URI and set it to postgresql://musicbrainz:musicbrainz@musicbrainz_db:5432/musicbrainz_test. I guess this file is a bit outdated and at some point some changes were removed.

To check if everything is good:

docker-compose -f docker/docker-compose.test.yml down && docker-compose -f docker/docker-compose.test.yml up --build.

@ferbncode

This comment has been minimized.

Copy link
Collaborator

commented Jun 27, 2019

Or maybe if you want, you can open another pull request with fixes to the docker-compose.test.yml?

docker/docker-compose.test.yml Outdated Show resolved Hide resolved
docker/docker-compose.test.yml Outdated Show resolved Hide resolved
docker/docker-compose.test.yml Show resolved Hide resolved
@ferbncode

This comment has been minimized.

Copy link
Collaborator

commented Jul 6, 2019

@spellew Please make changes to musicbrainz_db/__init__.py to instantiate connection to db via brainzutils init_db_engine (https://github.com/metabrainz/brainzutils-python/blob/master/brainzutils/musicbrainz_db/__init__.py#L10).

Did you run your changes locally and write a review for testing your changes (for this PR)? Maybe we need to discuss regarding tests and local setup. Ping me on the chat :)

@ferbncode
Copy link
Collaborator

left a comment

This pull request looks good now. @paramsingh Could you help merge and deploy it on beta.critiquebrainz.org? Also, feel free to review if you'd like.

@ferbncode

This comment has been minimized.

Copy link
Collaborator

commented Jul 10, 2019

@spellew Let's plan to add integration tests in Musicbrainz DB, once we merge all open pull requests you have. They are necessary now

@spellew

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2019

@ferbncode Do you mean they aren't necessary now?

@paramsingh paramsingh merged commit 4c472c9 into metabrainz:master Jul 11, 2019

2 checks passed

Jenkins [PyLint & Flake8] Build finished.
Details
Jenkins [PyTest] Build finished.
Details
@ferbncode

This comment has been minimized.

Copy link
Collaborator

commented Jul 11, 2019

@spellew I meant that now that we have removed the mocked tests from CB (which did check (if not all) the musicbrainz_db code), we should rather have some test data that we put into the musicbrainz_test_database and query it to verify that all the musicbrainz_db functions work as expected.

@spellew

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2019

@ferbncode and these tests would be different from the ones I wrote testing the functions in entities.py? I sent the gist previously, but didn't open a PR.

@ferbncode

This comment has been minimized.

Copy link
Collaborator

commented Jul 12, 2019

Could you send the link again? Also, I guess you missed from the chat, but there is a follow-up for this pull request: Functions in musicbrainz_db/utils.py can also re-use some functions from brainzutils/musicbrainz_db. There is a trickiness of handling the reviewed entities which were then deleted from the musicbrainz database. Take a look when you can :)

@spellew

This comment has been minimized.

Copy link
Contributor Author

commented Jul 12, 2019

@ferbncode It was this link spellew@1532405. Should I open a separate pull request for reusing utils.py?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.