Skip to content
This repository has been archived by the owner on Jan 7, 2024. It is now read-only.

Fix TOTP retry logic, improve cassette handling; document #135

Merged
merged 2 commits into from
Oct 13, 2020

Conversation

eloquence
Copy link
Member

@eloquence eloquence commented Oct 8, 2020

As part of working on #134 I noticed that setUp in test_api.py was not working as expected: when regenerating the cassette, authentication would either succeed, or it would fail and keep failing. The TOTP code was not successfully re-retrieved.

The logic does not work as currently written because 1) it would keep using the same TOTP code if it actually looped, which it doesn't because, 2) it will quietly exit the loop after sleeping.

Because we're already using a cassette to record the API response(s), there's really no reason for an additional utility method to stash and load it -- we just need to make sure we don't run sleeps when we're in playback mode.

Also fixes a path error for the RPC policy changes documented in the README.

Status

Ready for review. CI will fail until #133 and #136 land.

Test plan

Reproduce the error

On the main branch, run the following command in your virtualenv:

rm data/test-setup.yml && python -m pytest -v -k test_api_auth

What this command does: remove the setup cassette and attempt to obtain a fresh, valid API token

  • Observe: Expected behavior: test success. Actual behavior (usually, initially): test success

Run it again once or twice, in quick succession.

  • Observe: Expected behavior: test success. Actual behavior: long sleep, test failure.

Reproduce the fix

Perform the same steps on this branch.

  • Observe that the error no longer occurs, and that the API test successfully completes.

No sleep during playback

Check that the file data/test-setup.yml still contains a 403 response from the previous run. If not, re-run until you observe a sleep, then preserve data/test-setup.yml with a 403 in it.

Run all tests from the cassettes (reminded that dev env needs to be restarted between full test runs, due to the deletion related tests):

make test TESTS=tests/test_api.py
  • Observe that all tests pass
  • Observe that in spite of the 403 in the previous recorded auth response, there is no 31 second sleep during test execution.

try:
self.api.authenticate()
except BaseError:
except AuthError:
Copy link
Member Author

Choose a reason for hiding this comment

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

AuthError is the more specific, occasionally expected error, so it seemed the more appropriate one to catch here so we don't accidentally swallow other useful exceptions.

@eloquence eloquence marked this pull request as ready for review October 8, 2020 05:12
@eloquence eloquence changed the title Fix TOTP retry logic in HTTP test setup; document Fix TOTP retry logic, improve caassette handling; document Oct 8, 2020
@eloquence
Copy link
Member Author

Tweaked this further to remove utility methods we don't really need. Ready for review but would be good to land CI fixes first so we can rebase and get a clean CI run in.

@eloquence eloquence changed the title Fix TOTP retry logic, improve caassette handling; document Fix TOTP retry logic, improve cassette handling; document Oct 8, 2020
@eloquence eloquence mentioned this pull request Oct 9, 2020
2 tasks
@kushaldas kushaldas self-assigned this Oct 13, 2020
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.

After following the standard test steps from the PR description, I did the following steps:

  • rm data/*.yml
  • make test TESTS=tests/test_api.py

This is good for me, approved.

@kushaldas kushaldas merged commit e32d3e8 into main Oct 13, 2020
@kushaldas kushaldas deleted the fix-auth-setup branch October 13, 2020 13:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants