Fix Marionette, low-entropy and mock-induced test flakes. #4451
Conversation
In tests/functional/functional_test.py, FunctionalTest.setup mocked things up, and if an exception were thrown after that in setup, the mocks weren't getting cleaned up. Subsequent two-factor tests were then failing because models.Journalist.verify_token would always return true. This replaces setup/teardown with pytest fixtures and ensures that the mocks are stopped. Also retry web driver creation to work around intermittent Marionette problems. We had redundant session timeout tests, so I removed one. In a couple of places we were changing models.LOGIN_HARDENING and not resetting it to the original value, so those have been fixed. Also, because we just set the servers' names to "localhost" in the tests, Flask was warning that it wasn't a valid cookie domain. So I've appended ".localdomain" to shut it up. We have enough noise in our test output. In test_alembic, the whitespace pattern '\s*' could be empty, potentially causing trouble for split(). Make it r'\s+'. Patch get_entropy_estimate in tests/test_integration.py and tests/test_source.py to prevent failures when CI machines run low on entropy. In test_source.py, the call to logger.assert_called_once_with in test_failed_normalize_timestamps_logs_warning fails if entropy is low in CI -- it's failing not because the message is repeated, but because the mock is called more than once, when the submit route logs that it can't generate the source's key.
rmol
requested review from
heartsucker,
kushaldas and
redshiftzero
as code owners
| self._source_waits_for_session_to_timeout(self.session_length_minutes) | ||
| self._source_enters_text_in_message_field() | ||
| self._source_visits_source_homepage() | ||
| self._screenshot('source-session_timeout.png') |
There was a problem hiding this comment.
So while this is very similar to our functional test, it's worth keeping because the page layout test suite:
- Is ran on translation merges in all supported locales - if we remove test coverage, if there was breakage in the exported strings/HTML placeholders from Weblate (in this case, the string indicating that the session timed out), we may not catch it for that locale.
- Also generates screenshots for updating the SecureDrop user guides and for context for translators in Weblate (pending task to upload those screenshots here: Update screenshots on Weblate #3959)
(I think it is a valid point for us to combine the functional tests and the page layout tests into a single test suite with e.g. a flag for --screenshot instead of --page-layout and pass in the test locales via env var (and default to English for most developer test runs). We can do that but given all the churn in the tests suites I think we should hold off on this battle for now)
There was a problem hiding this comment.
Right. Will save the pruning for a future overhaul. Restored.
|
otherwise this lgtm (exceptions during functional test setup now shouldn't cause 2FA test failures 🌈 ), just needs that one comment addressing and then this is good to merge |
Codecov Report
@@ Coverage Diff @@
## develop #4451 +/- ##
===========================================
+ Coverage 83.72% 83.79% +0.06%
===========================================
Files 44 44
Lines 2956 2956
Branches 321 321
===========================================
+ Hits 2475 2477 +2
+ Misses 404 402 -2
Partials 77 77
Continue to review full report at Codecov.
|
Status
Ready for review
Description of Changes
In tests/functional/functional_test.py, FunctionalTest.setup mocked things up, and if an exception were thrown after that in setup, the mocks weren't getting cleaned up. Subsequent two-factor tests were then failing because models.Journalist.verify_token would always return true. This replaces setup/teardown with pytest fixtures and ensures that the mocks are stopped. (Thanks @redshiftzero for finding this one.)
Also retry web driver creation to work around intermittent Marionette problems.
We had redundant session timeout tests, so I removed one.
In a couple of places we were changing models.LOGIN_HARDENING and not resetting it to the original value, so those have been fixed.
Also, because we just set the servers' names to "localhost" in the tests, Flask was warning that it wasn't a valid cookie domain. So I've appended ".localdomain" to shut it up. We have enough noise in our
test output.
In test_alembic, the whitespace pattern '\s*' could be empty, potentially causing trouble for split(). Make it r'\s+'.
Patch get_entropy_estimate in tests/test_integration.py and tests/test_source.py to prevent failures when CI machines run low on entropy. In test_source.py, the call to logger.assert_called_once_with
in test_failed_normalize_timestamps_logs_warning fails if entropy is low in CI -- it's failing not because the message is repeated, but because the mock is called more than once, when the submit route logs
that it can't generate the source's key.
Fixes #4433.
Testing
Running
make -C securedrop testshould pass, but is not sufficient to verify these changes. Most of the flakes only reliably (heh) happened in CI, due to low entropy or test ordering after exceptions like failures to set up web drivers because of Firefox/geckodriver bugs. (Tests can run in a different order in CI because of how they're divided up and run concurrently.)A truly diligent test would be to set up CI under your account, create a branch from
rmol/fix-4433-tbb-flakes, add a trivial change and push to get CI run.At this point, having been through that process a lot over the last week, I would say it's optional if the CI for this PR passes.
Deployment
The changes are all in tests; this should not affect deployment.
Checklist
If you made changes to the server application code:
make lint) and tests (make -C securedrop test) pass in the development containerIf you made non-trivial code changes: