UIP-49 - Fix appinfo tests #97
Conversation
| self.narrativeMethodStoreURL, self.catalogURL) | ||
| catalog = Catalog(self.catalogURL) | ||
| nms = NarrativeMethodStore(self.narrativeMethodStoreURL) | ||
| output = get_all_app_info(input["tag"], input["user"], nms, catalog) |
There was a problem hiding this comment.
Modified this to pass along the clients instead of a URL, which makes test mocking easier.
| def get_ignore_categories(): | ||
| return {x: 1 for x in IGNORE_CATEGORIES} | ||
| def get_ignore_categories() -> dict[str, int]: | ||
| return dict.fromkeys(IGNORE_CATEGORIES, 1) |
There was a problem hiding this comment.
Suggested by ruff. I think it's fine. Really, I think that API call is kinda goofy in expecting a dictionary instead of a list, but 🤷
There was a problem hiding this comment.
That's a pretty exciting shorthand - I don't think I've seen it before.
Not sure why the API couldn't just return IGNORE_CATEGORIES as that would ensure it was deduped and there's no need to do anything else to it... but ours is not to question why!
| for t in type_list: | ||
| try: | ||
| with contextlib.suppress(IndexError): |
There was a problem hiding this comment.
Another piece of ruffage since we're ignoring errors anyway.
| def get_all_app_info(tag, user, nms_url, catalog_url): | ||
| if tag not in ["release", "beta", "dev"]: | ||
| def get_all_app_info(tag: str, user: str, nms: NarrativeMethodStore, catalog: Catalog): | ||
| if not isinstance(tag, str) or tag not in APP_TAGS: |
There was a problem hiding this comment.
Ruff suggested converting to a set which makes sense, then tests failed if non-hashable things were passed in as the app tag.
test/AppInfo_test.py
Outdated
There was a problem hiding this comment.
Not deleted, moved to test/integration/test_appinfo.py. Which is unfortunate for comparison. I'll do this differently in later PRs.
test/data/test_app_data.json
Outdated
There was a problem hiding this comment.
Some basic app data for the mocks to load. We could use VCR here, but I think this works well enough.
There was a problem hiding this comment.
This is a fragment of the return value from NMS.list_methods
test/data/test_app_favorites.json
Outdated
There was a problem hiding this comment.
One possible version of a return from Catalog.list_favorites.
There was a problem hiding this comment.
Reworked from the original to use pytest idioms and fixtures.
NMS and Catalog client fixtures might get moved to conftest.py in a later PR. I forget if they're used in other modules or not.
There was a problem hiding this comment.
The unit version's much simpler and uses the included mock data. It also doesn't validate server results like the integration test does.
test/integration/test_appinfo.py
Outdated
| IGNORE_CATEGORIES = {"inactive", "importers", "viewers"} | ||
| APP_TAGS = ["release", "beta", "dev"] |
There was a problem hiding this comment.
import from appinfo so they will always be guaranteed to be in sync?
| for a in info["app_infos"]: | ||
| app = info["app_infos"][a]["info"] | ||
| for str_key in app_str_keys: | ||
| assert str_key in app |
There was a problem hiding this comment.
add in a check that the value is a str?
test/integration/test_appinfo.py
Outdated
| validate_app_info(info) | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("bad_tag", [None, [], {}, "foo", 5, -3]) |
There was a problem hiding this comment.
Could add in an empty string and a bool, just for fun!
| ) -> None: | ||
| # this is all public info, so just use my username, and i'll keep at least one favorite | ||
| favorite_user = "wjriehl" | ||
| cat_favorites = catalog_client.list_favorites(favorite_user) |
There was a problem hiding this comment.
this sounds way more fun than it actually is!
test/unit/test_appinfo.py
Outdated
| IGNORE_CATEGORIES = {"inactive", "importers", "viewers"} | ||
| APP_TAGS = ["release", "beta", "dev"] |
|
I added the requested changes. Thanks for the review! |
This gives the cleanup / linting / test tidying treatment to the appinfo module.
Minimal changes to the module (other than typing), and tweaks to pytestify the integration tests.