dstandish commented on a change in pull request #19530:
URL: https://github.com/apache/airflow/pull/19530#discussion_r747216728
##########
File path: tests/providers/salesforce/hooks/test_salesforce.py
##########
@@ -86,23 +106,56 @@ def test_get_conn_password_auth(self, mock_salesforce):
mock_salesforce.assert_called_once_with(
username=password_auth_conn.login,
password=password_auth_conn.password,
- security_token=extras["extra__salesforce__security_token"],
- domain=extras["extra__salesforce__domain"],
+ security_token=extras.get('extra__salesforce__security_token') or
extras.get('security_token'),
Review comment:
this looks like it might need to be updated after the last update
removing the short param names
##########
File path: airflow/providers/salesforce/hooks/salesforce.py
##########
@@ -27,6 +27,11 @@
import time
from typing import Any, Dict, Iterable, List, Optional
+try:
+ from functools import cached_property
+except ImportError:
+ from cached_property import cached_property
+
Review comment:
```suggestion
from airflow.compat.functools import cached_property
```
##########
File path: tests/providers/salesforce/hooks/test_salesforce.py
##########
@@ -48,8 +49,41 @@ def test_get_conn_exists(self):
assert self.salesforce_hook.conn.return_value is not None
+ @parameterized.expand(
+ [
+ (
+ "all extras",
+ '''
+ {
+ "extra__salesforce__client_id": "my_client",
+ "extra__salesforce__consumer_key": "",
+ "extra__salesforce__domain": "test",
+ "extra__salesforce__instance": "",
+ "extra__salesforce__instance_url": "",
+ "extra__salesforce__organization_id": "",
+ "extra__salesforce__private_key": "",
+ "extra__salesforce__private_key_file_path": "",
+ "extra__salesforce__proxies": "",
+ "extra__salesforce__security_token": "token",
+ "extra__salesforce__version": "42.0"
+ }
+ ''',
+ ),
+ (
+ "required extras",
+ '''
+ {
+ "extra__salesforce__client_id": "my_client",
+ "extra__salesforce__domain": "test",
+ "extra__salesforce__security_token": "token",
+ "extra__salesforce__version": "42.0"
+ }
+ ''',
+ ),
+ ]
Review comment:
I see that you have added parameterization in 4 places, removing the
"empty string" extras.
I think we can simplify this and be more direct about what we're testing.
What do you think about the following.
Add one test that verifies that when extra params are supplied as empty
string, the call converts those params to `None`
Then, for all of the tests you have parameterized, instead of
parameterizing, just chop down the params to only the non-empty-string params.
We'll know they will be converted to `None` since you will add the test
suggested above, so they no longer need to be included in those 4 tests. This
leaves just your "required params" sets.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
