Rewrite tests with pytest fixtures #2948

heartsucker · 2018-01-28T17:42:48Z

Status

Ready for review

Description of Changes

Work towards #2877

Added pytest fixtures to centralize app creation logic for the source app

Testing

make test

Checklist

If you made changes to the app code:

Unit and functional tests pass on the development VM

kushaldas · 2018-02-19T08:41:43Z

I think somehow the cache is causing trouble in CircleCI. make test passes properly in local system.

heartsucker · 2018-02-19T09:00:14Z

@kushaldas That's what I thought too. I guess I'll open a ticket for that.

heartsucker · 2018-02-19T16:36:32Z

For now, it seems like this PR is blocked by #3033. Once that is merged, the strange test failure logic related to this line:

c = imp.load_module('c', *imp.find_module('config'))

should go away. But to be fair, I'm not positive that's the line that's causing problems, but it's definitely the most likely one.

codecov-io · 2018-02-28T10:30:31Z

Codecov Report

Merging #2948 into develop will increase coverage by 4.33%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #2948      +/-   ##
===========================================
+ Coverage    84.31%   88.64%   +4.33%     
===========================================
  Files           34       32       -2     
  Lines         2027     1849     -178     
  Branches       221      212       -9     
===========================================
- Hits          1709     1639      -70     
+ Misses         262      162     -100     
+ Partials        56       48       -8

Impacted Files	Coverage Δ
journalist_app/account.py	`92.85% <0%> (-0.33%)`	⬇️
journalist_app/utils.py	`86.23% <0%> (-0.22%)`	⬇️
source_app/main.py	`97.77% <0%> (-0.04%)`	⬇️
create-dev-data.py
i18n_tool.py
sdconfig.py
create-demo-user.py	`0% <0%> (ø)`
journalist_app/admin.py	`89.03% <0%> (+0.59%)`	⬆️
source_app/utils.py	`87.27% <0%> (+1.55%)`	⬆️
crypto_util.py	`97.91% <0%> (+1.99%)`	⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 094f4fa...0b25f12. Read the comment docs.

ghost

Excellent 💯, really happy this is done. I won't lie: it is not the most entertaining PR to review. But you made it very easy for the reviewer by clearly separating each commit, thanks a lot for this.

I have just one quesiton and two nits, other than this I think it is good to merge.

ghost · 2018-03-01T12:48:32Z

securedrop/tests/test_source.py

+            assert "BEGIN PGP PUBLIC KEY BLOCK" in text
+
+    def test_login_and_logout(self, source_app):
+        with source_app.test_client() as app:


Nit: Would it be a problem to keep multiple with source_app.test_client() as app: to separate the blocks?

Why is the grounds for that nit? I think it might do some silly things to sessions and whatnot (but could be wrong).

Since the code in each block is a separate test case, having them use a different test client makes sense (really they should be in different tests entirely - but in the spirit of not expanding the scope of this PR beyond strictly rewriting the tests to use pytest fixtures, @dachary's suggestion seems reasonable)

ghost · 2018-03-01T13:01:31Z

securedrop/tests/test_source.py

+                login_test(app, codename_)
+
+    @staticmethod
+    def _dummy_submission(app):


Nit: for clarity removal of the old _dummy_submission is in commit pytest fixtures: test_csrf_error_page but should be in commit pytest fixtures: test_initial_submission_notification where the static version is introduced.

I don't move all the tests at once, so I leave it in the old class because there are tests that required it,

ghost · 2018-03-01T13:17:01Z

securedrop/tests/test_source.py

+            session.pop('csrf_token', None)
+            assert not session, session
+            text = resp.data.decode('utf-8')
+            assert 'Your session timed out due to inactivity' in text


Why is it different from You have been logged out due to inactivity ? It indicates the test does not behave in the same way and this is either cause for concern or a detailed explanation in the commit message.

That string never appears in the source app, and also that test was prefixed with _test so it was never actually run before. We have higher coverage now that it is actually run.

redshiftzero

looks great, thanks for the organized git history here :) one nit inline and one optional suggestion to look at - otherwise I'm happy with this being merged

redshiftzero · 2018-03-01T22:58:29Z

securedrop/tests/test_source.py

+                            follow_redirects=True)
+            assert resp.status_code == 200
+            text = resp.data.decode('utf-8')
+            assert "Submit Materials" in text, text


Nit: in text, text -> in text

If I'm not mistaken the idea here is to display text in case the assert fails but ... pytest instruments the assert and displays the actual values being compared when it fails. So +1 on the Nit ;-)

Honestly, we should probably do that with all cases since seeing ... in the error output is not very helpful, but that one probably got left in by mistake.

redshiftzero · 2018-03-01T23:29:09Z

securedrop/tests/test_source.py


 overly_long_codename = 'a' * (Source.MAX_CODENAME_LEN + 1)


-class TestSourceApp(TestCase):
+class TestSourceApp:


We can get rid of the TestSourceApp class now, which I think would be nice to flatten this file a bit (if you make this change you could do it in an additional commit if it's easier)

I think we need this class for setup and teardown?

We don't use those anymore. Lifecycles are managed by pytest alone.

redshiftzero

Diff looks good, thanks for making those changes! 👍 to merge from me

ghost

Relying on @redshiftzero review. I would dismiss my review if only it was posssible.

We have not used the flask_testing library since <#2948> (2018), a small part of it was directly copied into SecureDrop. Plus that version of the library was incompatible with Flask 2.0+ and by getting rid of it we don't explicitly have to pin Jinja nor markupsafe.

We have not used the flask_testing library since <#2948> (2018), a small part of it was directly copied into SecureDrop. Plus that version of the library was incompatible with Flask 2.0+ and by getting rid of it we don't explicitly have to pin Jinja nor markupsafe. (cherry picked from commit 65f092c)

heartsucker added tests goals: app code cleanup labels Jan 28, 2018

heartsucker requested a review from a user January 28, 2018 17:42

heartsucker requested a review from redshiftzero as a code owner January 28, 2018 17:42

heartsucker mentioned this pull request Jan 28, 2018

Meta issue: No new tests until we have cleaned up the existing test infrastructure #2877

Closed

heartsucker force-pushed the pytest-fixtures branch from e114fd3 to 3cfd549 Compare February 4, 2018 08:54

redshiftzero added the PR: Pending additional work label Feb 15, 2018

heartsucker force-pushed the pytest-fixtures branch 6 times, most recently from b0acd4e to b38cdbf Compare February 16, 2018 17:14

heartsucker requested review from conorsch and msheiny as code owners February 16, 2018 17:14

heartsucker force-pushed the pytest-fixtures branch 3 times, most recently from ba9e6ca to 40fc5f5 Compare February 16, 2018 22:07

heartsucker mentioned this pull request Feb 19, 2018

Caching / tagging of docker images in CI causes errant build failures #3030

Closed

heartsucker added the blocked label Feb 22, 2018

heartsucker mentioned this pull request Feb 27, 2018

Use passlib for password hashing #2918

Closed

heartsucker force-pushed the pytest-fixtures branch from 90de09a to e118c6a Compare February 28, 2018 09:47

heartsucker removed the blocked label Feb 28, 2018

heartsucker removed the PR: Pending additional work label Feb 28, 2018

ghost suggested changes Mar 1, 2018

View reviewed changes

redshiftzero suggested changes Mar 1, 2018

View reviewed changes

heartsucker added 20 commits March 10, 2018 15:44

pytest fixtures: test_submit_big_message

5eb88d9

pytest fixtures: test_submit_file

d4868ad

pytest fixtures: test_submit_both

8b0842d

pytest fixtures: test_submit_message_with_low_entropy

7b706ef

pytest fixtures: test_submit_message_with_enough_entropy

31aaddc

pytest fixtures: test_delete_all_successfully_deletes_replies

ac98d0e

pytest fixtures: test_delete_all_replies_already_deleted

229fd45

pytest fixtures: test_submit_sanitizes_filename

41bdd26

pytest fixtures: test_tor2web_warning_headers

0434729

pytest fixtures: test_tor2web_warning

ad0c908

pytest fixtures: test_why_use_tor_browser

91a2655

pytest fixtures: test_why_journalist_key

e059a22

pytest fixtures: test_metadata_route

5995b06

pytest fixtures: test_login_with_overly_long_codename

ba0dc76

pytest fixtures: test_failed_normalize_timestamps_logs_warning

36c7fe7

pytest fixtures: test_source_is_deleted_while_logged_in

2ef9cc0

pytest fixtures: test_login_with_invalid_codename

f7ea332

pytest fixtures: test_source_session_expiration

9906680

pytest fixtures: test_csrf_error_page

372c565

drop class from test cases

0b25f12

heartsucker force-pushed the pytest-fixtures branch from 7f58b73 to 0b25f12 Compare March 10, 2018 17:16

heartsucker mentioned this pull request Mar 11, 2018

Journalist pytest fixtures #3140

Merged

1 task

redshiftzero approved these changes Mar 11, 2018

View reviewed changes

ghost approved these changes Mar 12, 2018

View reviewed changes

ghost merged commit 7e87330 into develop Mar 12, 2018

heartsucker deleted the pytest-fixtures branch March 12, 2018 16:45

This was referenced Sep 29, 2022

Remove usage of FlaskTesting #3607

Closed

Flask Testing uses one request context per TestCase method #1444

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite tests with pytest fixtures #2948

Rewrite tests with pytest fixtures #2948

heartsucker commented Jan 28, 2018 •

edited

Loading

kushaldas commented Feb 19, 2018

heartsucker commented Feb 19, 2018

heartsucker commented Feb 19, 2018

codecov-io commented Feb 28, 2018 •

edited

Loading

ghost left a comment

ghost Mar 1, 2018

heartsucker Mar 6, 2018

redshiftzero Mar 7, 2018

ghost Mar 1, 2018

heartsucker Mar 6, 2018

ghost Mar 1, 2018

heartsucker Mar 6, 2018

redshiftzero left a comment

redshiftzero Mar 1, 2018

ghost Mar 2, 2018

heartsucker Mar 6, 2018

heartsucker Mar 9, 2018

redshiftzero Mar 1, 2018

ghost Mar 2, 2018

heartsucker Mar 6, 2018

redshiftzero left a comment

ghost left a comment

Rewrite tests with pytest fixtures #2948

Rewrite tests with pytest fixtures #2948

Conversation

heartsucker commented Jan 28, 2018 • edited Loading

Status

Description of Changes

Testing

Checklist

If you made changes to the app code:

kushaldas commented Feb 19, 2018

heartsucker commented Feb 19, 2018

heartsucker commented Feb 19, 2018

codecov-io commented Feb 28, 2018 • edited Loading

Codecov Report

ghost 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

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

redshiftzero left a comment

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

heartsucker commented Jan 28, 2018 •

edited

Loading

codecov-io commented Feb 28, 2018 •

edited

Loading