Assertions are not evaluated in some functional tests

[DrGFreeman]

Description

Functional tests using navigation steps in which the assert statement is behind the if not hasattr(self, "accept_languages"): guard clause always pass regardless of the assertion result.

Steps to Reproduce

Replace the assert statement on line 168 below with assert False.

securedrop/securedrop/tests/functional/journalist_navigation_steps.py

Lines 162 to 170 in 8c86e8e

	def _journalist_clicks_delete_selected_on_modal(self):
	self._journalist_clicks_on_modal("delete-selected")
	def submission_deleted():
	if not hasattr(self, "accept_languages"):
	flash_msg = self.driver.find_element_by_css_selector(".flash")
	assert "Submission deleted." in flash_msg.text
	self.wait_for(submission_deleted)

Run the test in the dev container with make test or securedrop/bin/dev-shell bin/run-test -v -x tests/functional/test_journalist.py::TestJournalist

Expected Behavior

tests/functional/test_journalist.py::TestJournalist::test_journalist_verifies_deletion_of_one_submission_modal is expected to fail.

Actual Behavior

tests/functional/test_journalist.py::TestJournalist::test_journalist_verifies_deletion_of_one_submission_modal passes.

Comments

The FunctionalTest class from which all functional test classes inherit has accept_languages = None as default and this attribute is not modified except for the tests in tests/pageslayout. The if not hasattr(self, "accept_languages"): guard clause therefore always evaluates to False and the assertion is not executed.

I believe the guard clause could be changed to if self.accept_languages is None: so the assertions are evaluated during the functional tests and skipped in the page layout tests.

[DrGFreeman]

A quick search shows 24 occurrences of the if not hasattr(self, "accept_languages"): guard clause in the functional tests navigation steps (current develop branch):

jul@cobweb:~/git/securedrop/securedrop/tests/functional$ grep -ne "if not hasattr(self, \"accept" *.py
journalist_navigation_steps.py:156:            if not hasattr(self, "accept_languages"):
journalist_navigation_steps.py:166:            if not hasattr(self, "accept_languages"):
journalist_navigation_steps.py:278:            if not hasattr(self, "accept_languages"):
journalist_navigation_steps.py:330:        if not hasattr(self, "accept_languages"):
journalist_navigation_steps.py:352:        if not hasattr(self, "accept_languages"):
journalist_navigation_steps.py:367:        if not hasattr(self, "accept_languages"):
journalist_navigation_steps.py:383:            if not hasattr(self, "accept_languages"):
journalist_navigation_steps.py:406:            if not hasattr(self, "accept_languages"):
journalist_navigation_steps.py:417:            if not hasattr(self, "accept_languages"):
journalist_navigation_steps.py:569:            if not hasattr(self, "accept_languages"):
journalist_navigation_steps.py:611:            if not hasattr(self, "accept_languages"):
journalist_navigation_steps.py:679:        if not hasattr(self, "accept_languages"):
journalist_navigation_steps.py:768:            if not hasattr(self, "accept_languages"):
journalist_navigation_steps.py:797:            if not hasattr(self, "accept_languages"):
journalist_navigation_steps.py:887:            if not hasattr(self, "accept_languages"):
journalist_navigation_steps.py:917:            if not hasattr(self, "accept_languages"):
source_navigation_steps.py:112:        if not hasattr(self, "accept_languages"):
source_navigation_steps.py:120:            if not hasattr(self, "accept_languages"):
source_navigation_steps.py:147:                if not hasattr(self, "accept_languages"):
source_navigation_steps.py:163:            if not hasattr(self, "accept_languages"):
source_navigation_steps.py:201:            if not hasattr(self, "accept_languages"):
source_navigation_steps.py:231:        if not hasattr(self, "accept_languages"):
source_navigation_steps.py:245:        if not hasattr(self, "accepted_languages"):
source_navigation_steps.py:252:        if not hasattr(self, "accepted_languages"):

Replacing the guard clause with if True: and running the functional test results in three failed tests (output edited for clarity):

