Refactor all e2e tests to use fixture #49

a-hilaly · 2022-08-29T16:19:25Z

Refactoring e2e tests to use pytest.fixture for the resource under test.
Otherwise, assertion failures within the test may not lead to successful
resource cleanup.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

ack-bot · 2022-08-29T16:19:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: A-Hilaly

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [A-Hilaly]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Refactoring e2e tests to use pytest.fixture for the resource under test. Otherwise, assertion failures within the test may not lead to successful resource cleanup.

RedbackThomson · 2022-09-06T21:41:34Z

test/e2e/conftest.py

+@dataclass
+class ResourceWaitTimes:
+    Create: int
+    Update: int
+    Delete: int


Could be defined in the test-infra package. Also, we should probably provide sane defaults (say 5 seconds for each?)

RedbackThomson · 2022-09-06T21:42:00Z

test/e2e/conftest.py

+class ServiceWaitTimes:
+    Function: ResourceWaitTimes
+    Alias: ResourceWaitTimes
+    CodeSigningConfig: ResourceWaitTimes
+    EventSourceMapping: ResourceWaitTimes
+    FunctionURLConfig: ResourceWaitTimes
+
+Wait = ServiceWaitTimes(
+    Function = ResourceWaitTimes(30, 30, 30),
+    Alias = ResourceWaitTimes(10, 10, 10),
+    CodeSigningConfig = ResourceWaitTimes(10, 10, 10),
+    EventSourceMapping = ResourceWaitTimes(20, 20, 20),
+    FunctionURLConfig = ResourceWaitTimes(30, 10, 10),
+)


I think I'd prefer to see each of these defined in their respective test files. They can be cross-imported if we need to access them in other tests.

RedbackThomson · 2022-09-06T21:43:19Z

test/e2e/conftest.py

@@ -32,6 +72,12 @@ def pytest_configure(config):
    config.addinivalue_line(
        "markers", "slow: mark test as slow to run"
    )
