Misc test cleanup #1445

psivesely · 2016-11-04T03:42:18Z

Updates some syntax.
Imports config after setting SECUREDROP_ENV.
Gets the test suite working again by fetching an old version of Firefox from the Ubuntu archives that's still compatible with the last Selenium 2 release. We want to avoid switching to Selenium 3 before the Aaron Swartz day Hackathon this weekend.
Replaces a hardcoded path with a config attr.
Removes a couple test methods and a lot of TOTP values that were doing nothing because the verification is mocked.

Flask Testing maintains some `assert_foo` methods that redirect to the assertFoo syntax used by the `unittest.TestCase` class on which it's own `TestCase` class is based (I presume at one point they decided to standardize, but didn't want to break older code like ours). For consistency, I believe the camelcase syntax is preferable.

* Replaces TOTP token values with 'mocked' to make it clear it we're mocking TOTP verification. * Removes functions that test TOTP verification.

Selenium 3 makes breaking API changes that require Mozilla's geckodriver. We're going to hold off on supporting this for now. I've pinned Selenium at the latest 2.X release (2.53.6), and written Ansible code to download and install Ubuntu 46.0.1, which is the latest Firefox version that version of Selenium supports.

Since normally we run unit tests from manage.py this bug was never caught--in interactive cases where you might not be using manage.py, the config attributes were getting set mostly as in prod (if you don't set SECUREDROP_ENV you actually get a result that not's exactly any of the configuration settings).

redshiftzero · 2016-11-04T18:05:27Z

Hey @fowlslegs thanks for taking on the functional tests! I tried to provision a development VM using this branch but ran into some dependency issues installing Firefox 46:

TASK [app-test : Install Firefox 46.0.1 for compatibility with Selenium 2.53.6.] ***
fatal: [development]: FAILED! => {"changed": true, "cmd": ["dpkg", "-i", "/home/vagrant/firefox_46.0.1+build1-0ubuntu0.14.04.3_amd64.deb"], "delta": "0:00:04.272178", "end": "2016-11-04 17:57:32.565899", "failed": true, "rc": 1, "start": "2016-11-04 17:57:28.293721", "stderr": "dpkg: dependency problems prevent configuration of firefox:\n firefox depends on libasound2 (>= 1.0.16); however:\n  Package libasound2 is not installed.\n firefox depends on libcairo-gobject2 (>= 1.10.0); however:\n  Package libcairo-gobject2 is not installed.\n firefox depends on libgtk-3-0 (>= 3.4); however:\n  Package libgtk-3-0 is not installed.\n firefox depends on libstartup-notification0 (>= 0.8); however:\n  Package libstartup-notification0 is not installed.\n\ndpkg: error processing package firefox (--install):\n dependency problems - leaving unconfigured\nErrors were encountered while processing:\n firefox", "stdout": "Selecting previously unselected package firefox.\n(Reading database ... 39240 files and directories currently installed.)\nPreparing to unpack .../firefox_46.0.1+build1-0ubuntu0.14.04.3_amd64.deb ...\nUnpacking firefox (46.0.1+build1-0ubuntu0.14.04.3) ...\nProcessing triggers for mime-support (3.54ubuntu1.1) ...\nProcessing triggers for man-db (2.6.7.1-1ubuntu1) ...", "stdout_lines": ["Selecting previously unselected package firefox.", "(Reading database ... 39240 files and directories currently installed.)", "Preparing to unpack .../firefox_46.0.1+build1-0ubuntu0.14.04.3_amd64.deb ...", "Unpacking firefox (46.0.1+build1-0ubuntu0.14.04.3) ...", "Processing triggers for mime-support (3.54ubuntu1.1) ...", "Processing triggers for man-db (2.6.7.1-1ubuntu1) ..."], "warnings": []}

conorsch · 2016-11-04T18:07:16Z

install_files/ansible-base/roles/app-test/tasks/dev_setup_xvfb_for_functional_tests.yml

+    # /root. Since this command doesn't need to be run as root and is part of a
+    # crutch anyway, I've just hardcoded /home/vagrant.
+    dest: /home/vagrant/
+    url: https://launchpad.net/~ubuntu-mozilla-security/+archive/ubuntu/ppa/+build/9727836/+files/firefox_46.0.1+build1-0ubuntu0.14.04.3_amd64.deb


Let's get a checksum on this task to validate the download.

It is signed.

It is signed.

The associated Release file in the origin repo is signed, but these tasks perform no signature verification. Confirm the SHA256 checksum manually and hardcode it in the get_url task.

Okay, there is an option

--no-debsig Do not try to verify package signatures.

in the dpkg man page, but I am remembering now that dpkg doesn't actually verify signatures, and I don't know what that flag is about.

conorsch · 2016-11-04T18:10:54Z

