Added base OS info to metadata endpoint

[nightwarriorftw]

Status

Ready for review

Description of Changes

Fixes #4059

[nightwarriorftw]

@redshiftzero Please review this and suggest changes if any

[redshiftzero]

since we want these tests to pass on both trusty/14.04 and xenial/16.04 (we run tests in both environments), can you mock out platform.linux_distribution? Check out this commit for a example of how to do that

[nightwarriorftw]

Yes, will try my best

[nightwarriorftw]

@redshiftzero Why is it failing ? Is it the because of mock .

[kushaldas]

@redshiftzero Why is it failing ? Is it the because of mock .

Did you test the test case locally by running all of the tests?

Check https://circleci.com/gh/freedomofpress/securedrop/21953 and see the error message in red. If you still have questions, ask in gitter. Being able to read error messages will help in long run. Feel free to ping me on IRC if you want.

[nightwarriorftw]

@kushaldas Please have a look at this. I did some changes and then tested locally again on my system.

[kushaldas]

@kushaldas Please have a look at this. I did some changes and then tested locally again on my system.

Yes, you are running in a xenial system and checking the value of trusty. This is why @redshiftzero suggested to mock that call.

[redshiftzero]

hey @nightwarrior-xxx, just to elaborate a bit more on what @kushaldas and I were saying above: we don't want to actually call platform.linux_distribution() in the test for the metadata endpoint since we run the application tests on both Ubuntu Trusty and Ubuntu Xenial, so we'll have different expected responses based on the base OS we're currently testing on.

This is a good place for a mock object (well, it's a stub): which is just a dummy object for test purposes. For example in our case, we want to replace platform.linux_distribution() with one of these dummy objects. We can then have our dummy object always produce a return value of the what we expect from Ubuntu Xenial. That way, the behavior will be the same on whatever base OS we might test on.

Check out the following diff:

diff --git a/securedrop/tests/test_source.py b/securedrop/tests/test_source.py
index 5ce6a661..c4d53989 100644
--- a/securedrop/tests/test_source.py
+++ b/securedrop/tests/test_source.py
@@ -515,14 +515,17 @@ def test_why_journalist_key(source_app):
         assert "Why download the journalist's public key?" in text


-def test_metadata_route(source_app):
+def test_metadata_route(source_app, mocker):
     with source_app.test_client() as app:
+        mocked_platform = mocker.patch('platform.linux_distribution')
+        mocked_platform.return_value = ('Ubuntu', '16.04', 'xenial')
+
         resp = app.get(url_for('api.metadata'))
         assert resp.status_code == 200
         assert resp.headers.get('Content-Type') == 'application/json'
-        assert json.loads(resp.data.decode('utf-8')).get('sd_version') \
-            == version.__version__
-        assert resp.get('server_os') == 16.04
+        data = json.loads(resp.data.decode('utf-8'))
+        assert data.get('sd_version') == version.__version__
+        assert data.get('server_os') == '16.04'

Here we are patching platform.linux_distribution using the pytest-mock plugin. We're then setting the return value to be what we would expect from platform.linux_distribution() when we're on Ubuntu xenial. Try this diff out, and if all goes well, test_metadata_route should now pass! Let us know how it goes.

PS: btw this is one of my favorite talks on testing in Python, there are some really clear examples of various situations where one would want to use mocks towards the middle/end of the talk.

[nightwarriorftw]

@redshiftzero Still test failed . Here are the logs.

[heartsucker]

This line

mocked_platform = mocker.patch('platform.linux_distribution')

Needs to be this

mocked_platform = mocker.patch('source_app.api.platform.linux_distribution')

(I think)

[heartsucker]

But regardless, what's happening is the call to platoform.linux_distribution is returning a MagicMock which can't be JSON serialized (only bool, int/float, dict, list, str, and None).

[kushaldas]

@redshiftzero Still test failed . Here are the logs.

That is because you took half of @redshiftzero's original patch.

Here is one more example following the other examples in the same file:

iff --git a/securedrop/tests/test_source.py b/securedrop/tests/test_source.py
index 5ce6a661..d92231a3 100644
--- a/securedrop/tests/test_source.py
+++ b/securedrop/tests/test_source.py
@@ -16,6 +16,7 @@ import version
 from db import db
 from models import Source, Reply
 from source_app import main as source_app_main
+from source_app import api as source_app_api
 from utils.db_helper import new_codename
 from utils.instrument import InstrumentedApp
 
@@ -516,14 +517,15 @@ def test_why_journalist_key(source_app):
 
 
 def test_metadata_route(source_app):
-    with source_app.test_client() as app:
-        resp = app.get(url_for('api.metadata'))
-        assert resp.status_code == 200
-        assert resp.headers.get('Content-Type') == 'application/json'
-        assert json.loads(resp.data.decode('utf-8')).get('sd_version') \
-            == version.__version__
-        assert resp.get('server_os') == 16.04
-
+    with patch.object(source_app_api.platform, "linux_distribution") as mock_platform:
+        mock_platform.return_value = ("Ubuntu", "16.04", "xenial")
+        with source_app.test_client() as app:
+            resp = app.get(url_for('api.metadata'))
+            assert resp.status_code == 200
+            assert resp.headers.get('Content-Type') == 'application/json'
+            data = json.loads(resp.data.decode('utf-8'))
+            assert data.get('sd_version') == version.__version__
+            assert data.get('server_os') == '16.04'
 
 def test_login_with_overly_long_codename(source_app):
     """Attempting to login with an overly long codename should result in

[redshiftzero]

(the patch I posted above I did test and does work if applied in total, one can directly patch platform.linux_distribution)

[nightwarriorftw]

@redshiftzero Please review !!

[redshiftzero]

hi @nightwarrior-xxx, excellent - tests are passing now. The final thing is to resolve the linting errors:

./securedrop/tests/test_source.py:519:35: E231 missing whitespace after ','
./securedrop/tests/test_source.py:520:46: E231 missing whitespace after ','
./securedrop/tests/test_source.py:521:49: E231 missing whitespace after ','
./securedrop/tests/test_source.py:521:57: E231 missing whitespace after ','
./securedrop/tests/test_source.py:530:1: W293 blank line contains whitespace

You can run make flake8 locally to verify the above issues are resolved prior to pushing up. Let us know how it goes!

[nightwarriorftw]

@redshiftzero Done but still lint fails

securedrop/source_app/__init__.py:40: error: "Dict[str, Any]" has no attribute "from_object
securedrop/journalist_app/__init__.py:43: error: "Dict[str, Any]" has no attribute "from_object"

Are these causing problem ?

[redshiftzero]

thanks @nightwarrior-xxx!

[redshiftzero]

(for posterity: the lint failures are fixed on develop in 9d06704 and were caused due to an upstream mypy change, rebase on latest resolves)

[kushaldas]

@nightwarrior-xxx Why do we have this mocker here?

[redshiftzero]

that's from pytest-mock

[redshiftzero]

oh yeah this is doubly mocked...

dismissing because the mocking needs fixing

[nightwarriorftw]

@redshiftzero @heartsucker Please review

[kushaldas]

This is finally good.