-
Notifications
You must be signed in to change notification settings - Fork 9
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @jrbenny35! 👏
One comment on type annotations.
Currently the tests check that the first search result is what we expect. Ideally we also check that the search results don't contain items that do not match the search string. We could do this by additionally assertion the number of results?
What do you think?
tests/test_queries.py
Outdated
@@ -15,14 +16,93 @@ | |||
("Ashleys Query", "Query created by Ashley."), | |||
], | |||
) | |||
def test_query( | |||
def test_query_by_username( | |||
create_queries, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please update this to:
create_queries: typing.Callable[..., None],
I think asserting the number of results could work but what if they query is like 'default query' but there is one named 'default query 1' ? |
That depends on the expected behavior for the search feature of Redash. Thoughts @madalincm? |
Just trying it myself, they both show up. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Fixes #63 #62 #64 #61 #65 #59