Adding some additional query tests, as well as some options tests. #103
Conversation
There was a problem hiding this comment.
Great work @jrbenny35! 👍
As discussed earlier, can you please check if the root_user change is strictly required? I left a few comments on code formatting, type annotations and comprehensibility.
| @@ -103,10 +103,10 @@ def test_search_for_unpublished_query( | |||
| login_page: LoginPage, | |||
| server_url, | |||
| selenium, | |||
| user: User, | |||
| root_user: User, | |||
There was a problem hiding this comment.
Can you please confirm that this is necessary and if so, might this be a regression in Redash?
pages/queries.py
Outdated
|
|
||
| @property | ||
| def description(self) -> typing.Any: | ||
| return self.find_element(*self._query_description_locator).text | ||
|
|
||
| def dropdown_menu(self, item=None) -> typing.Any: |
There was a problem hiding this comment.
I suggest changing this method name as follow, so that it becomes clear that this performs a UI interaction and doesn't yield the dropdown menu element:
| def dropdown_menu(self, item=None) -> typing.Any: | |
| def click_dropdown_menu(self, item=None) -> typing.Any: |
pages/queries.py
Outdated
| term.click() | ||
| self.find_element(*self._query_modal_archive_locator).click() | ||
| return | ||
| return |
There was a problem hiding this comment.
Can you explain what those if branches do from a users perspective? The first one forks a query and shows the detail page for that query, and the other one archives the query, but doesn't return an element?
There was a problem hiding this comment.
As to the implementation, I suggest to rename the variables slightly and make use of a generator and returns.
def click_dropdown_menu(self, text=None) -> typing.Any:
menu = self.find_element(*self._query_dropdown_menu_locator)
items = menu.find_elements(*self._query_dropdown_menu_item_locator)
menu.click()
for item in (i for i in items if i.text in text):
if text == "Fork":
item.click()
return QueryDetailPage(self.selenium, self.base_url)
if text == "Archive":
item.click()
self.find_element(*self._query_modal_archive_locator).click()
returnThere was a problem hiding this comment.
The "Archive" just archives the query. There can be nothing done after you archive it.
There was a problem hiding this comment.
Maybe I can return a Exception with a message?
pages/queries.py
Outdated
| except Exception: | ||
| self.find_element(*self._query_description_locator).click() | ||
| element = self.find_element(*self._query_description_edit_locator) | ||
| element.send_keys(" {}".format(description)) |
There was a problem hiding this comment.
Python 3.6! 🚀
| element.send_keys(" {}".format(description)) | |
| element.send_keys(f" {description}") |
There was a problem hiding this comment.
Old habits die hard!
pages/queries.py
Outdated
| element = self.find_element(*self._query_name_locator) | ||
| element.click() | ||
| element = self.find_element(*self._query_name_edit_locator) | ||
| element.send_keys(" {}".format(title)) |
There was a problem hiding this comment.
| element.send_keys(" {}".format(title)) | |
| element.send_keys(f" {title}") |
tests/test_queries.py
Outdated
| server_url, | ||
| selenium, | ||
| root_user: User, | ||
| variables, |
There was a problem hiding this comment.
Can you please add type annotations for all parameters that don't are not annotated yet in all tests?
There was a problem hiding this comment.
Can I do this in a following PR?
There was a problem hiding this comment.
Sure, but it would be best to add them along with the new code.
tests/test_queries.py
Outdated
| query = search.queries[0].click() | ||
| assert query.title == query_specs["name"] | ||
| query.edit_title("NEW") | ||
| assert query.title == query_specs["name"] + " NEW" |
There was a problem hiding this comment.
query_specs = variables["default"]["queries"]["edit-query"]
name = query_specs["name"]
search = page.search(name)
query = search.queries[0].click()
assert query.title == name
query.edit_title("NEW")
assert query.title == f"{name} NEW"
|
I spoke to @jezdez on IRC today and was told that the usage of |
pages/queries.py
Outdated
| item.click() | ||
| self.find_element(*self._query_modal_archive_locator).click() | ||
| return | ||
| return Exception(f"{item} was not found within the dropdown menu.") |
There was a problem hiding this comment.
We need to raise this Exception and can probably make this more specific.
| return Exception(f"{item} was not found within the dropdown menu.") | |
| raise ValueError(f"{item} was not found within the dropdown menu.") |
pages/queries.py
Outdated
| def edit_source(self) -> typing.Any: | ||
| return self.find_element(*self._query_edit_source_locator) | ||
|
|
||
| def edit_title(self, title=None): |
There was a problem hiding this comment.
I'd make title required, because a query title of " None" seems unlikely.
| def edit_title(self, title=None): | |
| def edit_title(self, title: str) -> None: |
pages/queries.py
Outdated
| element.send_keys(Keys.ENTER) | ||
|
|
||
| @property | ||
| def edit_source(self) -> typing.Any: |
There was a problem hiding this comment.
I find the names of these methods slightly confusing: edit_description and edit_source suggests that they do similar things. However one is a method that performs UI actions and returns None, the other one is a property and returns the element.
I suggest to use verbs for methods edit_description() and nouns for properties edit_source_button.
What do you think?
There was a problem hiding this comment.
Yeah that makes sense. I was literally calling it what the button was, but adding btn or button was what I had in mind I think, as it is a button.
Fixes #86, #87, #88, #89, #90, #91, #92.