Skip to content
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

Enable JavaScript testing with Poltergeist #230

Merged
merged 2 commits into from
Feb 7, 2017
Merged

Conversation

alecgibson
Copy link
Contributor

This Pull Request enables JavaScript testing in Smokey with the Poltergeist web driver.

It does this in two steps:

  1. Unrevert @tijmenb 's original PR achieving this with Capybara-Webkit
  2. Switch the web driver over to Poltergeist.

The change to Poltergeist has been made in the hopes of improved stability. For example, with Capybara-Webkit the Scenario Signon cookies are marked as secure and HttpOnly would fail intermittently. This was reproducible on a Development environment running against the Integration server.

The (immediate) cause was that Capybara-Webkit would not populate the Capybara::Session object correctly (for example, the body, headers and status code of the response would be missing, but the title would be present, and a screenshot could be output displaying the expected page). I could not figure out anything deeper than this cause (which would presumably involve rooting around in Capybara-Webkit itself).

In an attempt to improve stability, I have changed the web driver to Poltergeist. Running under the same conditions, I have not been able to reproduce the previously failing intermittent error.

Trello: https://trello.com/c/WIunrfiT/322-investigate-turning-on-javascript-in-smokey

This reverts commit 123c54d, reversing
changes made to e1f195e.
@@ -132,5 +132,4 @@ Feature: Whitehall
And I force a varnish cache miss
And I am benchmarking
When I visit "/banknote"
# Redirects are transparently followed, so the end status code is 200
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test shouldn't be modified, for "reasons".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know, I've sent @davidillsley a Slack (and CC here for good measure)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I already talked to him a couple of weeks ago. Let's not change it.

Copy link
Contributor Author

@alecgibson alecgibson Feb 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I have to change it. For some reason, Poltergeist will successfully redirect pages, but won't update the status code from 301 to 200. As pointed out here, integration tests like this should be using a website how a user would use a website - and that doesn't include looking at status codes. (Arguably better practice still than checking the path is checking for a known element on the page to ensure it's actually been rendered).

Why can't we change it?


When /^I inject a JavaScript error on the page, Smokey raises an exception$/ do
expect { page.driver.execute_script('1.error') }.to raise_error
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No trailing line here (we should add govuk-lint to this repo).


@normal
Scenario: Smokey detects JS errors
When I inject a JavaScript error on the page, Smokey raises an exception
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is clever!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nicely done!

@alecgibson alecgibson force-pushed the capybara-webkit branch 2 times, most recently from 8cc6c02 to 700355c Compare February 2, 2017 09:43
tijmenb added a commit to alphagov/govuk-puppet that referenced this pull request Feb 2, 2017
alphagov/smokey#230 will make smokey use
phantomjs (via poltergeist).
Copy link
Contributor

@tijmenb tijmenb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, almost good to go.

alphagov/govuk-puppet#5427 will have to run on production before we merge this though.

jenkins.sh Outdated
bundle install --path "${HOME}/bundles/${JOB_NAME}" --deployment --quiet
RESTCLIENT_LOG="log/smokey-rest-client.log" govuk_setenv default bundle exec rake $MYTASK
bundle install --path "${HOME}/bundles/${JOB_NAME}" --deployment
RESTCLIENT_LOG="log/smokey-rest-client.log" govuk_setenv default xvfb-run -a bundle exec rake $MYTASK
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

xvfb-run -a is no longer necessary. It was added to make the tests run on webkit.

@alecgibson
Copy link
Contributor Author

@tijmenb is this good to go?

@@ -1,24 +1,23 @@
Given /^I am testing in an EFG context$/ do
# Not sure if there is a better way to do this?
page.driver.browser.agent.add_auth(efg_base_url, ENV['AUTH_USERNAME'], ENV['AUTH_PASSWORD'])
# TODO: remove me
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was this forgotten?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure - @tijmenb ?

Copy link
Contributor

@tijmenb tijmenb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Puppet has been deployed, this is good to go now!

@tijmenb tijmenb merged commit 45187f8 into master Feb 7, 2017
carvil added a commit that referenced this pull request Feb 7, 2017
This reverts commit 45187f8, reversing
changes made to c935d35.
carvil added a commit that referenced this pull request Feb 7, 2017
This reverts commit 45187f8, reversing
changes made to c935d35.
carvil added a commit that referenced this pull request Feb 7, 2017
Revert "Merge pull request #230 from alphagov/capybara-webkit"
@alecgibson alecgibson deleted the capybara-webkit branch February 7, 2017 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants