-
Notifications
You must be signed in to change notification settings - Fork 18
Use Firefox instead of Chrome in frontend tests #253
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
Changes from all commits
f73a697
a6cd967
ab4baff
f73d3bf
74da853
0acdb3b
874f74d
571acd5
b2fbfd2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
[submodule "baselayer"] | ||
path = baselayer | ||
url = git://github.com/cesium-ml/baselayer.git | ||
branch = master | ||
branch = firefox | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we merge this into master before continuing? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good - wanna merge my new baselayer PR? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see those tests are failing; do you know why? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @stefanv yeah, because those changes break the cesium_web tests, and the fixes are in this PR. So they'll need to be merged simultaneously There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually we'll need to merge this first, then baselayer, then update this to use the master branch. |
+12 −17 | app/test_util.py |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,46 +11,55 @@ def test_tab_tooltips(driver, project): | |
hover = ActionChains(driver).move_to_element( | ||
driver.find_element_by_id('react-tabs-0')) | ||
hover.perform() | ||
time.sleep(0.8) | ||
time.sleep(1) | ||
assert driver.find_element_by_xpath( | ||
"//span[contains(text(),'Manage your projects')]" | ||
).is_displayed() | ||
|
||
hover = ActionChains(driver).move_to_element( | ||
driver.find_element_by_id('react-tabs-2')) | ||
hover.perform() | ||
time.sleep(0.8) | ||
time.sleep(1) | ||
assert driver.find_element_by_xpath( | ||
"//span[contains(text(),'Upload your time-series data')]" | ||
).is_displayed() | ||
|
||
hover = ActionChains(driver).move_to_element( | ||
driver.find_element_by_id('react-tabs-4')) | ||
hover.perform() | ||
time.sleep(0.8) | ||
time.sleep(1) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we use our standard approach here for awaiting a new element, instead of doing an explicit sleep? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, the problem with that approach is that the element exists before it is displayed. Thus the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps, then, we are checking for the wrong thing. Should we check that the element is visible/selected/contains the expected text/whatever is applicable in each instance instead? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's effectively what we're doing - checking that the element is displayed and contains the expected text. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, but you need to wait until it is updated, otherwise it fails too early. I was hoping for a construct like "wait until this condition is true; but keep trying for at least 5 seconds". Does that exist? I'm merging in the mean time. |
||
assert driver.find_element_by_xpath( | ||
"//span[contains(text(),'Generate features from your time-series data')]" | ||
).is_displayed() | ||
|
||
|
||
def test_file_upload_tooltips(driver, project): | ||
def test_headerfile_upload_tooltip(driver, project): | ||
driver.get('/') | ||
driver.refresh() | ||
driver.find_element_by_id('react-tabs-2').click() | ||
driver.find_element_by_partial_link_text('Upload new dataset').click() | ||
time.sleep(0.2) | ||
|
||
header_file = driver.find_element_by_css_selector('[name=headerFile]') | ||
hover = ActionChains(driver).move_to_element(header_file) | ||
header_file = driver.find_element_by_css_selector('[name="headerFile"]') | ||
hover = ActionChains(driver).click_and_hold(header_file) | ||
hover.perform() | ||
time.sleep(0.8) | ||
time.sleep(1) | ||
assert driver.find_element_by_xpath( | ||
"//span[contains(.,'filename,label')]" | ||
).is_displayed() | ||
|
||
tar_file = driver.find_element_by_css_selector('[name=tarFile]') | ||
hover = ActionChains(driver).move_to_element(tar_file) | ||
|
||
def DISABLED_test_tarfile_upload_tooltip(driver, project): | ||
driver.get('/') | ||
driver.refresh() | ||
driver.find_element_by_id('react-tabs-2').click() | ||
driver.find_element_by_partial_link_text('Upload new dataset').click() | ||
time.sleep(0.2) | ||
|
||
tar_file = driver.find_element_by_css_selector('[name="tarFile"]') | ||
hover = ActionChains(driver).click_and_hold(tar_file) | ||
hover.perform() | ||
time.sleep(0.8) | ||
time.sleep(1) | ||
assert driver.find_element_by_xpath( | ||
"//span[contains(.,'Each file in tarball should be formatted as follows')]" | ||
).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.
I'll change this to
master
when we merge the associatedbaselayer
PR.