Use fixtures for test credentials and region name #10207

viren-nadkarni · 2024-02-09T09:41:42Z

Background

This PR makes some code improvements for test credentials and region.

Changes

~~Remove redundant is_aws() helper and use is_aws_cloud() everywhere.~~ Added by Extract snapshot lib #10164
Across our tests, instead of directly importing constants for test account ID and test region name, fixtures are used. New fixtures for secondary test account ID and secondary region name are also introduced.

github-actions · 2024-02-09T09:45:31Z

LocalStack Community integration with Pro

2 files ±0 2 suites ±0 1h 22m 20s ⏱️ + 2m 1s
2 641 tests +1 2 394 ✅ +1 247 💤 ±0 0 ❌ ±0
2 643 runs +1 2 394 ✅ +1 249 💤 ±0 0 ❌ ±0

Results for commit f56c80d. ± Comparison against base commit b5c5603.

♻️ This comment has been updated with latest results.

github-actions · 2024-02-09T09:47:35Z

S3 Image Test Results (AMD64 / ARM64)

2 files ±0 2 suites ±0 3m 16s ⏱️ -1s
386 tests ±0 336 ✅ ±0 50 💤 ±0 0 ❌ ±0
772 runs ±0 672 ✅ ±0 100 💤 ±0 0 ❌ ±0

Results for commit f56c80d. ± Comparison against base commit b5c5603.

♻️ This comment has been updated with latest results.

viren-nadkarni · 2024-02-12T12:58:39Z

localstack/testing/pytest/snapshot.py

-@pytest.fixture(name="region", scope="session")
-def fixture_region(aws_client):
-    if is_aws() or is_api_enabled("sts"):
-        return aws_client.sts.meta.region_name
-    else:
-        return TEST_AWS_REGION_NAME


This is moved to localstack.testing.pytest.fixtures alongside its companion fixtures

coveralls · 2024-02-13T06:46:00Z

coverage: 83.83% (-0.004%) from 83.834%
when pulling f56c80d on unify-fixtures
into b5c5603 on master.

dominikschubert · 2024-02-13T11:12:17Z

localstack/services/s3/presigned_url.py

-    ex.AWSAccessKeyId = TEST_AWS_ACCESS_KEY_ID  # todo maybe return access key used for the request
+    ex.AWSAccessKeyId = (
+        constants.TEST_AWS_ACCESS_KEY_ID
+    )  # todo maybe return access key used for the request


In this and the other cases in this file, I'd suggest we just remove it completely (basically implementing the TODO)

Yes, I plan to remove all uses of the TEST_* variables outside of the tests themselves. I've made some of it in #8204, would it be okay if I take it forward there?

For the presigned URLs, we will need a bit more logic especially when creating the Credentials to validate the URL and remove those constants. Right now, we explicitly say that we need the credentials to be test / test in order to validate them (so technically the test should fail when using other credentials, which is why I believe we're using TEST_* and not DEFAULT_*).

The secret access key is not passed in the signature, so we will need a network request to fetch it like IAM does, in order to properly validate it. But I agree that implementing this TODO would be really great!

Let me know if there's anything I can do there.

dominikschubert · 2024-02-13T11:16:15Z

localstack/testing/pytest/fixtures.py

+
+@pytest.fixture(scope="session")
+def secondary_region_name():
+    return constants.SECONDARY_TEST_AWS_REGION_NAME


Not a particular fan of using the constants for the fixtures. One of the longer-running efforts will be moving to a state where we won't have access to localstack internals from the test code (e.g. because we're running LS in a container and test against that). I also think constants should generally not be a place where we put values used in testing.

How about we use environment variables to configure these and falling back to a static default in the fixture?

Actually, this value already gives precedence to the env variable

https://github.com/localstack/localstack/blob/unify-fixtures/localstack/constants.py#L159-L163

I admit that its place in localstack.constants is misleading.

Would it be okay if I add a todo note and make the changes in a follow up PR, given how large this PR is already?

This reverts commit 517e149.

This reverts commit e9f9031.

This reverts commit 3406891.

dominikschubert

LGTM, didn't spot any obvious issues.

I have a bit of a concern in terms of using this fixture for unit tests, though. I think keeping those static is fine

dominikschubert · 2024-02-15T16:30:49Z

tests/unit/aws/test_connect.py

-            region_name="us-east-1",
+            region_name=region_name,


I'm not so sure we want to do this with unit tests. The behavior of the underlying region lookup seems to be not as well defined here 🤔

You're right, I picked this from #8204 but I'm unable to reproduce the original reason for the change. Reverted for now, I'll revisit this if it reoccurs.

dominikschubert · 2024-02-15T16:31:50Z

