VED-803 Refactor filenameprocessor#929
Conversation
JamesW1-NHS
temporarily deployed
to
internal-dev-sandbox
GitHub Actions
Inactive
JamesW1-NHS
temporarily deployed
to
internal-dev-sandbox
GitHub Actions
Inactive
JamesW1-NHS
temporarily deployed
to
internal-dev-sandbox
GitHub Actions
Inactive
JamesW1-NHS
had a problem deploying
to
internal-dev
GitHub Actions
Failure
JamesW1-NHS
temporarily deployed
to
internal-dev-sandbox
GitHub Actions
Inactive
| ) | ||
| @patch("elasticache.redis_client.hkeys", return_value=["FLU", "RSV"]) | ||
| def test_validate_file_key(self, mock_hkeys, mock_hget): | ||
| def test_validate_file_key(self, mock_get_redis_client): |
There was a problem hiding this comment.
Why this test has been split into three separate ones:
Since refactoring redis_client, the invalid file key tests were getting assertion errors on hkeys('vacc_to_diseases') not being called. In fact, in file_validation.validate_file_key(), we can see that in these cases the error is raised before elasticache.get_valid_vaccine_types_from_cache() (which sets hkeys) is called at all.
It isn't clear why this ever passed in its original form. I suspect an error in the test configuration.
There was a problem hiding this comment.
Yes, I believe the problem here is that they are using .subTest but not resetting the mocks. Unlike between whole tests and where you are using a setUp and tearDown to reset the mocks/patches, the problem here is that they are using a for loop with .subTest. Overall, it's not a very useful or reliable assertion or use of mocks.
JamesW1-NHS
temporarily deployed
to
internal-dev-sandbox
GitHub Actions
Inactive
JamesW1-NHS
temporarily deployed
to
internal-dev
GitHub Actions
Inactive
JamesW1-NHS
temporarily deployed
to
internal-dev
GitHub Actions
Inactive
JamesW1-NHS
temporarily deployed
to
internal-dev-sandbox
GitHub Actions
Inactive
JamesW1-NHS
temporarily deployed
to
internal-dev
GitHub Actions
Inactive
lambdas/shared/src/common/clients.py
Outdated
|
|
||
| firehose_client = boto3_client("firehose", region_name=REGION_NAME) | ||
| secrets_manager_client = boto3_client("secretsmanager", region_name=REGION_NAME) | ||
| lambda_client = boto3_client("lambda", region_name=REGION_NAME) |
There was a problem hiding this comment.
Does the file need this Lambda client? I believe we removed any code referring to it.
There was a problem hiding this comment.
You're quite right. We used to use it so that one of the lambdas could relaunch itself. We have indeed removed that bit of code, so this can go too.
dlzhry2nhs
left a comment
dlzhry2nhs
left a comment
There was a problem hiding this comment.
Looks good - only minor comment relating to removing a client we no longer need.
|
2 participants
Summary
As part of code restructuring.
Refactor the filenameprocessor lambda into lambdas/ and shared/ folders.
Source files which can move to shared:
logging_decorator
clients
errors (part)
Note to testers: This is a refactoring only. There will be no change in functionality or in logging. Smoke testing required only.
Reviews Required
Review Checklist
ℹ️ This section is to be filled in by the reviewer.