Use the existing GPG fixture in the functional tests (CryptoUtil 2/3) #6184

nabla-c0d3 · 2021-11-22T00:50:56Z

Status

Ready.

Description of Changes

This PR updates the functional tests to use the same fixture for GPG as the other tests. It also updates the config fixture.

This is a prerequisite for the refactoring of the GPG code in Refactor encryption logic to fix type-checking and improve tests (CryptoUtil 3/3) #6160 which uses the updated config fixture.
This is a small step forward for Convert functional tests to use pytest/fixtures #3836. More work is needed to refactor the functional tests and their fixtures.
Because the GPG fixture that is now being used is session-scoped, it is only called once during the whole test suite, and only one GPG home directory is used for all the tests. This helps a little bit with GPG agent processes are not cleaned up during testing #4332 (only 1 GPG process should get created instead of ~80).
There is more work to be done with the config fixture itself, and I might take a look at it eventually. The biggest problem is that tests might either use the config from the config() fixture, or the config from the config.py file generated by make test right before the tests run.
- This creates really tricky interactions and bugs when working on the test suite, because it's unclear which config is being used. It's also sometime unclear what config is needed for a specific test to succeed, and some tests also tend to mutate the config object (for example changing the time it takes for a session to expire).
- This PR makes small changes to slightly improve this situation.

nabla-c0d3 · 2021-11-23T04:57:51Z

securedrop/tests/__init__.py

+
+# WARNING: This variable must be set before any import of the securedrop code/app
+# or most tests will fail
+os.environ["SECUREDROP_ENV"] = "test"


Putting this here ensures that the SECUREDROP_ENV will always be set before any application code is imported.

nabla-c0d3 · 2021-11-23T04:58:46Z

securedrop/tests/conftest.py

@@ -110,37 +107,39 @@ def insecure_scrypt() -> Generator[None, None, None]:
        yield


-def setup_gpg(gpg_dir: Path) -> str:


All I did here was move the logic in setup_gpg() to setup_journalist_key_and_gpg_folder().

nabla-c0d3 · 2021-11-23T05:02:14Z

securedrop/tests/conftest.py

@@ -172,7 +171,10 @@ def config(

        config.SUPPORTED_LOCALES = i18n.get_test_locales()

-        yield config
+        # Set this newly-created config as the "global" config
+        with mock.patch.object(sdconfig, "config", config):


Important change here: this ensures that the application code "sees" the same config as the one created in the fixture (when the code imports the config).

It's not perfect and more work is needed with the config fixture, but it's a step forward.

SECUREDROP_ENV does not seem to be set here when make test is run locally . It's used to set the database URI to the appropriate value for the environment, so without it the alembic tests in particular will fail. (Said failure isn't happening in CI for some reason).

One solution is to add os.environ["SECUREDROP_ENV"] = "test" before filesystem setup, but I'm sure there are more elegant ones :)

nabla-c0d3 · 2021-11-23T05:03:00Z

securedrop/tests/conftest.py

+    config.SOURCE_APP_FLASK_CONFIG_CLS.USE_X_SENDFILE = False
+
+    # Disable CSRF checks to make writing tests easier
+    config.SOURCE_APP_FLASK_CONFIG_CLS.WTF_CSRF_ENABLED = False


This was copied from the config.py created by make test. These settings are needed for the tests to pass.

nabla-c0d3 · 2021-11-23T05:03:22Z

securedrop/tests/functional/functional_test.py

@@ -202,7 +199,7 @@ def set_default_driver(self):
            logging.error("Error stopping Firefox driver: %s", e)

    @pytest.fixture(autouse=True)
-    def sd_servers(self):
+    def sd_servers(self, setup_journalist_key_and_gpg_folder):


The "main" change in this PR.

nabla-c0d3 · 2021-11-23T05:04:30Z

securedrop/tests/utils/env.py

-        # Then check the directory was already deleted
-        assert not os.path.exists(config.TEMP_DIR)
-    try:
-        shutil.rmtree(config.SECUREDROP_DATA_ROOT)


