Migrate amazon provider hooks tests from unittests to pytest

[Taragolis]

related: #27970

Migrate Amazon provider's hooks and utils tests to pytest.

All changes are more or less straightforward:

• Get rid of unittests.TestCase class and TestCase.assert* methods
• Replace decorator parameterized.expand by pytest.mark.parametrize.
• Convert setUp* and tearDown* methods to appropriate pytest alternative

See additional findings, info about significant changes and potential follow up in comments

[Taragolis]

This simple change actually speedup test execution from 30 sec to 2 seconds

before

============================ slowest 100 durations =============================
  30.03s call     tests/providers/amazon/aws/hooks/test_emr_containers.py::TestEmrContainerHook::test_query_status_polling_with_timeout

after

2.01s call     tests/providers/amazon/aws/hooks/test_emr_containers.py::TestEmrContainerHook::test_query_status_polling_with_timeout

[vandonr-amz]

nice, but do we need to wait at all ? is poll_interval = 0 an option ?

[Taragolis]

Actually we do not need to wait at all but better to use freezegun or mock time.sleep however with poll_interval = 0 it would be run just 0.5 sec (localy).

But also it shows that there is no validation for this value. Usual 0 for any wait interval in general not a good idea,
because it could easily converted to something like

while True:
    pass  # 100% core 😢 usage

Or in this case just exceed the limits of AWS API

[ferruzzi]

That's awesome. I was literally looking at this earlier this week and have some local code where I started working on it. 👍 Will review ASAP

[ferruzzi]

Looks great, thanks!

(non-binding approval)

[ferruzzi]

Do we want to remove the given/when/then comments in the very few places they still exist while we're at it? If you don't want to mess with that now, that's understandable too.

[Taragolis]

Right now main target just migrate to pytests.
It is good point to change something from the past 😉
To be honest a lot of test need to rewrite later anyway

[ferruzzi]

Yeah, every time I try do add some new code I find a dozen older bits I'd love to redo in the process. Like why are we mocking get_conn in every test instead of making one fixture to do it? I'll take care of that one at some point. Can't fix it all at once 👍

[ferruzzi]

(No change requested/required, just chatter) I mentioned on slack that I had started on some of these; this was the rabbit hole that got me there... I started updating any hooks that still used get_conn() instead of the conn cached property, which led to updating the mocked values in the unit tests, which led to converting them all to pytest which... never got finished :P So again, thanks for taking this on!

[vincbeck]

Love to see this clean up!

[vandonr-amz]

nice.
I noticed that on some setup_methods you added the optional method parameter without using it, while you skipped it on others. I think it'd be best to omit it wherever possible (i.e. probably everywhere in this case)

[vandonr-amz]

nice, but do we need to wait at all ? is poll_interval = 0 an option ?

[Taragolis]

nice. I noticed that on some setup_methods you added the optional method parameter without using it, while you skipped it on others. I think it'd be best to omit it wherever possible (i.e. probably everywhere in this case)

That's funny but if setup_method or teardown_method decorated by @mock.patсh then method signature (self, method) should use

[vandonr-amz]

nice. I noticed that on some setup_methods you added the optional method parameter without using it, while you skipped it on others. I think it'd be best to omit it wherever possible (i.e. probably everywhere in this case)

That's funny but if setup_method or teardown_method decorated by @mock.patсh then method signature (self, method) should use

Ah yes I imagine because of the order of arguments, but I imagine in test_datasync it's not needed ?
Is it also needed for mocks that "generate" no arguments, like the @mock.patch.dict ?

and nit: I like using _ as name for the arguments that are unused, it's a clear way to mark them as "only here for the compiler to be happy"

[Taragolis]

Original @vandonr-amz

Ah yes I imagine because of the order of arguments, but I imagine in test_datasync it's not needed ?

Actually this arg needed because we decorated entire class

test setup failed
args = (<tests.providers.amazon.aws.hooks.test_datasync.TestDataSyncHookMocked object at 0x109fdf4c0>, <bound method TestData...HookMocked.test_init of <tests.providers.amazon.aws.hooks.test_datasync.TestDataSyncHookMocked object at 0x109fdf4c0>>)
kwargs = {}

    def wrapper(*args, **kwargs):
        self.start(reset=reset)
        try:
>           result = func(*args, **kwargs)
E           TypeError: setup_method() takes 1 positional argument but 2 were given

[Taragolis]

Is it also needed for mocks that "generate" no arguments, like the @mock.patch.dict ?

And even with this case we need 2 arguments

args = (<tests.providers.amazon.aws.hooks.test_batch_client.TestBatchClientDelays object at 0x1071f0f70>, <bound method TestB...tDelays.test_init of <tests.providers.amazon.aws.hooks.test_batch_client.TestBatchClientDelays object at 0x1071f0f70>>)
kw = {}

    @wraps(f)
    def _inner(*args, **kw):
        self._patch_dict()
        try:
>           return f(*args, **kw)
E           TypeError: setup_method() takes 1 positional argument but 2 were given

[Taragolis]

Right now main target just migrate to pytests.
It is good point to change something from the past 😉
To be honest a lot of test need to rewrite later anyway

[Taragolis]

Original @vandonr-amz

and nit: I like using _ as name for the arguments that are unused, it's a clear way to mark them as "only here for the compiler to be happy"

This mostly as reminder what the actual argument here.

[vandonr-amz]

interesting how arguments work with mocks in setup...
Thank you for double-checking.

[ferruzzi]

Thanks for the explanation on the method fixture, that was somewhat confusing. I think the rest of my thoughts were addressed, non-binding approval pending the other folks are cool with their comments.