jul@cobweb:~/git/securedrop$ securedrop/bin/dev-shell bin/run-test -v --lf tests/functional
============================================= test session starts ==============================================
platform linux -- Python 3.5.2, pytest-6.1.1, py-1.9.0, pluggy-0.13.1 -- /opt/venvs/securedrop-app-code/bin/python3
cachedir: .pytest_cache
hypothesis profile 'securedrop' -> deadline=None, database=DirectoryBasedExampleDatabase('/home/jul/git/securedrop/securedrop/.hypothesis/examples')
rootdir: /home/jul/git/securedrop/securedrop/tests, configfile: pytest.ini
plugins: forked-1.3.0, flaky-3.6.0, mock-1.7.1, cov-2.5.1, xdist-2.1.0, hypothesis-4.22.2
collected 31 items / 28 deselected / 3 selected                                                                
run-last-failure: rerun previous 3 failures

tests/functional/test_admin_interface.py::TestAdminInterface::test_admin_edits_hotp_secret FAILED        [ 33%]
tests/functional/test_admin_interface.py::TestAdminInterface::test_admin_edits_totp_secret FAILED        [ 66%]
tests/functional/test_admin_interface.py::TestAdminInterface::test_disallow_file_submission FAILED       [100%]

=================================================== FAILURES ===================================================
_______________________________ TestAdminInterface.test_admin_edits_hotp_secret ________________________________

self = <tests.functional.test_admin_interface.TestAdminInterface object at 0x7f2a68e7be48>

    def test_admin_edits_hotp_secret(self):
        # Toggle security slider to force prefs change
        self.set_tbb_securitylevel(ft.TBB_SECURITY_HIGH)
        self.set_tbb_securitylevel(ft.TBB_SECURITY_LOW)
    
        self._admin_logs_in()
        self._admin_visits_admin_interface()
        self._admin_adds_a_user()
        self._admin_visits_edit_user()
>       self._admin_visits_reset_2fa_hotp()

/home/jul/git/securedrop/securedrop/tests/functional/test_admin_interface.py:34: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
/home/jul/git/securedrop/securedrop/tests/functional/journalist_navigation_steps.py:893: in _admin_visits_reset_2fa_hotp
    self.retry_2fa_pop_ups(_admin_visits_reset_2fa_hotp_step, "reset-two-factor-hotp")
/home/jul/git/securedrop/securedrop/tests/functional/journalist_navigation_steps.py:859: in retry_2fa_pop_ups
    navigation_step()
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

    def _admin_visits_reset_2fa_hotp_step():
        # 2FA reset buttons show a tooltip with explanatory text on hover.
        # Also, confirm the text on the tooltip is the correct one.
        hotp_reset_button = self.driver.find_elements_by_id(
            "reset-two-factor-hotp")[0]
        hotp_reset_button.location_once_scrolled_into_view
        ActionChains(self.driver).move_to_element(hotp_reset_button).perform()
    
        time.sleep(1)
    
        tip_opacity = self.driver.find_elements_by_css_selector(
            "#button-reset-two-factor-hotp span")[0].value_of_css_property('opacity')
        tip_text = self.driver.find_elements_by_css_selector(
            "#button-reset-two-factor-hotp span")[0].text
    
        assert tip_opacity == "1"
    
        if True:
>           assert tip_text == "Reset two-factor authentication for security keys like Yubikey"
E           AssertionError

/home/jul/git/securedrop/securedrop/tests/functional/journalist_navigation_steps.py:888: AssertionError

_______________________________ TestAdminInterface.test_admin_edits_totp_secret ________________________________

self = <tests.functional.test_admin_interface.TestAdminInterface object at 0x7f2a68789be0>

    def test_admin_edits_totp_secret(self):
        # Toggle security slider to force prefs change
        self.set_tbb_securitylevel(ft.TBB_SECURITY_HIGH)
        self.set_tbb_securitylevel(ft.TBB_SECURITY_LOW)
    
        self._admin_logs_in()
        self._admin_visits_admin_interface()
        self._admin_adds_a_user()
        self._admin_visits_edit_user()
>       self._admin_visits_reset_2fa_totp()

