-
Notifications
You must be signed in to change notification settings - Fork 244
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cleanup some warnings #3491
Cleanup some warnings #3491
Changes from 6 commits
ea016ac
49c5d26
ad53e80
b486754
70edc91
2bdb8cf
198027b
00d2bb0
e7915db
be19f05
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Test*.yaml |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,8 @@ | |
from collections import defaultdict, namedtuple | ||
from contextlib import contextmanager | ||
from datetime import date, datetime | ||
from distutils.version import StrictVersion | ||
from typing import Optional | ||
from urllib.parse import urlparse | ||
|
||
import requests | ||
|
@@ -47,14 +49,12 @@ class OrgConfig(BaseConfig): | |
is_sandbox: bool | ||
namespace: str | ||
namespaced: bool | ||
org_id: str | ||
org_type: str | ||
password: str | ||
scratch: bool | ||
scratch_org_type: str | ||
set_password: bool | ||
sfdx_alias: str | ||
username: str | ||
userinfo: str | ||
id: str | ||
active: bool | ||
|
@@ -63,8 +63,9 @@ class OrgConfig(BaseConfig): | |
refresh_token: str | ||
client_secret: str | ||
connected_app: str | ||
serialization_format: str | ||
|
||
createable: bool = None | ||
createable: Optional[bool] = None | ||
|
||
# make sure it can be mocked for tests | ||
OAuth2Client = OAuth2Client | ||
|
@@ -204,7 +205,15 @@ def user_id(self): | |
|
||
@property | ||
def org_id(self): | ||
return self.id.split("/")[-2] | ||
try: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This code probably relied previously on implicit None for config objects which is evil. |
||
if org_id := self.config.get("org_id"): | ||
return org_id | ||
elif hasattr(self, "id") and self.id: | ||
return self.id.split("/")[-2] | ||
else: | ||
return None | ||
except Exception as e: # pragma: no cover | ||
assert e is None, e | ||
|
||
@property | ||
def username(self): | ||
|
@@ -254,7 +263,7 @@ def populate_expiration_date(self): | |
@property | ||
def organization_sobject(self): | ||
"""Cached copy of Organization sObject. Does not perform API call.""" | ||
return self._org_sobject | ||
return getattr(self, "_org_sobject", None) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This code probably relied previously on implicit None for config objects which is evil. |
||
|
||
def _fetch_community_info(self): | ||
"""Use the API to re-fetch information about communities""" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -66,7 +66,7 @@ def test_getattr_toplevel_key_missing(self): | |
assert config.foo is None | ||
with mock.patch( | ||
"cumulusci.core.config.base_config.STRICT_GETATTR", True | ||
), pytest.raises(AssertionError): | ||
), pytest.deprecated_call(), pytest.raises(AssertionError): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Capture a deprecated call. |
||
assert config.foo is None | ||
|
||
def test_getattr_child_key(self): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -645,6 +645,7 @@ def test_install(self, api_deploy_mock, zip_builder_mock, download_mock): | |
assert mock_task.project_config == context | ||
|
||
api_deploy_mock.return_value.assert_called_once() | ||
zf.close() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Close a file explicitly. |
||
|
||
def test_get_unmanaged(self): | ||
org = mock.Mock() | ||
|
@@ -733,6 +734,7 @@ def test_install(self, api_deploy_mock, zip_builder_mock, download_mock): | |
assert mock_task.project_config == context | ||
|
||
api_deploy_mock.return_value.assert_called_once() | ||
zf.close() | ||
|
||
def test_get_unmanaged(self): | ||
org = mock.Mock() | ||
|
@@ -793,6 +795,7 @@ def test_get_metadata_package_zip_builder__mdapi_root( | |
}, | ||
context=mock.ANY, | ||
) | ||
zf.close() | ||
|
||
@mock.patch("cumulusci.core.dependencies.dependencies.MetadataPackageZipBuilder") | ||
@mock.patch("cumulusci.core.dependencies.dependencies.download_extract_zip") | ||
|
@@ -827,6 +830,7 @@ def test_get_metadata_package_zip_builder__mdapi_subfolder( | |
}, | ||
context=mock.ANY, | ||
) | ||
zf.close() | ||
|
||
@mock.patch("cumulusci.core.dependencies.dependencies.MetadataPackageZipBuilder") | ||
@mock.patch("cumulusci.core.dependencies.dependencies.download_extract_zip") | ||
|
@@ -866,6 +870,7 @@ def test_get_metadata_package_zip_builder__sfdx( | |
capture_output=True, | ||
check_return=True, | ||
) | ||
zf.close() | ||
|
||
|
||
class TestParseDependency: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
version: 1 | ||
interactions: | ||
- include_file: GET_sobjects_Organization.yaml |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,7 +13,10 @@ | |
import responses | ||
from requests.models import Response | ||
|
||
from cumulusci.core.exceptions import SalesforceCredentialsException | ||
from cumulusci.core.exceptions import ( | ||
CumulusCIUsageError, | ||
SalesforceCredentialsException, | ||
) | ||
from cumulusci.core.keychain.base_project_keychain import DEFAULT_CONNECTED_APP_PORT | ||
from cumulusci.oauth.client import ( | ||
PORT_IN_USE_ERR, | ||
|
@@ -72,9 +75,17 @@ def http_client(client_config): | |
|
||
@contextmanager | ||
@mock.patch("time.sleep", time.sleep) # undo mock from conftest | ||
def httpd_thread(oauth_client): | ||
def httpd_thread(oauth_client, expected_error=None): | ||
# call OAuth object on another thread - this spawns local httpd | ||
thread = threading.Thread(target=oauth_client.auth_code_flow) | ||
|
||
def run_code_and_check_exception(): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Catch exceptions explicitly in this function instead of implicitly. |
||
if expected_error: | ||
with pytest.raises(expected_error): | ||
oauth_client.auth_code_flow() | ||
else: | ||
oauth_client.auth_code_flow() | ||
|
||
thread = threading.Thread(target=run_code_and_check_exception) | ||
thread.start() | ||
while thread.is_alive(): | ||
if oauth_client.httpd: | ||
|
@@ -191,7 +202,7 @@ def test_oauth_flow_error_from_auth(self, client): | |
) | ||
|
||
# call OAuth object on another thread - this spawns local httpd | ||
with httpd_thread(client): | ||
with httpd_thread(client, OAuth2Error): | ||
# simulate callback from browser | ||
with pytest.raises(urllib.error.HTTPError): | ||
urllib.request.urlopen( | ||
|
@@ -203,7 +214,7 @@ def test_oauth_flow_error_from_auth(self, client): | |
sys.platform.startswith("win"), reason="setup differs from windows" | ||
) | ||
def test_create_httpd__port_already_in_use(self, client): | ||
with httpd_thread(client): | ||
with httpd_thread(client, CumulusCIUsageError): | ||
with pytest.raises( | ||
OAuth2Error, match=PORT_IN_USE_ERR.format(DEFAULT_CONNECTED_APP_PORT) | ||
): | ||
|
@@ -226,7 +237,7 @@ def test_oauth_flow_error_from_token(self, client): | |
) | ||
|
||
# call OAuth object on another thread - this spawns local httpd | ||
with httpd_thread(client): | ||
with httpd_thread(client, OAuth2Error): | ||
# simulate callback from browser | ||
with pytest.raises(urllib.error.HTTPError): | ||
urllib.request.urlopen(client.client_config.redirect_uri + "?code=123") | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,7 @@ | |
|
||
import pytest | ||
import responses | ||
from responses.matchers import query_string_matcher | ||
from simple_salesforce import SalesforceGeneralError | ||
|
||
from cumulusci.core import exceptions as exc | ||
|
@@ -73,9 +74,9 @@ def setup_method(self): | |
|
||
def _mock_apex_class_query(self, name="TestClass_TEST", namespace=None): | ||
namespace_param = "null" if namespace is None else f"%27{namespace}%27" | ||
url = ( | ||
self.base_tooling_url | ||
+ "query/?q=SELECT+Id%2C+Name+" | ||
url = self.base_tooling_url + "query/" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
query_string = ( | ||
"q=SELECT+Id%2C+Name+" | ||
+ f"FROM+ApexClass+WHERE+NamespacePrefix+%3D+{namespace_param}" | ||
+ "+AND+%28Name+LIKE+%27%25_TEST%27%29" | ||
) | ||
|
@@ -85,7 +86,10 @@ def _mock_apex_class_query(self, name="TestClass_TEST", namespace=None): | |
"totalSize": 1, | ||
} | ||
responses.add( | ||
responses.GET, url, match_querystring=True, json=expected_response | ||
responses.GET, | ||
url, | ||
match=[query_string_matcher(query_string)], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
json=expected_response, | ||
) | ||
|
||
def _get_mock_test_query_results(self, methodnames, outcomes, messages): | ||
|
@@ -163,59 +167,63 @@ def _get_mock_test_query_results(self, methodnames, outcomes, messages): | |
|
||
def _get_mock_test_query_url(self, job_id): | ||
return ( | ||
self.base_tooling_url | ||
+ "query/?q=%0ASELECT+Id%2CApexClassId%2CTestTimestamp%2C%0A+++++++Message%2CMethodName%2COutcome%2C%0A+++++++RunTime%2CStackTrace%2C%0A+++++++%28SELECT%0A++++++++++Id%2CCallouts%2CAsyncCalls%2CDmlRows%2CEmail%2C%0A++++++++++LimitContext%2CLimitExceptions%2CMobilePush%2C%0A++++++++++QueryRows%2CSosl%2CCpu%2CDml%2CSoql%0A++++++++FROM+ApexTestResults%29%0AFROM+ApexTestResult%0AWHERE+AsyncApexJobId%3D%27{}%27%0A".format( | ||
job_id | ||
) | ||
self.base_tooling_url + "query/", | ||
f"q=%0ASELECT+Id%2CApexClassId%2CTestTimestamp%2C%0A+++++++Message%2CMethodName%2COutcome%2C%0A+++++++RunTime%2CStackTrace%2C%0A+++++++%28SELECT%0A++++++++++Id%2CCallouts%2CAsyncCalls%2CDmlRows%2CEmail%2C%0A++++++++++LimitContext%2CLimitExceptions%2CMobilePush%2C%0A++++++++++QueryRows%2CSosl%2CCpu%2CDml%2CSoql%0A++++++++FROM+ApexTestResults%29%0AFROM+ApexTestResult%0AWHERE+AsyncApexJobId%3D%27{job_id}%27%0A", | ||
) | ||
|
||
def _get_mock_testqueueitem_status_query_url(self, job_id): | ||
return ( | ||
self.base_tooling_url | ||
+ f"query/?q=SELECT+Id%2C+Status%2C+ExtendedStatus%2C+ApexClassId+FROM+ApexTestQueueItem+WHERE+ParentJobId+%3D+%27{job_id}%27+AND+Status+%3D+%27Failed%27" | ||
(self.base_tooling_url + "query/"), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Switch to |
||
f"q=SELECT+Id%2C+Status%2C+ExtendedStatus%2C+ApexClassId+FROM+ApexTestQueueItem+WHERE+ParentJobId+%3D+%27{job_id}%27+AND+Status+%3D+%27Failed%27", | ||
) | ||
|
||
def _mock_get_test_results( | ||
self, outcome="Pass", message="Test Passed", job_id="JOB_ID1234567" | ||
): | ||
url = self._get_mock_test_query_url(job_id) | ||
url, query_string = self._get_mock_test_query_url(job_id) | ||
|
||
expected_response = self._get_mock_test_query_results( | ||
["TestMethod"], [outcome], [message] | ||
) | ||
responses.add( | ||
responses.GET, url, match_querystring=True, json=expected_response | ||
responses.GET, | ||
url, | ||
match=[query_string_matcher(query_string)], | ||
json=expected_response, | ||
) | ||
|
||
def _mock_get_test_results_multiple( | ||
self, method_names, outcomes, messages, job_id="JOB_ID1234567" | ||
): | ||
url = self._get_mock_test_query_url(job_id) | ||
url, query_string = self._get_mock_test_query_url(job_id) | ||
|
||
expected_response = self._get_mock_test_query_results( | ||
method_names, outcomes, messages | ||
) | ||
responses.add( | ||
responses.GET, url, match_querystring=True, json=expected_response | ||
responses.GET, | ||
url, | ||
match=[query_string_matcher(query_string)], | ||
json=expected_response, | ||
) | ||
|
||
def _mock_get_failed_test_classes(self, job_id="JOB_ID1234567"): | ||
url = self._get_mock_testqueueitem_status_query_url(job_id) | ||
url, query_string = self._get_mock_testqueueitem_status_query_url(job_id) | ||
|
||
responses.add( | ||
responses.GET, | ||
url, | ||
match_querystring=True, | ||
match=[query_string_matcher(query_string)], | ||
json={"totalSize": 0, "records": [], "done": True}, | ||
) | ||
|
||
def _mock_get_failed_test_classes_failure(self, job_id="JOB_ID1234567"): | ||
url = self._get_mock_testqueueitem_status_query_url(job_id) | ||
url, query_string = self._get_mock_testqueueitem_status_query_url(job_id) | ||
|
||
responses.add( | ||
responses.GET, | ||
url, | ||
match_querystring=True, | ||
match=[query_string_matcher(query_string)], | ||
json={ | ||
"totalSize": 1, | ||
"records": [ | ||
|
@@ -231,14 +239,15 @@ def _mock_get_failed_test_classes_failure(self, job_id="JOB_ID1234567"): | |
) | ||
|
||
def _mock_get_symboltable(self): | ||
url = ( | ||
self.base_tooling_url | ||
+ "query/?q=SELECT+SymbolTable+FROM+ApexClass+WHERE+Name%3D%27TestClass_TEST%27" | ||
url = self.base_tooling_url + "query/" | ||
query_string = ( | ||
"q=SELECT+SymbolTable+FROM+ApexClass+WHERE+Name%3D%27TestClass_TEST%27" | ||
) | ||
|
||
responses.add( | ||
responses.GET, | ||
url, | ||
match=[query_string_matcher(query_string)], | ||
json={ | ||
"records": [ | ||
{ | ||
|
@@ -261,9 +270,9 @@ def _mock_get_symboltable_failure(self): | |
responses.add(responses.GET, url, json={"records": []}) | ||
|
||
def _mock_tests_complete(self, job_id="JOB_ID1234567"): | ||
url = ( | ||
self.base_tooling_url | ||
+ "query/?q=SELECT+Id%2C+Status%2C+" | ||
url = self.base_tooling_url + "query/" | ||
query_string = ( | ||
"q=SELECT+Id%2C+Status%2C+" | ||
+ "ApexClassId+FROM+ApexTestQueueItem+WHERE+ParentJobId+%3D+%27" | ||
+ "{}%27".format(job_id) | ||
) | ||
|
@@ -273,23 +282,29 @@ def _mock_tests_complete(self, job_id="JOB_ID1234567"): | |
"records": [{"Status": "Completed"}], | ||
} | ||
responses.add( | ||
responses.GET, url, match_querystring=True, json=expected_response | ||
responses.GET, | ||
url, | ||
match=[query_string_matcher(query_string)], | ||
json=expected_response, | ||
) | ||
|
||
def _mock_tests_processing(self, job_id="JOB_ID1234567"): | ||
url = ( | ||
self.base_tooling_url | ||
+ "query/?q=SELECT+Id%2C+Status%2C+" | ||
url = self.base_tooling_url + "query/" | ||
query_string = ( | ||
"q=SELECT+Id%2C+Status%2C+" | ||
+ "ApexClassId+FROM+ApexTestQueueItem+WHERE+ParentJobId+%3D+%27" | ||
+ "{}%27".format(job_id) | ||
+ f"{job_id}%27" | ||
) | ||
expected_response = { | ||
"done": True, | ||
"totalSize": 1, | ||
"records": [{"Status": "Processing", "ApexClassId": 1}], | ||
} | ||
responses.add( | ||
responses.GET, url, match_querystring=True, json=expected_response | ||
responses.GET, | ||
url, | ||
match=[query_string_matcher(query_string)], | ||
json=expected_response, | ||
) | ||
|
||
def _mock_run_tests(self, success=True, body="JOB_ID1234567"): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -783,12 +783,11 @@ def test_explicit_channel_declarations(self, mock_load_data, create_task): | |
"recipe": Path(__file__).parent | ||
/ "snowfakery/simple_snowfakery.recipe.yml", | ||
"run_until_recipe_repeated": 15, | ||
"recipe_options": {"xyzzy": "Nothing happens", "some_number": 42}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was cut and paste from elsewhere and didn't need to be tested over and over. |
||
"loading_rules": Path(__file__).parent | ||
/ "snowfakery/simple_snowfakery_channels.load.yml", | ||
}, | ||
) | ||
with mock.patch.object( | ||
with pytest.warns(UserWarning), mock.patch.object( | ||
task.project_config, "keychain", DummyKeychain() | ||
) as keychain: | ||
|
||
|
@@ -833,7 +832,6 @@ def test_serial_mode(self, mock_load_data, create_task): | |
"recipe": Path(__file__).parent | ||
/ "snowfakery/simple_snowfakery.recipe.yml", | ||
"run_until_recipe_repeated": 15, | ||
"recipe_options": {"xyzzy": "Nothing happens", "some_number": 42}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was cut and paste from elsewhere and didn't need to be tested over and over. |
||
"bulk_mode": "Serial", | ||
}, | ||
) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't bother prettying Cassettes