Skip to content
This repository has been archived by the owner on Nov 8, 2018. It is now read-only.

Add a functional test for the to service interstitial #46

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions tests/functional/test_to_service_interstitial.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
import re

from bs4 import BeautifulSoup
from flask import url_for
import pytest


def request(url, method):
r = method(url)
r.soup = BeautifulSoup(r.get_data(as_text=True), 'html.parser')
return r


class When_on_to_service_interstitial(object):

@pytest.fixture(autouse=True)
def setup_page(self, client, app):
self.config_countdown = app.config['META_REFRESH_DELAY']
Copy link
Contributor

Choose a reason for hiding this comment

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

This variable could be better named - config_countdown sounds like a verb. Perhaps delay?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok


self.service_page_redirect = 'to-service-page'

with client.session_transaction() as session:
session['auth_redirect'] = self.service_page_redirect

self.response = request(url_for('main.to_service'), client.get)

def it_has_continue_link_href_set_to_service_page(
self, client):
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to break this line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

next_li = self.response.soup.find("li", class_="next")

assert self.service_page_redirect == next_li.find('a')['href']
Copy link
Contributor

Choose a reason for hiding this comment

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

The method to find next_li is too implementation specific.

When writing CSS selectors, it's better to be only as specific as necessary, but no more. Specifying that the "next" link appears in a li tag here is unnecessarily specific, and will break if the template changes to use a span, for example. It's better to use id and class attributes to find elements.

There's also no need to find the li first, and then the a within it - you can find the a element directly, eg:

next_link = self.response.soup.find('.next a')
assert self.service_page_redirect == next_link['href']

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, changed find to select_one and got it working


def it_has_countdown_set_to_the_meta_refresh_config(self, config):
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't look like the config fixture is used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, removed

countdown_text = self.response.soup.select_one(
"#content span.pagination-text span").text
Copy link
Contributor

Choose a reason for hiding this comment

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

This CSS selector is both too specific (#content is unnecessary and span.pagination-text doesn't need to specify span.) and not specific enough (without an id or class the last span is not specific enough, and requires the previous selectors to provide context).

This should be

countdown_text = self.response.soup.find('.redirect-timer').text`

Also, redirect_timer_text might be a better variable name, since the word "countdown" does not appear anywhere except in this test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok


countdown_int = int(re.match(r'\d+', countdown_text).group())

assert countdown_int == self.config_countdown
Copy link
Contributor

Choose a reason for hiding this comment

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

Parsing the number out of the countdown_text will cause this test to error, rather than fail, if the text contains no digits.

Instead, cast the config_countdown integer to a string and either compare that to the regex match, or avoid the overhead of regular expressions and just check that the countdown_text contains the config_countdown number, eg:

assert "{} seconds".format(self.config_countdown) in countdown_text

Ideally, this test should check for something other than the existence of specific text on the page, as the copy can change without affecting the functionality of the app.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok