calpoly-csai / api

Official API for the NIMBUS Voice Assistant accessible via HTTP REST protocol.
https://nimbus.api.calpolycsai.com/
GNU General Public License v3.0
9 stars 4 forks source link

Get code coverage up #118

Closed Jason-Ku closed 4 years ago

Jason-Ku commented 4 years ago

What's New?

How Has This Been Tested?

image

Checklist (check-all-before-merge)

formatting help: - [x] means "checked' and - [ ] means "unchecked"

mfekadu commented 4 years ago

added mock into the pipenv Pipfile and got similar results

image
mfekadu commented 4 years ago

@Jason-Ku Do you know what the warning is about?

Could it be resolved by defining TestEntity within the tests/test_database_wrapper.py file?

After all, it would make maintaining the test_database_wrapper a lot easier in the future if the entity is in the same file.

image

https://docs.pytest.org/en/latest/warnings.html

Jason-Ku commented 4 years ago

@Jason-Ku Do you know what the warning is about?

Could it be resolved by defining TestEntity within the tests/test_database_wrapper.py file?

After all, it would make maintaining the test_database_wrapper a lot easier in the future if the entity is in the same file.

image

https://docs.pytest.org/en/latest/warnings.html

I think it's because pytest skips any class that starts with "test", and raises an error even though its totally fine.

That leads me to believe that including the TestEntity class would in test_database_wrapper might not solve the issue, but it's worth a shot and I agree that it would make things easier. An alternative I just found would be to add test = False as an attribute of TestEntity and it'll ignore it and not raise the error you're seeing.

The last approach which I don't recommend and am not even sure if it will work, would be to rename TestEntity to not have the word "test" in it, but that would make things pretty confusing, huh?

mfekadu commented 4 years ago

Removed all the junk from the database_wrapper.py main method, shouldn't need it anymore now that we have tests, I hope. If theres a case for keeping it there, we can add it back.

Here's a quick case for keeping it there:

However, I won't miss it much considering you converted the code into test cases! 😄

mfekadu commented 4 years ago

The last approach which I don't recommend and am not even sure if it will work, would be to rename TestEntity to not have the word "test" in it, but that would make things pretty confusing, huh?

@Jason-Ku What if we called it MockEntity?

Would that be a good middle ground?

Jason-Ku commented 4 years ago

The last approach which I don't recommend and am not even sure if it will work, would be to rename TestEntity to not have the word "test" in it, but that would make things pretty confusing, huh?

@Jason-Ku What if we called it MockEntity?

Would that be a good middle ground?

I totally forgot about prefixing things with Mock - yeah I'll try that!

EDIT: It works. I'll update this pr!

mfekadu commented 4 years ago

image

This PR had a significant and positive impact on our code coverage!