8000 use snapshots for SNS tests by bentsku · Pull Request #6601 · localstack/localstack · GitHub
[go: up one dir, main page]
Skip to content

use snapshots for SNS tests #6601

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Aug 9, 2022
Merged

use snapshots for SNS tests #6601

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3318c00
started snapshot tests for SNS
bentsku Aug 2, 2022
41d4935
fix change
bentsku Aug 4, 2022
9fe2785
add SNS transformer utility, move some SQS into SNS one
bentsku Aug 5, 2022
5f6f684
fix SNS error messages parity
bentsku Aug 5, 2022
0883522
regenerate snapshots with ordering
bentsku Aug 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
add SNS transformer utility, move some SQS into SNS one
  • Loading branch information
@bentsku
bentsku committed Aug 9, 2022
commit 9fe27854af37724335cabb30ffcbc8245d51e7fa
26 changes: 10 additions & 16 deletions localstack/testing/snapshots/transformer_utility.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,10 @@ def sqs_api():
"""
return [
TransformerUtility.key_value("ReceiptHandle"),
TransformerUtility.key_value("SenderId"),
# account ID
TransformerUtility.key_value(
"SenderId", value_replacement="111111111111", reference_replacement=False
Copy link
Member

Choose a reason for hiding this comment

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

@bentsku: is there a specific reason why you decided to replace the senderId with 1s? IMO it can be a bit misleading, as we also have the regex-replacement for the account-id, and on first sight it will look like the regex-replacement is responsible for this replacement.

Personally, I would prefer if we could use distinct replacements values for transformers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am going to test it again, but I believe the issue was that locally, the SenderId was replaced by 1s, and not in the tests against AWS. I'll check again, could be priorities, I need to check what the response was before transformation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is now changed and simply use reference_replacement=False instead, as it is enough to not mess with ARN anymore

),
Copy link
Member

Choose a reason for hiding this comment

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

I had a similar issue in #6603. It seems related to the the fact that the SenderId of an SQS message can sometimes be the root account ID, and sometimes the access key id, see. https://stackoverflow.com/a/28603793/804840
This then messes up the comparisons.

Any idea what a good pattern is for this @steffyP @dominikschubert ?

Copy link
Member

Choose a reason for hiding this comment

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

@thrau: Interesting :) I think a key-value replacement should be fine in that case. We could also include a regex-check to make sure it matches one of the two valid patterns. but seems like a overhead in that case.

Only thing that feels a bit hacky here: if we replace it with 1s, one could assume that the region-regex-replacement was used here - which is not the case. @bentsku is there a particular reason why you have chosen this value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See #6601 (review)

TransformerUtility.jsonpath("$..MessageAttributes.RequestID.StringValue", "request-id"),
KeyValueBasedTransformer(_resource_name_transformer, "resource"),
]
Expand All @@ -184,18 +187,15 @@ def sns_api():
:return: array with Transformers, for sns api.
"""
return [
TransformerUtility.key_value("SequenceNumber"), # this might need to be in SQS
TransformerUtility.key_value(
"Signature", value_replacement="<signature>", reference_replacement=False
),
# maybe need check why hashes are different
TransformerUtility.key_value(
"MD5OfMessageAttributes",
value_replacement="<md5-hash>",
reference_replacement=False,
),
TransformerUtility.key_value("MD5OfBody", "<md5-hash>", reference_replacement=False),
Copy link
Member

Choose a reason for hiding this comment

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

should we not maybe leave this in per default? or are you having issues with bodies containing data that is dynamically generated by the tests? (like passing ARNs through a message body).

Copy link
Member

Choose a reason for hiding this comment

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

+1
If the md5 hash matches in most cases, we should not always exclude it. Is this the case for SNS?
Ideally we would only exclude md5-hashes for individual test cases with a note why it has to be replaced (e.g. includes timestamp).

Copy link
Contributor Author
@bentsku bentsku Aug 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The MD5 hash almost never matches: the body of the SNS message always contains a timestamp, and a signature.
It only matches when RawDelivery is set to True, as the body is the string message passed. I had to manually add the MD5 transformer to almost every test. I thought the MD5 hash could just be checked with a unit test.
Otherwise, I had an over engineered way to do, which was to delete the MD5 transformer from the sns_api with a fixture that I would add to the test.
What do you think we should do in this case?

Copy link
Member

Choose a reason for hiding this comment

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

alright :) i think in that case, having unittests should be fine.
alternatively we could also create another api-helper, which excludes the md5-transformer and use this one for the RawDelivery messages

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it could work as well. The issue also that I didn't think about is that in some tests, we have both RawDelivery and regular in the same test. So I have to ignore the hash. Maybe I will manually check the MD5 of raw messages in the integration tests? As it's AWS validated, it will check the behaviour when we regenerate the snapshots

Copy link
Member

Choose a reason for hiding this comment

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

I see :) yes, sounds good. but I think it would also be fine to cover the md5-checksum in unittests as you suggested before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The md5 is part of SQS, and is covered inside the integration tests there, there shouldn't be any regression on that side.

KeyValueBasedTransformer(_sns_unsubscribe_url_token_transformer, replacement="token"),
KeyValueBasedTransformer(
_sns_pem_file_token_transformer, replacement="signing-cert-file"
_sns_pem_file_token_transformer,
replacement="signing-cert-file",
),
# replaces the domain in "UnsubscribeURL" URL (KeyValue won't work as it replaces reference, and if
# replace_reference is False, then it replaces the whole key
Expand All @@ -204,13 +204,7 @@ def sns_api():
RegexTransformer(
r"(?<=(?i)UnsubscribeURL[\"|']:\s[\"|'])(https?.*?)(?=/\?Action=Unsubscribe)",
replacement="<unsubscribe-domain>",
)
# TransformerUtility.regex(
# re.compile(
# r"(?<=(?i)UnsubscribeURL[\"|']:\s[\"|'])(https?.*?)(?=/\?Action=Unsubscribe)"
# ),
# replacement="<unsubscribe-domain>",
# ),
),
]

@staticmethod
Expand Down Expand Up @@ -245,7 +239,7 @@ def secretsmanager_secret_id_arn(create_secret_res: CreateSecretResponse, index:

def _sns_pem_file_token_transformer(key: str, val: str) -> str:
if isinstance(val, str) and key.lower() == "SigningCertURL".lower():
pattern = re.compile(r".*SimpleNotificationService-(.*)?\.pem")
pattern = re.compile(r".*SimpleNotificationService-(.*\.pem)")
match = re.match(pattern, val)
if match:
return match.groups()[0]
Expand Down
0