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

Fix /discovery-methods endpoint #33

Merged
merged 1 commit into from Sep 12, 2016
Merged

Fix /discovery-methods endpoint #33

merged 1 commit into from Sep 12, 2016

Conversation

ikalnytskyi
Copy link
Contributor

Currently there're two issues with that endpoint:

  • It uses DBAPI.get_discovery_methods() without arguments but it
    does receive one.
  • It expects DBAPI.get_discovery_methods() to return a list of
    supported discovery methods, but it returns a dictionary instead.

The fix for the first one is to get rid of unused argument, and for the
second one - return a list instead of dictionary. The funny thing is, we
have a correct mock in tests.. it's a production code that's broken.

@sc68cal
Copy link
Contributor

sc68cal commented Sep 9, 2016

OK - can you add a unit test to https://github.com/sc68cal/Kostyor/blob/master/kostyor/tests/unit/test_db_api.py to catch this?

@ikalnytskyi
Copy link
Contributor Author

Sure.

@ikalnytskyi
Copy link
Contributor Author

@sc68cal Well, I can add test like this:

    def test_discovery_methods(self):
        methods = db_api.get_discovery_methods()
        self.assertItemEquals(methods, [constants.OPENSTACK])

but it won't catch anything. I think we should start testing API without mocking database. It's cheap and will prevent us from possible errors. What do you think?

@sc68cal
Copy link
Contributor

sc68cal commented Sep 9, 2016

We already mock the database in the unit tests for the REST API. Please create a test that checks the return value of db_api.get_discovery_methods()

@ikalnytskyi
Copy link
Contributor Author

@sc68cal i'm proposing the vice versa: do not mock it. Otherwise, it won't cover the issue because we simply won't check integration between db_api and rest_api.

Example:

  • rest_api test mocks db_api to return correct data. test is passed.
  • db_api test checks returned data. test is passed.
  • however, db_api may return data not in format that's expected by rest_api module. this is exactly the case of this bug.

Currently there're two issues with that endpoint:

* It uses DBAPI.get_discovery_methods() without arguments but it
  does receive one.

* It expects DBAPI.get_discovery_methods() to return a list of
  supported discovery methods, but it returns a dictionary instead.

The fix for the first one is to get rid of unused argument, and for the
second one - return a list instead of dictionary. The funny thing is, we
have a correct mock in tests.. it's a production code that's  broken.
@sc68cal sc68cal merged commit b123d7e into Mirantis:master Sep 12, 2016
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.

None yet

2 participants