Migrate tests in google/cloud/sensors from unittest to pytest - #29642
Conversation
d9f46f6 to
44c0a93
Compare
|
@Taragolis @potiuk , another PR for a subtask of issue #29305. This doesnt entirely close the issue, some packages still remain to migrate. TIA |
Change the |
There was a problem hiding this comment.
If we're switching from unittest to pytest then why are we even using test classes at all? We might as well go all the way and make the tests into functions. I'd rather make this change now while we're already modifying the code.
There was a problem hiding this comment.
I hope that classes vs functions it is not new tabs vs spaces 🤣
Personally I do not have any preferences, outside of Airflow I use both of them depends on situation.
If module contain a lot of different functions / classes which need to be tested then classes very nice to use as namespace so developers could easily found tests for specific part, like:
class TestAwesomeHook:
def test_init(self, ...):
...
def test_init_failure_reason_1(self, ...):
...
def test_get_conn(self, ...):
...
class TestAnotherAwesomeHook:
def test_init(self, ...):
...
def test_init_failure_reason_1(self, ...):
...
def test_get_conn(self, ...):
...vs
def test_awesome_hook_init(...):
...
def test_awesome_hook_init_failure_reason_1(...):
...
def test_another_awesome_hook_init(...):
...
def test_another_awesome_hook_get_conn(...):
...
def test_awesome_hook_init_failure_reason_1(...):
...
def test_awesome_hook_get_conn(...):
...There was a problem hiding this comment.
especially in the case of providers, I feel that class-based tests help because we have many operators/sensors in a single file so if tests are class-based it improves the readability of tests and adding/removing/updating/searching tests became a bit easy. otherwise, If I'll make small changes in the existing operator most of the time goes in figuring out do I need to add new tests or can update existing and where shall I add the new tests in a test file
There was a problem hiding this comment.
I agree! Tried retaking a look, and in my opinion as well, having class names for sensors makes the tests lot more readable. @dimberman I would request you to re-evaluate your request to remove classes. Thanks
There was a problem hiding this comment.
Ahhh ok that makes sense. I'm always a fan of minimizing nesting as much as possible but I think this is reasonable considering there are multiple classes in the file
This change removes dependencies on unittest.TestCase from google/cloud/sensors package. The use of unittest in this package is now limited to only unittest.mock . Related: apache#29305
44c0a93 to
39322ff
Compare
This change removes dependencies on unittest.TestCase from google/cloud/sensors package. The use of unittest in this package is now limited to only unittest.mock which is fine acc to the original issue.
Tested using
breeze testing tests --test-type "Providers[google]"related #29305
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in newsfragments.