/home/jul/git/securedrop/securedrop/tests/functional/test_admin_interface.py:46: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
/home/jul/git/securedrop/securedrop/tests/functional/journalist_navigation_steps.py:927: in _admin_visits_reset_2fa_totp
    self.retry_2fa_pop_ups(_admin_visits_reset_2fa_totp_step, "reset-two-factor-totp")
/home/jul/git/securedrop/securedrop/tests/functional/journalist_navigation_steps.py:859: in retry_2fa_pop_ups
    navigation_step()
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

    def _admin_visits_reset_2fa_totp_step():
        totp_reset_button = self.driver.find_elements_by_id("reset-two-factor-totp")[0]
        assert "/admin/reset-2fa-totp" in totp_reset_button.get_attribute("action")
        # 2FA reset buttons show a tooltip with explanatory text on hover.
        # Also, confirm the text on the tooltip is the correct one.
        totp_reset_button = self.driver.find_elements_by_css_selector(
            "#button-reset-two-factor-totp")[0]
        totp_reset_button.location_once_scrolled_into_view
        ActionChains(self.driver).move_to_element(totp_reset_button).perform()
    
        time.sleep(1)
    
        tip_opacity = self.driver.find_elements_by_css_selector(
            "#button-reset-two-factor-totp span")[0].value_of_css_property('opacity')
        tip_text = self.driver.find_elements_by_css_selector(
            "#button-reset-two-factor-totp span")[0].text
    
        assert tip_opacity == "1"
        if True:
            expected_text = (
                "Reset two-factor authentication for mobile apps such as FreeOTP "
                "or Google Authenticator"
            )
>           assert tip_text == expected_text
E           AssertionError

/home/jul/git/securedrop/securedrop/tests/functional/journalist_navigation_steps.py:922: AssertionError

_______________________________ TestAdminInterface.test_disallow_file_submission _______________________________

self = <tests.functional.test_admin_interface.TestAdminInterface object at 0x7f2a6af25f60>

    def test_disallow_file_submission(self):
        self._admin_logs_in()
        self._admin_visits_admin_interface()
        self._admin_visits_system_config_page()
        self._admin_disallows_document_uploads()
    
        self._source_visits_source_homepage()
        self._source_chooses_to_submit_documents()
>       self._source_continues_to_submit_page()

/home/jul/git/securedrop/securedrop/tests/functional/test_admin_interface.py:88: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
/home/jul/git/securedrop/securedrop/tests/functional/source_navigation_steps.py:124: in _source_continues_to_submit_page
    self.wait_for(submit_page_loaded)
/home/jul/git/securedrop/securedrop/tests/functional/functional_test.py:339: in wait_for
    return function_with_assertion()
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

    def submit_page_loaded():
        if True:
            headline = self.driver.find_element_by_class_name("headline")
>           assert "Submit Files or Messages" == headline.text
E           AssertionError

/home/jul/git/securedrop/securedrop/tests/functional/source_navigation_steps.py:122: AssertionError

=========================================== short test summary info ============================================
FAILED tests/functional/test_admin_interface.py::TestAdminInterface::test_admin_edits_hotp_secret - Assertion...
FAILED tests/functional/test_admin_interface.py::TestAdminInterface::test_admin_edits_totp_secret - Assertion...
FAILED tests/functional/test_admin_interface.py::TestAdminInterface::test_disallow_file_submission - Assertio...
========================== 3 failed, 28 deselected, 12 warnings in 132.14s (0:02:12) ===========================

[kushaldas]

I think @DrGFreeman you also found another functional test error, the message Reset two-factor authentication for mobile apps such as FreeOTP should be changed to Reset two-factor authentication for security keys, like a YubiKey.

[kushaldas]

I believe the guard clause could be changed to if self.accept_languages is None:so the assertions are evaluated during the functional tests and skipped in the page layout tests.

if not self.accept_languages: is better than is None check in my mind :)

[DrGFreeman]

@eloquence, I can take care of this one if it's OK with the team.

[eloquence]

@DrGFreeman That would be awesome, thank you :)