Repository navigation
Importing correct MySqlContext in test_generic_transfer - #46256
amoghrajesh wants to merge 2 commits into
Conversation
| ) | ||
| def test_mysql_to_mysql(self, client): | ||
| from providers.tests.mysql.hooks.test_mysql import MySqlContext | ||
| from providers.mysql.tests.provider_tests.mysql.hooks.test_mysql import MySqlContext |
There was a problem hiding this comment.
I think proper fix will be copy&paste the MySQL context here. We are not supposed to import test code between providers - their test code should be rather independent.
We could potentially move the context to tests_common - because this is where providers import common code from, but I think in this case. just copying the context manager here will be a better solution - both MySQL tests and Generic tests here are testing MySQL Hooks in similar way, but keeping DRY in tests is not really a goal, tests are generally better if they are "standalone" - even if some code is copied.
There was a problem hiding this comment.
I think proper fix will be copy&paste the MySQL context here. We are not supposed to import test code between providers - their test code should be rather independent.
Yeah makes sense. I was too sleepy to think straight :D!
There was a problem hiding this comment.
class MySqlContext:
def __init__(self, client):
self.client = client
self.connection = MySqlHook.get_connection(MySqlHook.default_conn_name)
self.init_client = self.connection.extra_dejson.get("client", "mysqlclient")
def __enter__(self):
self.connection.set_extra(f'{{"client": "{self.client}"}}')
def __exit__(self, exc_type, exc_val, exc_tb):
self.connection.set_extra(f'{{"client": "{self.init_client}"}}')
This however has a MySqlHook. And even if i move that to tests_common, it will still need importing it correct?
Even prior to my change, we imported the MysqlHook, just from different path @potiuk
There was a problem hiding this comment.
MysqlHook is fine. All the providers are properly acceessible cross providers. It's the "test" code that is not accessible. see #46274
There was a problem hiding this comment.
tests_common should be a better thing to do however, moved it and changed the refs
There was a problem hiding this comment.
I am not sure. Tests_common - should contain only "provider-agnostic code" i.e. something that does not belong to any provider. By Adding MySQL there, we make common code (which should be used by the provider tests - i.e. be dependnecy of the provider tests) into something that is USING mysql provider. So if mysql provider uses "tests_common" and "test_commmon" use mysql provider, this is "almost a cycle" it only works because provider does not use tests code - it's used by the test code..
I think I prefere copying the contextmanager in both places.
There was a problem hiding this comment.
Yeah thanks for the clarification. That was my initial thought too.
Why to create an unwanted dependency. Anyways, ill approve your PR and you can close this one
|
Closing in favour of #46274 |
Saw this failure in CI:
^ 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.