Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

reduce failure rate of 2FA admin functional tests #4591

Merged
merged 1 commit into from
Jul 4, 2019
Merged

Conversation

redshiftzero
Copy link
Contributor

Status

Ready for review

Description of Changes

Fixes #4580
Fixes #4541

This PR adds a retry loop function for clicking Selenium alerts for the 2FA tests. My procedure for debugging these test issues is to:

  1. Hop into the dev container and pip3 install pytest-repeat

  2. Add the --count arg that pytest-repeat provides so I can rerun the offending test, e.g. here 100 times:

diff --git a/securedrop/bin/run-test b/securedrop/bin/run-test
index 4571276ff..bcb548639 100755
--- a/securedrop/bin/run-test
+++ b/securedrop/bin/run-test
@@ -33,6 +33,7 @@ export PAGE_LAYOUT_LOCALES
 export TOR_FORCE_NET_CONFIG=0


 pytest \
+    --count 100 \
     --page-layout \
     --durations 10 \
     --junitxml=/tmp/test-results/junit.xml \
  1. Use the above to run single tests 100 times on this diff versus develop to confirm I'm reducing the failure rate, my observations from local testing:
bin/run-test tests/functional/test_admin_interface.py::TestAdminInterface::test_admin_edits_hotp_secret

With this PR: 100/100 success
On develop: 99/100 success

bin/run-test tests/functional/test_admin_interface.py::TestAdminInterface::test_admin_edits_totp_secret

With this PR: 100/100 success
On develop: 97/100 success

Note that there's obviously going to be some variability and these are low number statistics so your results may be slightly different.

Testing

While one could redo the above, since these are test only changes I don't think it's super valuable redoing the above. Instead, I would just scan the diff and see if it looks reasonable (e.g. _admin_visits_reset_2fa_totp didn't have any retry logic previously so not surprising it had a higher failure rate). If we see these same issues pop up again, we can reopen the tickets.

Deployment

Test only

Checklist

If you made changes to the server application code:

  • Linting (make lint) and tests (make -C securedrop test) pass in the development container

Copy link
Contributor

@kushaldas kushaldas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good, thank you 🌈

@kushaldas kushaldas merged commit 41026b8 into develop Jul 4, 2019
@kushaldas kushaldas deleted the fix-flake-4541 branch July 4, 2019 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants