Refactor e2e tests to use assertion helper class

[brycahta]

Issue #, if available: N/A

Description of changes:

• Adds a helper class, Ec2Validator, to assert resource data/state against aws apis more easily
• This refactor will also make implementing resource reference e2e test easier (follow-up PR)

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

[RedbackThomson]

While I think checking for existence is useful, I believe we can harden our tests by checking their spec matches. Especially for code with custom hooks. Perhaps, we should have methods in the EC2Validator that get the resources, and then we can just check assert dhcp_options is not None (and all of our spec assertions as well).

[brycahta]

I agree with raising the quality of these tests, but should be addressed in a separate PR/issue. Do you know of an issue (or parent issue) to improve e2e testing or should I open one for this controller?

Once this is merged, my plan is to open a follow-up PR with a resource-reference e2e test modeled after ApiGatewayV2 (checks existence of referenced resources) so we can get some coverage on the feature. Then, I can work on improving e2e tests to check resources at a more granular level across the entire suite. Lmk if you have any other context (i.e. if some automated test-gen in the works, you prefer to finish vpc enhancements before e2e test improvements, etc.) or if you think it can be approached better.

[RedbackThomson]

Do you know of an issue (or parent issue) to improve e2e testing or should I open one for this controller?

This is on a per-controller basis. So if there isn't one for EC2, you can create one. No automated test-gen in the works atm :(

[brycahta]

ec2-kind-e2e — Job failed.

TransitGateway test failed because the resource still exists, but in the 'deleting' state. I checked the account and it has been deleted. This also happened to me when I tested locally, but was hoping it was transient. Not sure why it's occurring now, so will deep dive and improve the test in next revision.

Logs

tests/test_transit_gateway.py:77: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <e2e.tests.helper.Ec2Validator object at 0x7f57cafb6070>
tgw_id = 'tgw-0bbc70e488bf84a9a', exists = False

    def assert_transit_gateway(self, tgw_id: str, exists=True):
        res_found = False
        try:
            aws_res = self.ec2_client.describe_transit_gateways(TransitGatewayIds=[tgw_id])
            res_found = len(aws_res["TransitGateways"]) > 0
        except self.ec2_client.exceptions.ClientError:
            pass
>       assert res_found is exists
E       AssertionError

tests/helper.py:85: AssertionError
=========================== short test summary info ============================
FAILED tests/test_transit_gateway.py::TestTGW::test_create_delete - Assertion...
=================== 1 failed, 13 passed in 120.92s (0:02:00) ===================

[brycahta]

/retest

[RedbackThomson]

Only merging this because you promised me better tests in the near future haha. Counting on it!

/lgtm

[ack-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brycahta, RedbackThomson

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 [RedbackThomson,brycahta]

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