Refactor tests/common.py #1474

psivesely · 2016-11-14T22:27:54Z

To be rebased after #1467 is merged!

Rebase + refactor tests for new download functionality
This rebases the new download unread and download all functionality in the index
view of the journalist app on the current develop, and in the process does some
minor refactoring of the test suite.

Organizes imports: first come external package imports, then the
SECUREDROP_ENV environment variable is set, and finally the SD module imports.
They are now alphabetized. Also removes some unused imports, and additional
addition or removal was done as necessary to support the refactor.
Prefers use of APIs for object construction versus explicit assignment. For
example, a call to :function:crypto_util.genrandomid() is preferred to the
assignment sid = 'somestring'.
Class-based rewrite of common.py: in order to better organize the
functionality, and encourage better OOP and DRY practices, all the loose
functions in securedrop/tests/common.py have become (mostly) static or class
methods various organizational classes. This allowed us to write a lot of
code.
More accurate test method names: a lot of test method names were vaguely
descriptive. A common motif was to just end test names in 'success.' It's
better to name a method test_admin_login_redirects_to_index if it just tests
that after successful login you're redirected to the index page, than
test_admin_login_success. There are many things that should happen on
success--:obj:flask.g should be set correctly, calls to the database should
be made, session should be set in a client-side cookie, and that cookie should
be set--the list goes on. Let's be specific about which thing that's supposed
to happen happened successfully. (Side note: we do really need to do more
comprehensive tests in a lot of places where we only tests that the app
redirects correctly, instead of checking that all state and context is
modified--or not--in the expected way).
Better async support: The new :class:tests.common.Async provides a little
bit better asynchronous functionality for a variety of situations, and
deprecates the use of sleep statements.
General cleanup: PEP8 stuff, and nicer formatting--some other minor touches.

redshiftzero · 2016-11-29T17:52:05Z

securedrop/tests/common.py

