Tests: Eligibility API

[angela-tran]

Closes #550

Summary

This PR adds new unit tests for the eligibility app's confirm view function. The Eligibility API is consumed as a part of the logic for confirm - namely, the part that calls client.verify(sub, name).

Tests added

• test_confirm_success
  • Tests the happy path
  • Requires use of working server private key and client public key that correspond with benefits fixture data for creation of mock response. I copied these from eligibility-server
• test_confirm_failure_error_on_request
  • Parameterized test that checks we throw an ApiError when an exception prevents us from successfully sending a request to the API
• test_confirm_failure_unexpected_status_code
  • Tests that unexpected response codes throw an ApiError
• test_confirm_failure_error_tokenizing_request
  • Tests that exception thrown during request tokenization results in a TokenError
• test_confirm_failure_error_tokenizing_response
  • Parameterized test that checks we throw a TokenError when an exception prevents us from tokenizing the response

Notes

These tests try to cover the various success/error paths as much as possible. We can also add tests with more nuanced inputs in the eligibility-api repo when we take on #141.

[angela-tran]

New test is failing in CI because it fails to connect to an eligibility server.

I think we'll either need to

• change the test-pytest workflow to run pytest within a Docker container AND start up and access an eligibility-server container, retracting from the approach taken in Chore: run pytest in CI #454
  OR
• mock the eligibility-server response, which may require pulling in some code for us to then just rip back out when we do Refactor eligibility verification into reusable package #141

[thekaveman]

We definitely don't want pytest tests hitting anything real - e.g. #553 just disabled pytest ability to make external network requests.

Don't worry about refactoring things later for #141 - the task here is to get tests running for the current Eligibility API code.

[afeld]

I think mocking is the way to go, even if we end up moving/copying it to the other repo soon.

[angela-tran]

@thekaveman @afeld How much coverage do you think we should strive for here? There are several error paths with the Eligibility API code - for example, when making the request, when getting back the response, and various points when parsing the response (reading response text , decrypting the signed token, verifying the signature). Just want to make sure we actually want all that coverage before I start writing tests for all those different errors.

Also, I was able to mock the eligibility-server response. Does the approach of using HTTPretty look ok to you?

[angela-tran]

@thekaveman @afeld How much coverage do you think we should strive for here? There are several error paths with the Eligibility API code - for example, when making the request, when getting back the response, and various points when parsing the response (reading response text , decrypting the signed token, verifying the signature). Just want to make sure we actually want all that coverage before I start writing tests for all those different errors.

Also, I was able to mock the eligibility-server response. Does the approach of using HTTPretty look ok to you?

Re: coverage, I decided that the cases related to decrypting/verifying signatures don't need to be done here and would be better/easier to write tests for in the eligibility-api repo.

[afeld]

How much coverage do you think we should strive for here?

I say "write tests until you're sick of it" 😉 Just to throw a number out there, I'm happy around 90% overall and per file (with some lower, as some code isn't easily testable). Arguably should be higher given while we are without error monitoring (#448). That said, some tests are better than no tests (for this part of the codebase), so I would merge this sooner than later.

want to make sure we actually want all that coverage before I start writing tests for all those different errors … cases related to decrypting/verifying signatures don't need to be done here and would be better/easier to write tests for in the eligibility-api repo

You may find the tests easier to write if you refactor smaller functions out of the Eligibility views and write unit tests against those… that refactored code would then more easily get turned into a standalone package. In other words, it may be easiest to build out the eligibility-api package in this repository as just standalone Python files, then move it to the other repository once it's stabilized a bit.

I was able to mock the eligibility-server response.

👏

Does the approach of using HTTPretty look ok to you?

For sure!

[angela-tran]

Ok, thanks for the feedback @afeld! I'll put this PR back into draft and see what additional coverage I can get in.

[angela-tran]

We went from 27% coverage to 95% coverage for eligibility/api.py!

Before	After

[afeld]

Just the one comment. Otherwise, good to go!

[angela-tran]

I'm going to keep this PR open at least through Monday in case @thekaveman wanted to look through it too.

[thekaveman]

I don't want to block since this is definitely better than the low coverage in this area that we had before!

This feels like combining a few different concerns into the same tests though, namely how the form/view work, and how the API client works. I am also not a fan of what is essentially a reimplementation of the token encryption and signing logic as a helper for test setup. That feels... bad.

I would think the view tests should use a fully mocked client, not even getting to the point of having to mock HTTP requests, encryption, etc. These tests would ensure the view logic is correct when various inputs and outputs of the API are processed.

Separate tests for the Eligibility API / client / etc. could get in to the mechanics of working with the request and response tokens, encryption keys, servers, and the like.

[angela-tran]

This feels like combining a few different concerns into the same tests though, namely how the form/view work, and how the API client works.

This is a really good point, and I'm glad you pointed this out. I've created #595 to cover this, which should be done as a part of the Eligibility API refactor (should not be left behind as technical debt).

Thank you @afeld and @thekaveman for your thorough reviews and feedback! 🙏