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

Use headless chrome with selenium in docker #1767

Merged
merged 12 commits into from Apr 16, 2018

Conversation

Projects
None yet
2 participants
@toolness
Contributor

toolness commented Apr 13, 2018

This is an alternative to #1758 and #1765 which attempts to just use headless chrome + chromedriver (instead of phantomjs) with selenium.

As I mentioned in Slack:

uh... i feel kinda bad bringing this up ... but yet another option is for us to actually just use chromedriver to make our selenium tests run against headless chrome instead of phantom. from what i could tell it seemed like the real problem w/ our selenium tests was phantomjs' implementation of the webdriver protocol, not selenium itself. blerg.

The one big advantage I still see with Selenium is the fact that it's a full-on integration test that allows us to easily switch between the front-end browser (via selenium) and the back-end db (via the LiveServerTestCase that runs the test). That ensures that our back-end and front-end are cooperating in the ways we expect, which we can't really get from the standard jest tests.

To do

  • Remove support for phantomjs (both in code and docs).
  • Make CircleCI log the version of Chrome and chromedriver in use.
  • Either ensure that pytest data_capture/tests/test_integration.py works with headless chrome, or remove support for selenium and just run those tests using robobrowser.
  • Now that this works, I am extremely confused as to why our aXe tests seem to pass here, but don't on #1758. We should investigate this to ensure that the aXe tests are actually running properly. aXe tests work here because frontend.tests.axe.IGNORED_VIOLATIONS contains color-contrast, whereas I think #1758 paid attention to all violations. If we remove color-contrast then our aXe tests fail with contrast violations, so things are definitely working as expected here. The color contrast exception was even filed as #1553 so we don't need to create any new issues for this.
  • Ensure CircleCI is running selenium tests against headless chrome
  • Add docs for new environment variables/settings/etc:
    • Specifically, document WD_CHROME_ARGS.
    • Consider just removing the Example: Docker on OS X and Running the tests on your machine's Chrome browser sections from docs/selenium.md. The first doesn't make sense since we're already using headless Chrome by default now, and the second doesn't really make sense in light of #1743.

toolness added some commits Apr 13, 2018

@toolness toolness referenced this pull request Apr 13, 2018

Closed

[WIP] Attempt to operationalize puppeteer #1765

0 of 3 tasks complete
@toolness

This comment has been minimized.

Contributor

toolness commented Apr 16, 2018

@jseppi this is basically done, but I could use some advice on what to do about selenium.md. Given that testing on Chrome is a pretty good default, and that we don't even support Sauce Labs anymore, I'm tempted to just remove support for testing on any non-Chrome browser, as it would simplify our documentation and testing implementation. (That said, in the future we could potentially add support for headless Firefox so that CircleCI runs selenium tests against both Chrome and Firefox.)

What do you think?

raise Exception('No such webdriver: "%s"' % name)
class SeleniumTestCase(StaticLiveServerTestCase):
connect = None
driver = None
screenshot_filename = 'selenium_tests/screenshot.png'
screenshot_filename = 'selenium_tests_screenshot.png'

This comment has been minimized.

@toolness

toolness Apr 16, 2018

Contributor

I'm not sure what behavior was like before, but right now specifying a non-existent directory (selenium_tests) doesn't make that directory be created, so the screenshot doesn't get written. By making this screenshot just a filename, we ensure the screenshot is actually written.

This comment has been minimized.

@jseppi

jseppi Apr 16, 2018

Contributor

Is this functionality even needed any more? It doesn't look like it would be that helpful, so maybe even consider removing it.

This comment has been minimized.

@toolness

toolness Apr 16, 2018

Contributor

Hmm, I think it's useful to see what a browser is "seeing" when a test fails. It was actually useful when I was debugging the failure here in this very PR.

Er wait, not that one... the one involving safe mode. I assumed something bad was happening that was triggering the "do you want to try safe mode?" but once I looked at the screenshot, I realized it was already in safe mode.

This comment has been minimized.

@jseppi

jseppi Apr 16, 2018

Contributor

Ah, ok. Not a problem to keep it :)

self.form.submit()
WebDriverWait(self.driver, 10).until(staleness_of(old_page))

This comment has been minimized.

@toolness

toolness Apr 16, 2018

Contributor

This basically assumes that there's a full page refresh when a form is submitted, i.e. we don't do some fancy ajax thing. I thought this was the default behavior of a selenium form submission but I guess not.

@jseppi

This comment has been minimized.

Contributor

jseppi commented Apr 16, 2018

Re: Your selenium.md question - I think making it simpler (ie, only supporting chromium for now) is a good course of action.

@jseppi

Looking really nice!

@@ -2,11 +2,11 @@ version: 2.0
default-docker-config: &default-docker-config
docker:
- image: circleci/python:3.6
- image: circleci/python:3.6-browsers

This comment has been minimized.

@jseppi

jseppi Apr 16, 2018

Contributor

Whoa, didn't know about these CircleCI docker variants. Very useful!

@@ -26,7 +25,14 @@ def autologin(request):
if user is None:
raise Exception('Unable to authenticate')
django.contrib.auth.login(request, user)
request.session[safe_mode.SESSION_KEY] = True
# We used to enable safe mode to reduce the chance of JS race conditions

This comment has been minimized.

@jseppi

jseppi Apr 16, 2018

Contributor

Do you think we'll re-enable this at some point? If not, maybe just strike this comment and commented-out code since it probably won't make much sense to future developers.

This comment has been minimized.

@toolness

toolness Apr 16, 2018

Contributor

I think we might want to... The original reason I enabled it was because I was worried that Web Components' asynchronous upgrading mechanism might mess with the tests, and I also figured it'd be useful to explicitly ensure that the flow worked with JS disabled. Ah well.

toolness added some commits Apr 16, 2018

@toolness toolness changed the title from [WIP] Use headless chrome with selenium in docker to Use headless chrome with selenium in docker Apr 16, 2018

@toolness

This comment has been minimized.

Contributor

toolness commented Apr 16, 2018

@jseppi ok I think this is good to go!

@jseppi

jseppi approved these changes Apr 16, 2018

This looks great! 🥇
I made a few fairly minor comments that should be quick to address, then merge away!

@@ -14,7 +14,7 @@ venv
ghostdriver.log
*.log
node_modules
selenium_tests/screenshot.png
selenium_tests_screenshot.png

This comment has been minimized.

@jseppi

jseppi Apr 16, 2018

Contributor

might want to make the same change to .cfignore and .dockerignore

* `WD_CHROME_ARGS` are command-line flags to pass to Chrome,
e.g. `--headless --no-sandbox --disable-setuid-sandbox`.
* `TEST_WITH_ROBOBROWSER` is a boolean that indicates whether to run

This comment has been minimized.

@jseppi

jseppi Apr 16, 2018

Contributor

do you think we still need RoboBrowser? I see we still have TEST_WITH_ROBOBROWSER=yup in our CircleCI config.yml.

Maybe since Phantom was the underlying problem, those tests will now be fast enough to just get rid of this setting and dependency+code altogether.

This comment has been minimized.

@toolness

toolness Apr 16, 2018

Contributor

Can we do that in a separate PR? There's actually a fair amount of abstraction and goop in there that I'd rather just do in a separate PR... Harumph.

The other possibility is to actually bust this up using both Selenium and RoboBrowser. The former case--especially since we're no longer enabling safe mode--will ensure that things work in a browser with JavaScript, while the latter will ensure that things still work with JS disabled... ehh.

This comment has been minimized.

@jseppi

jseppi Apr 16, 2018

Contributor

Sure!

@@ -15,6 +15,7 @@
from django.contrib.staticfiles.testing import StaticLiveServerTestCase
from selenium import webdriver
from selenium.webdriver.chrome.options import Options as ChromeOptions

This comment has been minimized.

@jseppi

jseppi Apr 16, 2018

Contributor

There are a bunch of comments at the top of this file about intermittent failures on Travis. Can those be removed now?

This comment has been minimized.

@toolness

toolness Apr 16, 2018

Contributor

oh sure might as well remove em.

@toolness toolness merged commit 095f808 into develop Apr 16, 2018

3 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
codeclimate All good!
Details
codeclimate/total-coverage 92% (0.0% change)
Details

@toolness toolness deleted the chrome-selenium branch Apr 16, 2018

@jseppi jseppi referenced this pull request May 14, 2018

Closed

Review and update developer documentation #1698

10 of 15 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment