Use Firefox instead of Chrome in frontend tests #253
Conversation
acrellin
force-pushed
the
firefox
branch
4 times, most recently
from
4c59fc7 to
d27a5b3
Compare
acrellin
force-pushed
the
firefox
branch
4 times, most recently
from
dac159d to
428a3ad
Compare
|
Hello @acrellin! Thanks for updating the PR. Cheers ! There are no PEP8 issues in this Pull Request. 🍻 Comment last updated on May 15, 2018 at 20:17 Hours UTC |
…obscured by navbar
|
@stefanv there's a mysterious thing happening on Travis where whichever of the two file upload tooltip tests runs last always fails, even when they're separated into separate files. I can't reproduce this locally, and have the same selenium and Firefox/GeckoDriver versions. For now I've disabled one of the two file upload tooltip tests, and everything else is passing now. |
|
@acrellin I think that's the right approach for now; the tooltips aren't important enough to hold back the rest. |
| path = baselayer | ||
| url = git://github.com/cesium-ml/baselayer.git | ||
| branch = master | ||
| branch = firefox |
There was a problem hiding this comment.
I'll change this to master when we merge the associated baselayer PR.
| driver.find_element_by_id('react-tabs-4')) | ||
| hover.perform() | ||
| time.sleep(0.8) | ||
| time.sleep(1) |
There was a problem hiding this comment.
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.
Right, the problem with that approach is that the element exists before it is displayed. Thus the sleep here.
There was a problem hiding this comment.
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.
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.
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.
| path = baselayer | ||
| url = git://github.com/cesium-ml/baselayer.git | ||
| branch = master | ||
| branch = firefox |
There was a problem hiding this comment.
Should we merge this into master before continuing?
There was a problem hiding this comment.
Sounds good - wanna merge my new baselayer PR?
There was a problem hiding this comment.
I see those tests are failing; do you know why?
There was a problem hiding this comment.
@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.
Actually we'll need to merge this first, then baselayer, then update this to use the master branch.
Closes #245