I am no longer deleting the SECUREDROP_DATA_ROOT between each test because it will delete the GPG_KEY_DIR too (as it's a subfolder of SECUREDROP_DATA_ROOT).

zenmonkeykstop

CI looks good, but the test_alembic.py tests fail when make test is run locally as the db URI is being set to the prod value instead of test's.

zenmonkeykstop · 2021-11-30T22:16:29Z

securedrop/tests/conftest.py

@@ -172,7 +171,10 @@ def config(

        config.SUPPORTED_LOCALES = i18n.get_test_locales()

-        yield config
+        # Set this newly-created config as the "global" config
+        with mock.patch.object(sdconfig, "config", config):


SECUREDROP_ENV does not seem to be set here when make test is run locally . It's used to set the database URI to the appropriate value for the environment, so without it the alembic tests in particular will fail. (Said failure isn't happening in CI for some reason).

One solution is to add os.environ["SECUREDROP_ENV"] = "test" before filesystem setup, but I'm sure there are more elegant ones :)

nabla-c0d3 · 2021-12-03T21:12:21Z

securedrop/manage.py

-from db import db
-from models import (
+if not os.environ.get("SECUREDROP_ENV"):
+    os.environ['SECUREDROP_ENV'] = 'dev'  # noqa


@zenmonkeykstop This fixes the alembic tests. Thanks for explaining what the problem was.

Fix alembic tests failing

zenmonkeykstop · 2021-12-09T18:07:27Z

(rebased to pull in a dependency update and get tests passing)

zenmonkeykstop

Changes look good, tests passing locally as well now!

nabla-c0d3 force-pushed the consolidate-gpg-test-fixtures branch 2 times, most recently from a3d28e0 to d7be83c Compare November 22, 2021 01:26

nabla-c0d3 mentioned this pull request Nov 23, 2021

Disable caching of GPG passphrases in the GPG agent (CryptoUtil 1/3) #6174

Merged

nabla-c0d3 commented Nov 23, 2021

View reviewed changes

nabla-c0d3 force-pushed the consolidate-gpg-test-fixtures branch from d7be83c to fac74a5 Compare November 25, 2021 02:47

nabla-c0d3 marked this pull request as ready for review November 25, 2021 02:55

nabla-c0d3 requested a review from a team as a code owner November 25, 2021 02:55

zenmonkeykstop self-assigned this Nov 29, 2021

zenmonkeykstop requested changes Nov 30, 2021

View reviewed changes

eloquence added this to In Development in SecureDrop Team Board Dec 1, 2021

nabla-c0d3 force-pushed the consolidate-gpg-test-fixtures branch from fd5c35b to 11d7490 Compare December 3, 2021 21:08

nabla-c0d3 commented Dec 3, 2021

View reviewed changes

nabla-c0d3 requested a review from zenmonkeykstop December 3, 2021 21:12

Use the existing GPG fixture in the functional tests

403629d

Fix alembic tests failing

zenmonkeykstop previously approved these changes Dec 9, 2021

View reviewed changes

zenmonkeykstop dismissed their stale review via 403629d December 9, 2021 18:06

zenmonkeykstop force-pushed the consolidate-gpg-test-fixtures branch from 11d7490 to 403629d Compare December 9, 2021 18:06

zenmonkeykstop approved these changes Dec 10, 2021

View reviewed changes

zenmonkeykstop merged commit 90b3a08 into freedomofpress:develop Dec 10, 2021

SecureDrop Team Board automation moved this from In Development to Done Dec 10, 2021

nabla-c0d3 mentioned this pull request Dec 18, 2021

GPG agent processes are not cleaned up during testing #4332

Open

nabla-c0d3 deleted the consolidate-gpg-test-fixtures branch June 11, 2022 17:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the existing GPG fixture in the functional tests (CryptoUtil 2/3) #6184

Use the existing GPG fixture in the functional tests (CryptoUtil 2/3) #6184

nabla-c0d3 commented Nov 22, 2021 •

edited

nabla-c0d3 Nov 23, 2021

nabla-c0d3 Nov 23, 2021

nabla-c0d3 Nov 23, 2021

zenmonkeykstop Nov 30, 2021

nabla-c0d3 Nov 23, 2021

nabla-c0d3 Nov 23, 2021

nabla-c0d3 Nov 23, 2021

zenmonkeykstop left a comment

zenmonkeykstop Nov 30, 2021

nabla-c0d3 Dec 3, 2021

zenmonkeykstop commented Dec 9, 2021

zenmonkeykstop left a comment

		@@ -110,37 +107,39 @@ def insecure_scrypt() -> Generator[None, None, None]:
		yield


		def setup_gpg(gpg_dir: Path) -> str:

Use the existing GPG fixture in the functional tests (CryptoUtil 2/3) #6184

Use the existing GPG fixture in the functional tests (CryptoUtil 2/3) #6184

Conversation

nabla-c0d3 commented Nov 22, 2021 • edited

Status

Description of Changes

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

zenmonkeykstop left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zenmonkeykstop commented Dec 9, 2021

zenmonkeykstop left a comment

Choose a reason for hiding this comment

nabla-c0d3 commented Nov 22, 2021 •

edited