tests/aws/services/s3/test_s3.py

        # Boto still does not support chunk encoding, which means we can't test with the client nor
        # aws_http_client_factory. See open issue: https://github.com/boto/boto3/issues/751
        # Test for https://github.com/localstack/localstack/issues/1571
        object_key = "data"
        body = "Hello\r\n\r\n\r\n\r\n"
        headers = {
            "Authorization": mock_aws_request_headers(
-                "s3", aws_access_key_id=TEST_AWS_ACCESS_KEY_ID, region_name=TEST_AWS_REGION_NAME
+                "s3",
+                aws_access_key_id=TEST_AWS_ACCESS_KEY_ID,


we should be able to get this via STS as well,so might be worth to make this a fixture too

Do you mean STS:GetSessionToken? I'll make a note, thanks!

This reverts commit 97de34e.

viren-nadkarni self-assigned this Feb 9, 2024

viren-nadkarni added the semver: patch Non-breaking changes which can be included in patch releases label Feb 9, 2024

viren-nadkarni force-pushed the unify-fixtures branch from 182664f to b617a8f Compare February 9, 2024 14:23

viren-nadkarni added 12 commits February 12, 2024 15:26

Add region_name fixture

3474159

Remove the old fixture_region

650f35a

Replace direct import of TEST_AWS_REGION_NAME in tests with fixture

7fac98a

Replace direct import of TEST_AWS_ACCOUNT_ID in tests with fixture

734e661

Use module attribute referencing instead of direct imports

3406891

Remove is_aws() and use is_aws_cloud() instead

f7c6439

Introduce and use the fixture secondary_region_name

a56db6b

Use secondary_account_id fixture instead of constant import

6803063

Move is_aws_cloud() to localstack.testing.aws.core

e31992f

Use backward compatible hints

8d98264

Avoid direct imports

e9f9031

Revert accidental renames

f36c3db

viren-nadkarni force-pushed the unify-fixtures branch from b617a8f to 0b505a3 Compare February 12, 2024 10:09

Fix secondary_region_name fixture

d812bbb

viren-nadkarni force-pushed the unify-fixtures branch from 0b505a3 to d812bbb Compare February 12, 2024 10:13

Fix parametrised test using confusing args

27e4cc7

viren-nadkarni commented Feb 12, 2024

View reviewed changes

Merge branch 'master' into unify-fixtures

98d6f88

viren-nadkarni force-pushed the unify-fixtures branch from 7278ca5 to 98d6f88 Compare February 13, 2024 05:33

Remove direct imports

517e149

viren-nadkarni force-pushed the unify-fixtures branch 2 times, most recently from 424a97c to 934e5bd Compare February 13, 2024 09:22

Revert move of is_aws_cloud()

db3fd2b

viren-nadkarni force-pushed the unify-fixtures branch from 934e5bd to db3fd2b Compare February 13, 2024 09:24

viren-nadkarni requested review from pinzon, simonrw and calvernaz as code owners February 13, 2024 10:39

viren-nadkarni removed request for simonrw, calvernaz, thrau, steffyP, giograno, joe4dev, silv-io, macnev2013, dfangl, pinzon, sannya-singal, bentsku, MEPalma and baermat February 13, 2024 10:39

dominikschubert reviewed Feb 13, 2024

View reviewed changes

viren-nadkarni added 6 commits February 14, 2024 19:58

Merge branch 'master' into unify-fixtures

7320597

Revert "Remove direct imports"

1a8ff7b

This reverts commit 517e149.

Revert "Avoid direct imports"

cab3a26

This reverts commit e9f9031.

Revert "Use module attribute referencing instead of direct imports"

afc1bc9

This reverts commit 3406891.

Use region_name fixture in test_connect.py

97de34e

Revert ineffect change

470a177

viren-nadkarni requested a review from dominikschubert February 15, 2024 12:59

dominikschubert approved these changes Feb 15, 2024

View reviewed changes

Revert "Use region_name fixture in test_connect.py"

f56c80d

This reverts commit 97de34e.

viren-nadkarni merged commit f58f922 into master Feb 16, 2024

viren-nadkarni deleted the unify-fixtures branch February 16, 2024 09:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Use fixtures for test credentials and region name #10207

Use fixtures for test credentials and region name #10207

Uh oh!

viren-nadkarni commented Feb 9, 2024 •

edited

Loading

Uh oh!

github-actions bot commented Feb 9, 2024 •

edited

Loading

Uh oh!

github-actions bot commented Feb 9, 2024 •

edited

Loading

Uh oh!

viren-nadkarni Feb 12, 2024

Uh oh!

coveralls commented Feb 13, 2024 •

edited

Loading

Uh oh!

dominikschubert Feb 13, 2024

Uh oh!

viren-nadkarni Feb 13, 2024

Uh oh!

bentsku Feb 13, 2024

Uh oh!

dominikschubert Feb 13, 2024

Uh oh!

viren-nadkarni Feb 13, 2024

Uh oh!

dominikschubert left a comment

Uh oh!

dominikschubert Feb 15, 2024

Uh oh!

viren-nadkarni Feb 16, 2024

Uh oh!

dominikschubert Feb 15, 2024

Uh oh!

viren-nadkarni Feb 16, 2024 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Use fixtures for test credentials and region name #10207

Use fixtures for test credentials and region name #10207

Uh oh!

Conversation

viren-nadkarni commented Feb 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Background

Changes

Uh oh!

github-actions bot commented Feb 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

LocalStack Community integration with Pro

Uh oh!

github-actions bot commented Feb 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

S3 Image Test Results (AMD64 / ARM64)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Feb 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dominikschubert left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

viren-nadkarni Feb 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

viren-nadkarni commented Feb 9, 2024 •

edited

Loading

github-actions bot commented Feb 9, 2024 •

edited

Loading

github-actions bot commented Feb 9, 2024 •

edited

Loading

coveralls commented Feb 13, 2024 •

edited

Loading

viren-nadkarni Feb 16, 2024 •

edited

Loading