nit: I don't think this is a valid test snapshot. I can't see where this function was run with the parameter False in the code

you are right, I think I missed this while I was doing some refactoring and forgot to change it back again. It should not be False, it should be None indeed, I can update this

As per my previous comment, can we remove this validation if it is not a currently tracked test combination?

Same as I mentioned before, you are right. This should be None not False. The test for False is already existed

nit: we can use logger.debug instead of logger.info here.

The reason why I choose the info is because in the same function for AWSCURRENT and others log levels we are also using info so I thought this was added in purpose, so I stick to use info in this case

This is only in the lambda function body though right @macnev2013?

Yes, that’s right @simonrw. This can be ignored.

better to add default value in case rotation_period is not set

I added this in purpose without or 0 because usually the default value for https://github.com/localstack/moto/blob/localstack/moto/secretsmanager/models.py#L118 based on this logic usually is 0 in case its not set.

For extra check, we can still also fallback to 0
secret.auto_rotate_after_days = rotation_period or 0

I will update this

nit: we could move this to localstack.testing.snapshots.transformer_utility.TransformerUtility.secretsmanager_api since it's a geneic transformer.

Definitely, I can move it there to make it a generic transformer since RotationLambdaARN could be user later on with multiple tests. define it there could be useful later on

this could cause 'rotation_period' variable to be referenced before assignment here

Yeah thats right, I will init rotation_period = 0

Is it always 0 or can we move the rotation_period = secret.auto_rotate_after_days above the if block and have that as the default, then remove the else branch?

I can remove the else branch and make it the default one. However, I will fallback to 0 like this

rotation_period = secret.auto_rotate_after_days or 0

That way even if previous value for for auto_rotate_after_days that is part of secret is not integer, we are make sure always if the value is not set the default one is 0

What if the secret is being rotated first time and we haven't specified the rotation_lambda_arn?

Actually, this was an existing behavior even before I introduce my fix. In case for the first time rotation we did not specify rotation_lambda_arn then it will raise error, check this code here

    try:
        lm_client = connect_to(region_name=self.region_name).lambda_
        lm_client.get_function(FunctionName=rotation_lambda_arn)
    except Exception:
        raise ResourceNotFoundException("Lambda does not exist or could not be accessed")

This will raise error whenever we invoked it from the cli

An error occurred (ResourceNotFoundException) when calling the RotateSecret operation: Lambda does not exist or could not be accessed

What error message does AWS return in this case? Does it match the error we raise in this circumstance? I suspect not.

you are right. I will update the code because I need to add a check earlier to raise an error that match AWS one

The error mainly look like this from AWS

        "Error": {
          "Code": "InvalidRequestException",
          "Message": "No Lambda rotation function ARN is associated with this secret."
        },

suggestion: we usually don't use assertion in the helper methods.

Is it a pattern you followed when you write tests ? or you are talking in general ?. The reason why I add this method is to avoid duplication for multiple tests and I thought usually this could be re-used only within multiple tests for assertions. However, I'm open to change this if you think this could create confusion or violates any sort of patterns/convention you usually followed

nit: we normally prefer two things:

1. assertions in the test body, not in helper methods
2. fixtures over helper methods

I personally find it clearer for the assertion failure to occur in the test body so it's clear what's being tested at first glance in the test code itself, rather than having to go diving into helper methods. It also makes the helper methods more flexible, as the assertions may not be consistent from test to test.

For the second item, we often create fixtures that look like:

def my_fixture(aws_client, snapshot):  # other fixtures
    def inner(name: str, *args, **kwargs):  # variables taken from the test body
        # do something with both arguments and fixtures, e.g.:
        return aws_client.sts.get_caller_identity()
    
    return inner

then in the test we only need to include the fixture and use the "inner" function, which simplifies the usage of that fixture:

def test_foo(my_fixture):
    identity = my_fixture("name")

See how we could call AWS methods without passing in the aws_client.

These are just preferences and I'm not asking you to refactor the helper methods into fixtures, but perhaps you could consider moving the assertions to the test bodies?

Make sense, I think you raised a valid point here. I will do some refactoring to this

It would be worth testing - what happens if the function_arn is not provided for the first time rotation.

Sure thing, I can add this as a new test since it will fail if we did not specify the function_arn. We do not have a test for this case but the rotation backend can handle that as expected based on my previous comment. I will add a test to catch this

What error message does AWS return in this case? Does it match the error we raise in this circumstance? I suspect not.

nit: we normally prefer two things:

1. assertions in the test body, not in helper methods
2. fixtures over helper methods

I personally find it clearer for the assertion failure to occur in the test body so it's clear what's being tested at first glance in the test code itself, rather than having to go diving into helper methods. It also makes the helper methods more flexible, as the assertions may not be consistent from test to test.

For the second item, we often create fixtures that look like:

def my_fixture(aws_client, snapshot):  # other fixtures
    def inner(name: str, *args, **kwargs):  # variables taken from the test body
        # do something with both arguments and fixtures, e.g.:
        return aws_client.sts.get_caller_identity()
    
    return inner

then in the test we only need to include the fixture and use the "inner" function, which simplifies the usage of that fixture:

def test_foo(my_fixture):
    identity = my_fixture("name")

See how we could call AWS methods without passing in the aws_client.

These are just preferences and I'm not asking you to refactor the helper methods into fixtures, but perhaps you could consider moving the assertions to the test bodies?

nit: these assertions are not required giving we are matching the snapshot above

This test is a start, however it does not execute the rotation itself, therefore testing the error message given by AWS if a lambda function arn is not specified. This will help clarify the error handling here.

Here is a demonstration of the confusion that extracting these helper methods causes. I (incorrectly) believed that you hadn't tried to rotate the lambda function, but when I looked in the snapshot I saw that the _setup_invalid_rotation_secret method tries to rotate a secret without specifying a function ARN.

I would find it much clearer if the test was more like:

• create secret (using our create_secret fixture)
• try to rotate secret, wrapped in pytest.raises(...)
• capturing a snapshot of the error message
• (optional) a describe secret call, however this is covered by other more specific tests

Now I have been more lean toward refactor this since it created this confusion. Thanks for raising this

q: do you understand why this changed?

My assumption for this. The list is not deterministic as I noticed that sometimes version1 could be the previous version and version 2 is the current version and vice versa. Thats the reason why sometimes version 1 could be previous and sometimes it could be current. The same thing for version 2 as well

issue: you have skipped the key part of this test => the error message. Can you remove this verify and implement the correct behaviour?

Sure thing, I will remove the check it. The only thing that need to be updated is to add a check that raise an error InvalidRequestException that match AWS one

This is only in the lambda function body though right @macnev2013?