install_files/ansible-base/roles/app-test/tasks/dev_setup_xvfb_for_functional_tests.yml

+    - apt
+
+- name: Install Firefox 46.0.1 for compatibility with Selenium 2.53.6.
+  command: dpkg -i /home/vagrant/firefox_46.0.1+build1-0ubuntu0.14.04.3_amd64.deb


The apt module supports a deb parameter, so you don't need to invoke dpkg via command.

conorsch · 2016-11-04T18:44:44Z

As @redshiftzero reported, Firefox installation fails due to lack of dependencies. Folded error output:

TASK: [app-test | Install Firefox 46.0.1 for compatibility with Selenium 
2.53.6.] ***
failed: [development] => {"changed": true, "cmd": ["dpkg", "-i", 
"/home/vagrant/firefox_46.0.1+build1-0ubuntu0.14.04.3_amd64.deb"], "delta": 
"0:00:05.810106", "end": "2016-11-04 18:21:13.372646", "rc": 1, "start": 
"2016-11-04 18:21:07.562540", "warnings": []}
stderr: dpkg: dependency problems prevent configuration of firefox:
 firefox depends on libasound2 (>= 1.0.16); however:
  Package libasound2 is not installed.
 firefox depends on libcairo-gobject2 (>= 1.10.0); however:
  Package libcairo-gobject2 is not installed.
 firefox depends on libgtk-3-0 (>= 3.4); however:
  Package libgtk-3-0 is not installed.
 firefox depends on libstartup-notification0 (>= 0.8); however:
  Package libstartup-notification0 is not installed.

dpkg: error processing package firefox (--install):
 dependency problems - leaving unconfigured
Errors were encountered while processing:
 firefox
stdout: Selecting previously unselected package firefox.
(Reading database ... 41302 files and directories currently installed.)
Preparing to unpack .../firefox_46.0.1+build1-0ubuntu0.14.04.3_amd64.deb ...
Unpacking firefox (46.0.1+build1-0ubuntu0.14.04.3) ...
Processing triggers for mime-support (3.54ubuntu1.1) ...
Processing triggers for man-db (2.6.7.1-1ubuntu1) ...

FATAL: all hosts have already failed -- aborting

psivesely · 2016-11-04T18:59:45Z

Okay, two ways to solve:

Set failed_when: false to the dpkg (soon to be apt module) task. And then run sudo apt-get install -f in the next task.
Install those 4 dependencies first.

I think 2 is probably cleaner for Ansible, even though I would use 1 for convenience on my own laptop.

conorsch · 2016-11-04T19:13:42Z

I think 2 is probably cleaner for Ansible

Agreed. Approach 1 would be clean with use of Ansible 2's block syntax, but alas, we are on Ansible v1 (cc #1146).

psivesely · 2016-11-04T19:21:50Z

Ready for re-review. Working locally for me.

redshiftzero

Looks good to me @fowlslegs! Thanks for these changes. Functional tests are working again 👍 Just remove the one line I noted in functional/functional_test.py and we can get this merged.

redshiftzero · 2016-11-04T21:10:10Z

securedrop/tests/functional/functional_test.py

@@ -11,6 +11,8 @@
 import sys


+# Set environment variable so config.py uses a test environment
+os.environ['SECUREDROP_ENV'] = 'test'


I see why you're moving this here, but remove the old setting of the environmental variable on line 29

Good catch.

redshiftzero · 2016-11-04T21:26:43Z

securedrop/tests/functional/journalist_navigation_steps.py

-        otp = self.new_user['orm_obj'].totp.at(
-            datetime.datetime.now() + datetime.timedelta(seconds=interval))
-        self._check_login_with_otp(otp)
-


This test would always pass because it's mocked, and it looks like the underlying functionality - the valid_window option of verify which we now use for this check - is tested in pytop, so good to remove 👍

redshiftzero · 2016-11-04T21:32:46Z

securedrop/tests/common.py

@@ -5,6 +5,8 @@

 import gnupg

+# Set environment variable so config.py uses a test environment
+os.environ['SECUREDROP_ENV'] = 'test'


At some point, we might find another way to do this. We have this set in a few different places (also migrating this code above these module-level imports makes this file no longer PEP8 compliant - bad if we want to go in the direction of #886). Perhaps we could migrate some of the environmental variable setting necessary for our test running elsewhere (e.g. in manage.py or in the test runner (tox or pytest-env)). For now let's just leave as is and then come back after the hackathon.

I think all environment variables need to be removed. Instead, the relevant arguments should be passed directly, such as to object constructors. See, e.g., the TestDatabase mixin class I recently added to help solve this problem in FPSD. The SecureDrop story will be a little more complicated, and I haven't figured it all out yet, but I plan to make a PR for this. Should maybe open an issue.