+                  initialized. The second, their password string.
+        """
+        username = crypto_util.genrandomid()
+        user_pw = crypto_util.genrandomid()


As a general principle of software testing, these unit tests should be deterministic. The issue with using random data in a unit test is that it makes tracking down test failures more difficult because the test data is changing every time the test suite is run. To ensure people are seeing the same test data we should either hardcode a random seed (if possible) or pick specific values to test

Don't think it's a big deal in this case, but we can hard-code a seed with random.seed() in common.SetUp.setup(). I'd rather do that and make the API calls for everything rather than hardcoding in actual values in these cases, as it is a better way to test functions are working in conjunction with each other as expected. This was also, if any of the API calls we make change, we don't have to change every fixture such that it could be a state produced by calls to those functions.

Yep, specifying a random seed sounds good 👍

Can't seed the PyCrypto RNG which is used for genrandomid. The more important RNG IMO to seed is the random RNG in test_unit_journalist.py, so I did that instead.

redshiftzero · 2016-12-06T18:18:47Z

Ping whenever this one is ready for re-review - happy to look at it!

psivesely · 2016-12-08T23:39:39Z

Unfortunately there's this bug that's only surfacing in Travis. I'm making some debugging commits I'm going to clean up later, but I don't know how better to do this, so it's happening.

psivesely · 2016-12-09T23:20:38Z

Ready for review now.

redshiftzero

Overall looks really good @fowlslegs! There are some nice improvements to the test suite in here. I’ve stepped through this and added a few final comments in line and then this should be good to merge in (pending comments from anyone else who wants to look at this).

Generic feedback:

Your point regarding the overuse of testing for redirects is a fair one, we should definitely try to fix these where possible in the code.
Being more specific about the expected state in the unit test name is good, but we should make sure that we are adding information to the test name and not being so precise as to obscure the general intent of the test. login_valid_journalist_user_success seems like a decent test name imho: it indicates the function under test and the expected behavior. If it’s only testing a redirect then adding a check_for_redirects or something similar is a good improvement. I might take a quick pass through the renaming of test names and check that they are adding information (I've pointed a couple of cases out in inline comments).
Comment on PEP8, minor formatting, alphabetization of imports: all these things are fine, but we should really establish a consistent coding style and automate these checks as much as possible (writing out Include a style guide for contributors #47 to include things like alphabetizing imports if that's something we care about (I don't care tbh but if others do then I'll roll with it), merging Make securedrop PEP8-compliant (mostly) and add PEP8 pre-commit hook #1471) in order to minimize the contributor and maintainer energy spent on code style.

redshiftzero · 2016-12-12T22:37:41Z

securedrop/tests/test_journalist.py


    @classmethod
    def tearDownClass(cls):
        # Reset the module variables that were changed to mocks so we don't
        # break other tests
        reload(journalist)

-
+seshy = None


What is this for.. ?

But you added this, no?

Maybe it got accidentally pasted in there since it's out of context. Anyway, removed now.

redshiftzero · 2016-12-12T22:42:02Z

securedrop/tests/test_journalist.py

-    def test_login_with_valid_length_password_calls_scrypt(
-            self, mock_scrypt_hash, mock_valid_password):
-        self.login(self.username, self.password)
+    def test_login_calls_scrypt(self, mock_scrypt_hash, mock_valid_password):


This rename seems to make the unit test less clear?

Just trying to make the convention of using login or, abstracted, just the action (here login) when the action will be successful, and only adding additional words to test names to when there is a problem with the action.

Maybe test_valid_login_calls_scrypt? There are multiple unit tests for login testing scrypt calls, good to be precise here

redshiftzero · 2016-12-12T22:43:25Z

securedrop/tests/test_journalist.py

-    def test_login_with_invalid_length_password_doesnt_call_scrypt(
-            self, mock_scrypt_hash):
-        print "test_login_with_invalid_length_password_calls_scrypt"
+    def test_invalid_login_doesnt_call_scrypt(self, mock_scrypt_hash):


What about: test_invalid_length_password_login_doesnt_call_scrypt? Seems like again the original name here was very clear, no reason to remove information from the test name afaict.

I can change it back.

redshiftzero · 2016-12-12T22:44:56Z

securedrop/tests/test_unit_integration.py

-            rv = source_app.get('/generate')
-            rv = source_app.post('/create', follow_redirects=True)
+            resp = source_app.get('/generate')
+            resp = source_app.post('/create', follow_redirects=True)


Good change, clearer as resp 👍

redshiftzero · 2016-12-12T23:13:11Z

securedrop/tests/test_unit_source.py


        with self.client as c:
-            rv = self.client.post('/login', data=dict(codename='invalid'),
+            resp = self.client.post('/login', data=dict(codename='invalid'),


Should this be resp = c.post('/login', data=dict(codename='invalid'), follow_redirects=True)? I know you didn't change this, but since it's in the diff... 🐭

I think it works either way, but I can change it because it's not a sensible block.

redshiftzero · 2016-12-12T23:48:17Z

securedrop/tests/common.py

+class TestJournalist:
+    """Test fixtures for working with :class:`db.Journalist`.
+    """
+    @staticmethod


Why not use __init__() in these TestObject classes?

They're more of a way to sub-namespace functionality. I could split the code up into sub-modules to achieve the same affect.

Not sure what you're suggesting as there are many ways I could use __init__(). Would TestJournalist be a subclass of db.Journalist with some special methods or?

If there's no need for __init__() and only static methods, why are these classes? Why is this better than module-level functions e.g. in the relevant files? Apologies if I'm missing something here and this is a pattern I am not aware of

Because I find the namespacing helpful. The way I would change it is to break this into a few different modules: tests.utils.async, tests.utils.journalist, etc.. I find it organizationally useful and think it will encourage code reuse.

One of the problems with the old common module is that it was really disorganized, and people weren't using it well (I presume partly for this reason), or were rewriting the same utilities elsewhere.

redshiftzero · 2016-12-12T23:58:09Z

securedrop/tests/common.py

+        return TestJournalist.init_journalist(True)
+
+    @staticmethod
+    def mock_verify_token(self):


Is the self arg left in from the earlier code (this is now a static method)?

Nvm--answered too quick. It's supposed to be there. Look at how it's used in test_journalist and test_unit_journalist. Should probably rename the variable to app.

Yep rename it to be something more clear, best to avoid self as an argument in static methods

redshiftzero · 2016-12-13T00:01:05Z

securedrop/tests/common.py

 import subprocess
-
-import gnupg
+import time


In PEP8, standard library imports come before third party imports (e.g. time before gnupg)

redshiftzero · 2016-12-13T00:08:14Z

securedrop/tests/test_unit_journalist.py


-    def test_admin_authorization_for_posts(self):
+    def test_admin_page_restrction_http_posts(self):


Spelling mistake: test_admin_page_restriction_http_posts

redshiftzero · 2016-12-13T00:24:44Z

securedrop/tests/test_unit_journalist.py

-
-        self.assertIn('Passwords didn', res.data)
-
-    def test_admin_add_user_failure_password_too_long(self):


Did this test go away?

No I renamed it test_admin_edits_user_password_too_long_warning since it doesn't test that the operation actually fails (i.e., that the database record is unmodified)--it only checks a warning is shown. Think it's important to be specific about that sort of thing.

redshiftzero · 2016-12-14T01:12:16Z

securedrop/tests/test_unit_store.py

+        source, _ = TestSource.init_source()
+        old_journalist_filename = source.journalist_filename
+        old_filename = TestSubmission.submit(source, 1)[0].filename
+        new_journalist_filename = 'nestor_makhno'


psivesely · 2016-12-14T01:45:17Z

I'm going to split common up into a utils namespace, with several sub-packages instead of abusing staticmethod.

psivesely · 2016-12-16T23:04:47Z

Ready for re-review.

psivesely · 2016-12-16T23:10:05Z

Woops forgot to fix the functional tests as well. Impending commit!

psivesely · 2016-12-16T23:12:08Z

Okay, this should pass Travis in a minute. Has been rebased on the latest develop at time of writing.

psivesely · 2016-12-16T23:12:48Z

Could use another rebase just to clean up the git history, but I wanted to preserve it until this PR gets approved.

psivesely · 2016-12-19T19:20:15Z

Rebased on the latest develop.

conorsch · 2016-12-20T19:52:43Z

Can't get all the tests to pass locally on this branch; getting consistent failures on a redis worker timeout. Full test output here: https://gist.github.com/conorsch/bed571f440da3cfcb4d0edd8455c82ce

Are you able to run the entire test suite locally and have all tests pass, @fowlslegs?

psivesely · 2016-12-20T20:00:15Z

Yes. As a rule, I won't say a PR is ready for review unless it is passing all tests locally and on Travis.

redshiftzero · 2016-12-20T20:35:43Z

Hmm... just provisioned a VM on this branch, ran the test suite a few times and everything seems to be passing for me (no Redis worker timeouts).

conorsch · 2016-12-20T20:38:50Z

@redshiftzero Thanks for testing! That's good enough for me, I say good ahead and mark as approved. I'll step through the logs on my local VMs if it happens again. Given that you two and also Travis agree the tests are passing, we can be pretty confident these aren't breaking changes.

As for the actual subject of the PR, the separation looks clean, and in general the tests are a bit more readable. Hopefully these changes lower the bar for devs updating the tests in the future.

LGTM! 🏁

redshiftzero · 2016-12-20T20:49:35Z

Just gave the changes another look - this is much better, lots of good improvements. Let's get this merged in - @fowlslegs can you rebase and clean up the git history?

* Namespaces utilities: common.py was added to and then divided into the `utils` sub-package containing the following modules: * `env`: utilities related to setup and teardown of the test environment including temporary filesystems, keyrings, and databases. * `db_helper`: utilities related to easily creating database records via API calls. * `async`: utilities related to waiting for/ blocking on asynchronous processes (e.g., Redis workers, python-gnupg daemon threads, etc.), checking return status, and throwing errors when asynchronous process fail. In the process a lot of the tools from common.py were re-written and improved in order to keep their use-case as general as possible. Some utilities from test modules were moved into this sub-package as well. This should encourage code re-use and good OOP practices. * Organizes imports: imports now follow PEP8 as best as possible (I think we'll have to get rid of config-as-a-module before we can be fully PEP8-compliant for imports). Unused imports have been removed. * Prefers use of APIs for object construction versus explicit assignment. For example, a call to :function:`crypto_util.genrandomid()` is preferred to the assignment `sid = 'somestring'`. * More accurate test method names: a lot of test method names were vaguely descriptive. A common motif was to just end test names in 'success.' It's better to name a method `test_admin_login_redirects_to_index` if it just tests that after successful login you're redirected to the index page, than `test_admin_login_success`. There are many things that should happen on success--:obj:`flask.g` should be set correctly, calls to the database should be made, session should be set in a client-side cookie, and that cookie should be set--the list goes on. Let's be specific about which thing that's supposed to happen happened successfully. (Side note: we do really need to do more comprehensive tests in a lot of places where we only tests that the app redirects correctly, instead of checking that all state and context is modified--or not--in the expected way). * General cleanup: PEP8 stuff, and nicer formatting--some other minor touches. * Since `test_unit_journalist` uses a lot of psuedorandomness, we use a deterministic seed. * Add branch coverage measurement to `.coveragerc`.

redshiftzero · 2016-12-20T21:26:11Z

I see you've rebased, the git history cleaned up, and all checks are passing. Thanks @fowlslegs! I'm going to merge this one in. If additional refactoring is needed in this part of the code, let's do it in another PR.

redshiftzero reviewed Nov 29, 2016

View reviewed changes

redshiftzero added PR: Pending additional work PR: Rebase needed labels Dec 1, 2016

psivesely force-pushed the refactor-tests/common.py branch from 0307265 to 1fa849f Compare December 2, 2016 19:10

psivesely removed the PR: Rebase needed label Dec 2, 2016

redshiftzero assigned psivesely Dec 8, 2016

psivesely force-pushed the refactor-tests/common.py branch from 7c02d21 to 6065f05 Compare December 8, 2016 22:29

psivesely force-pushed the refactor-tests/common.py branch 3 times, most recently from 73298a9 to 3656ead Compare December 9, 2016 23:09

psivesely added PR: Needs review and removed PR: Pending additional work labels Dec 9, 2016

redshiftzero assigned redshiftzero and unassigned psivesely Dec 9, 2016

redshiftzero suggested changes Dec 13, 2016

View reviewed changes

redshiftzero added PR: Pending additional work and removed PR: Needs review labels Dec 13, 2016

redshiftzero removed their assignment Dec 13, 2016

psivesely force-pushed the refactor-tests/common.py branch from 3493da9 to decc4bf Compare December 13, 2016 23:50

psivesely added PR: Needs review and removed PR: Pending additional work labels Dec 13, 2016

psivesely assigned psivesely and redshiftzero and unassigned psivesely Dec 13, 2016

redshiftzero reviewed Dec 14, 2016

View reviewed changes

psivesely added the PR: Pending additional work label Dec 14, 2016

psivesely added tests and removed PR: Needs review labels Dec 14, 2016

redshiftzero mentioned this pull request Dec 16, 2016

Improve usable folder structure for downloads #1486

Merged

psivesely added PR: Needs review and removed PR: Pending additional work labels Dec 16, 2016

psivesely force-pushed the refactor-tests/common.py branch from 30e5faa to c1cdae0 Compare December 19, 2016 19:20

psivesely force-pushed the refactor-tests/common.py branch from c1cdae0 to 0925a18 Compare December 20, 2016 21:10

Noah Vesely added 2 commits December 20, 2016 13:11

Minor formatting cleanup

990f717

psivesely force-pushed the refactor-tests/common.py branch from 0925a18 to 45f1b83 Compare December 20, 2016 21:11

redshiftzero approved these changes Dec 20, 2016

View reviewed changes

redshiftzero merged commit 9f7b7de into develop Dec 20, 2016

msheiny deleted the refactor-tests/common.py branch January 31, 2017 21:31

conorsch mentioned this pull request Feb 14, 2017

Convert ServerSpec tests to TestInfra #1580

Closed


		def test_admin_authorization_for_posts(self):
		def test_admin_page_restrction_http_posts(self):


		self.assertIn('Passwords didn', res.data)

		def test_admin_add_user_failure_password_too_long(self):

Refactor tests/common.py #1474

Refactor tests/common.py #1474

Conversation

psivesely commented Nov 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

redshiftzero commented Dec 6, 2016 • edited Loading

psivesely commented Dec 8, 2016

psivesely commented Dec 9, 2016

redshiftzero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psivesely commented Dec 14, 2016

psivesely commented Dec 16, 2016

psivesely commented Dec 16, 2016

psivesely commented Dec 16, 2016

psivesely commented Dec 16, 2016

psivesely commented Dec 19, 2016

conorsch commented Dec 20, 2016

psivesely commented Dec 20, 2016

redshiftzero commented Dec 20, 2016

conorsch commented Dec 20, 2016

redshiftzero commented Dec 20, 2016

redshiftzero commented Dec 20, 2016 • edited Loading

redshiftzero commented Dec 6, 2016 •

edited

Loading

redshiftzero commented Dec 20, 2016 •

edited

Loading