Fix profile.d tests for sd-app #623
Conversation
emkll
force-pushed
the
fix-test-sd-app-profile
branch
from
598cc16 to
599f846
Compare
tests/test_app.py
Outdated
| @@ -11,7 +11,7 @@ def setUp(self): | |||
|
|
|||
| def test_decrypt_sd_user_profile(self): | |||
| contents = self._get_file_contents("/etc/profile.d/sd-app-qubes-gpg-domain.sh") | |||
| expected_content = 'export QUBES_GPG_DOMAIN="sd-gpg"\n' | |||
| expected_content = 'if [ "$(qubesdb-read /name)" = "sd-app" ]; then export QUBES_GPG_DOMAIN="sd-gpg"; fi\n' # noqa: E501 | |||
There was a problem hiding this comment.
This'll work, but to make this more of a test of behavior rather than implementation I wonder if it might be better to check if the output of qvm-run -p sd-app "echo \$QUBES_GPG_DOMAIN" is the expected sd-gpg -- that way we could also run the inverse test in the other AppVMs, and would be more independent of the implementation.
There was a problem hiding this comment.
good call, updated the test in f65b62c
There was a problem hiding this comment.
Nice! In case you didn't already consider and reject that in this case, note that have a base.py we can use to avoid duplication of test code -- see logging_configured as an example. Because these helper methods don't start with test, they won't be run as part of the test suite unless explicitly invoked with test_* methods in child classes, as we do for the logging test.
(Personally, I would vote to remove the content test now because I don't find these types of tests that directly duplicate implementation super-helpful, especially with the behavior test in place, but defer to you and reviewer of course.)
emkll
force-pushed
the
fix-test-sd-app-profile
branch
2 times, most recently
from
a7cdf03 to
f65b62c
Compare
There was a problem hiding this comment.
Built client and patched as per test plan.
-
make devcompleted happily -
make testcompleted successfully with the exception oftest_all_sd_vms_uptodate, which only failed because the securedrop-client nightly is available as an upgradable option
LGTM once it's flipped out of "in dev" status - suggested test refactoring would be nice to have but I don't think it should block merge.
QUBES_GPG_DOMAIN will be conditionally set based on the running VM in order to support template consolidation. See freedomofpress/securedrop-client#1141
emkll
force-pushed
the
fix-test-sd-app-profile
branch
from
f65b62c to
64a24e5
Compare
|
Updated tests based on feedback and rebased on latest |
Follow-up to [0]. Post-consolidation, we can expect the /etc/profile.d/ path to be present on all systems, but only on sd-app should it return "sd-gpg" rather than an empty string. [0] #623
Follow-up to [0]. Post-consolidation, we can expect the /etc/profile.d/ path to be present on all systems, but only on sd-app should it return "sd-gpg" rather than an empty string. [0] #623
Follow-up to [0]. Post-consolidation, we can expect the /etc/profile.d/ path to be present on all systems, but only on sd-app should it return "sd-gpg" rather than an empty string. [0] #623
Status
Ready for review
This PR should be reviewed at the same time as freedomofpress/securedrop-client#1159 and freedomofpress/securedrop-builder#202.
Description of Changes
Fixes #621
QUBES_GPG_DOMAINis conditionally set based on the running VM in order to support template consolidation. See freedomofpress/securedrop-client#1141This will also help testing both client and packaging PRs that resolve these test failures.
Testing
Build a client package based on the following branches/PRs:
Apply the diff below to install the sd-app package locally instead of through apt server
make devshould complete successfullymake testshould complete successfully (except sd-app being up-to-date, this is because the client version needs to be bumped)Checklist
If you have made changes to the provisioning logic
make test) pass indom0of a Qubes install (except the upgrades for sd-app (the client version needs to be bumped)