-
-
Notifications
You must be signed in to change notification settings - Fork 151
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
Improved tests structure in files #355
base: master
Are you sure you want to change the base?
Improved tests structure in files #355
Conversation
… element__lazy_search_test.py
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.
please refactor according to comments;)
page = GivenPage(session_browser.driver) | ||
page.opened_empty() | ||
|
||
page.load_body( | ||
''' | ||
<h1 id="will-be-existing-element"> | ||
<span class="will-exist-inner">Hello</span> kitty:* | ||
</h1>''' | ||
</h1> | ||
''' | ||
) | ||
answer = element().is_displayed() |
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.
this is an act, it need an empty line before... (the load_body is not an act, it's an arrange clause)
) | ||
element = session_browser.element( | ||
'#will-be-existing-element' | ||
).element('.will-exist-inner') | ||
page = GivenPage(session_browser.driver) | ||
page.opened_empty() | ||
|
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.
this emply line is wrong, the next page load is also an arrange
) | ||
element = session_browser.element( | ||
'#will-be-existing-element' | ||
).element('.will-exist-inner') | ||
page = GivenPage(session_browser.driver) | ||
page.opened_empty() | ||
|
||
page.load_body( |
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.
you don't need in this case separation between opened_empty() and load_page(), you can use the .opened_with_body() instead, that covers both previous ones
the separation was needed when element = ... was after opened_empty(), but not it is in the beginning of the test...
@@ -57,17 +58,19 @@ def test_search_is_postponed_until_actual_action_like_questioning_displayed( | |||
def test_search_is_updated_on_next_actual_action_like_questioning_displayed( |
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.
please watch this: https://youtu.be/47yhzbiCJSw?t=1147
and refactor the test below, it should have same structure as in video;
assert should be broken into ACT and ASSERT clauses, and all previous page loading including first store of "answer" should be in ARRANGE
also
I would use the update word instead of new for consistency with test name; all this was in video;) watch it!
@@ -92,7 +98,8 @@ def test_search_finds_exactly_inside_parent(session_browser): | |||
<a href="#second">go to Heading 2</a> | |||
<h1 id="first">Heading 1</h1> | |||
<h2 id="second">Heading 2</h2> | |||
/p>''' | |||
/p> |
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.
should be
</p>
<h1 id="will-be-existing-element-id">Hello kitty:* | ||
</h1> | ||
''' | ||
) | ||
|
||
assert element().is_displayed() is True |
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.
should be brokent into ACT + ASSERT with additional variable with answer
@@ -50,11 +59,22 @@ def test_search_is_updated_on_next_actual_action_like_questioning_displayed( | |||
page = GivenPage(session_browser.driver) |
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.
same logic should be applied as in video mentioned earlier
'<h1 id="will-be-existing-element-id" style="display:none">Hello kitty:*</h1>' | ||
''' | ||
<h1 id="will-be-existing-element-id" style="display:none">Hello kitty: | ||
*</h1> |
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.
should be broken with a common sense, not just at some point...
good version is more like this:
<h1 id="will-be-existing-element-id" style="display:none">
Hello kitty:*
</h1>
) | ||
|
||
assert element().is_displayed() is False |
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.
same as in first file...
Improved tests' structure in element__element__lazy_search_test.py and element__lazy_search_test.py