use snapshots for SNS tests #6601
Conversation
There was a problem hiding this comment.
Looks fantastic, snapshots allover 📸 ✨
I'll let @dominikschubert and @steffyP comment/decide on the changes in the transformer utils.
| TransformerUtility.key_value( | ||
| "SenderId", value_replacement="111111111111", reference_replacement=False | ||
| ), |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
@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?
| TransformerUtility.key_value( | ||
| "Signature", value_replacement="<signature>", reference_replacement=False | ||
| ), | ||
| TransformerUtility.key_value("MD5OfBody", "<md5-hash>", reference_replacement=False), |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
+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).
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I see :) yes, sounds good. but I think it would also be fine to cover the md5-checksum in unittests as you suggested before.
There was a problem hiding this comment.
The md5 is part of SQS, and is covered inside the integration tests there, there shouldn't be any regression on that side.
| TransformerUtility.key_value( | ||
| "Signature", value_replacement="<signature>", reference_replacement=False | ||
| ), |
There was a problem hiding this comment.
should we maybe just use the default value_replacement?
There was a problem hiding this comment.
with the reference_replacement set to false, the default value would be signature. I think it's fine to use <signature> here as it indicates at the first sight that the value was replaced.
There was a problem hiding this comment.
Yes that was my issue, the fact that the <[...]> were missing was troubling me, so I added it manually!
There was a problem hiding this comment.
Nice to see so many snapshot tests 😄 📸 😍
Only one thing that bothers me a bit: the SenderId replacement somehow shadows the regex accountid replacement. Maybe we can improve this?
Regarding the snapshot-PR you are depending on: I am waiting for a review from @dominikschubert and his input :)
| TransformerUtility.key_value("SenderId"), | ||
| # account ID | ||
| TransformerUtility.key_value( | ||
| "SenderId", value_replacement="111111111111", reference_replacement=False |
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This is now changed and simply use reference_replacement=False instead, as it is enough to not mess with ARN anymore
|
Thanks a lot for your reviews, I updated the PR with your comments. |
ee257ab to
25913f1
Compare
25913f1 to
0883522
Compare
This PR adds the use of snapshots for every
aws_validatedtest in the SNS test suite, following #6586We skip some paths for now to be fixed in a later iteration. It is mostly fields not added (or added when it should not be) by Moto.
Also, it seems AWS doesn't add message signature for Fifo topics, so we will need to remove those fields.
I added some exception messages fixes as well.
It contains a hacky fix that will be added in #6598 and will be removed then in order to guarantee the order of the fields.