+    config.addinivalue_line(
+        "markers", "function_overrides: function parameters to override when creating fixture"


PyTest markers are used to label a test - not to provide a parameter to tests. For instance, we have a marker for "slow" tests, in case you want to skip them. I would prefer if we just hardcode the test values, or use another mechanism for parameterising these

RedbackThomson · 2022-09-06T21:44:02Z

test/e2e/conftest.py

@@ -44,8 +90,259 @@ def pytest_collection_modifyitems(config, items):
 # Provide a k8s client to interact with the integration test cluster
 @pytest.fixture(scope='class')
 def k8s_client():
-    return k8s._get_k8s_api_client()
+    return k8sclient._get_k8s_api_client()


Just for the sake of consistency with the other tests, would you mind changing this back to k8s. In conjunction with my comments below, this shouldn't conflict.

RedbackThomson · 2022-09-06T21:45:18Z

test/e2e/conftest.py

+    return boto3.client('lambda', region_name=get_region())
+
+@pytest.fixture


Pytest fixtures should be defined in whichever scope they're used. I defined lambda_client and k8s_client in this file because they can be used by every/any test. These fixtures below are specific to individual tests (function, code signing, etc.). I'd prefer if we moved these to the test_*.py files. If we need to re-use the fixture as part of another test, it would be better to just import them across files.

RedbackThomson · 2022-09-06T21:46:07Z

test/e2e/replacement_values.py

 REPLACEMENT_VALUES = {
-
+    "AWS_REGION": get_region()


This is globally defined, as part of acktest - no need to redefine it here. https://github.com/aws-controllers-k8s/test-infra/blob/main/src/acktest/resources.py#L32

RedbackThomson · 2022-09-06T21:48:32Z

test/e2e/tests/test_event_source_mapping.py

+    @pytest.mark.function_overrides({
+        'package_type': 'Zip',
+        'role_type': 'esm',
+    })
+    @pytest.mark.esm_overrides({'source': 'ddbtable'})


What's the purpose of these markers? Do they interact with the fixtures?

eks-bot · 2022-12-05T22:02:52Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close.
Provide feedback via https://github.com/aws-controllers-k8s/community.
/lifecycle stale

a-hilaly · 2022-12-06T14:25:35Z

/lifecycle frozen

ack-bot · 2022-12-06T14:25:36Z

@a-hilaly: The lifecycle/frozen label cannot be applied to Pull Requests.

In response to this:

/lifecycle frozen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

a-hilaly · 2022-12-06T14:25:47Z

/remove-lifecycle stale

ack-prow · 2023-05-18T16:55:46Z

@a-hilaly: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
lambda-metadata-file-test	`de394af`	link	true	`/test lambda-metadata-file-test`
lambda-codespell	`de394af`	link	true	`/test lambda-codespell`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

ack-bot · 2023-11-14T17:26:36Z

Issues go stale after 180d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 60d of inactivity and eventually close.
If this issue is safe to close now please do so with /close.
Provide feedback via https://github.com/aws-controllers-k8s/community.
/lifecycle stale

ack-bot · 2024-01-13T18:43:15Z

Stale issues rot after 60d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 60d of inactivity.
If this issue is safe to close now please do so with /close.
Provide feedback via https://github.com/aws-controllers-k8s/community.
/lifecycle rotten

ack-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 29, 2022

ack-bot requested review from jaypipes and RedbackThomson August 29, 2022 16:19

ack-bot added the approved label Aug 29, 2022

a-hilaly force-pushed the e2e-refactor branch 12 times, most recently from a7dd9ff to 0fe7c71 Compare August 31, 2022 16:21

Refactor all e2e tests to use fixture

de394af

Refactoring e2e tests to use pytest.fixture for the resource under test. Otherwise, assertion failures within the test may not lead to successful resource cleanup.

a-hilaly force-pushed the e2e-refactor branch from 0fe7c71 to de394af Compare August 31, 2022 16:45

a-hilaly changed the title ~~[WIP] Refactor all e2e tests to use fixture~~ Refactor all e2e tests to use fixture Aug 31, 2022

ack-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 31, 2022

RedbackThomson suggested changes Sep 6, 2022

View reviewed changes

ack-bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 5, 2022

ack-bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 6, 2022

ack-prow bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 14, 2023

ack-prow bot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jan 13, 2024

a-hilaly removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Jan 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor all e2e tests to use fixture #49

Refactor all e2e tests to use fixture #49

a-hilaly commented Aug 29, 2022

ack-bot commented Aug 29, 2022

RedbackThomson Sep 6, 2022

RedbackThomson Sep 6, 2022

RedbackThomson Sep 6, 2022

RedbackThomson Sep 6, 2022

RedbackThomson Sep 6, 2022

RedbackThomson Sep 6, 2022

RedbackThomson Sep 6, 2022

eks-bot commented Dec 5, 2022

a-hilaly commented Dec 6, 2022

ack-bot commented Dec 6, 2022

a-hilaly commented Dec 6, 2022

ack-prow bot commented May 18, 2023

ack-bot commented Nov 14, 2023

ack-bot commented Jan 13, 2024

		return boto3.client('lambda', region_name=get_region())

		@pytest.fixture

Refactor all e2e tests to use fixture #49

Are you sure you want to change the base?

Refactor all e2e tests to use fixture #49

Conversation

a-hilaly commented Aug 29, 2022

ack-bot commented Aug 29, 2022

RedbackThomson Sep 6, 2022

Choose a reason for hiding this comment

RedbackThomson Sep 6, 2022

Choose a reason for hiding this comment

RedbackThomson Sep 6, 2022

Choose a reason for hiding this comment

RedbackThomson Sep 6, 2022

Choose a reason for hiding this comment

RedbackThomson Sep 6, 2022

Choose a reason for hiding this comment

RedbackThomson Sep 6, 2022

Choose a reason for hiding this comment

RedbackThomson Sep 6, 2022

Choose a reason for hiding this comment

eks-bot commented Dec 5, 2022

a-hilaly commented Dec 6, 2022

ack-bot commented Dec 6, 2022

a-hilaly commented Dec 6, 2022

ack-prow bot commented May 18, 2023

ack-bot commented Nov 14, 2023

ack-bot commented Jan 13, 2024