Skip to content
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

Merged
merged 10 commits into from
Nov 15, 2024
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .prettierignore
Copy link
Contributor Author

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

Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Test*.yaml
19 changes: 14 additions & 5 deletions cumulusci/core/config/org_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -204,7 +205,15 @@ def user_id(self):

@property
def org_id(self):
return self.id.split("/")[-2]
try:
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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):
Expand Down Expand Up @@ -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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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"""
Expand Down
2 changes: 1 addition & 1 deletion cumulusci/core/config/tests/test_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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):
Expand Down
5 changes: 5 additions & 0 deletions cumulusci/core/dependencies/tests/test_dependencies.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Close a file explicitly.


def test_get_unmanaged(self):
org = mock.Mock()
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -866,6 +870,7 @@ def test_get_metadata_package_zip_builder__sfdx(
capture_output=True,
check_return=True,
)
zf.close()


class TestParseDependency:
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
version: 1
interactions:
- include_file: GET_sobjects_Organization.yaml
23 changes: 17 additions & 6 deletions cumulusci/oauth/tests/test_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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():
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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:
Expand Down Expand Up @@ -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(
Expand All @@ -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)
):
Expand All @@ -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")
Expand Down
75 changes: 45 additions & 30 deletions cumulusci/tasks/apex/tests/test_apex_tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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/"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Deprecate match_querystring argument in Response and CallbackResponse.
    Use responses.matchers.query_param_matcher or responses.matchers.query_string_matcher

query_string = (
"q=SELECT+Id%2C+Name+"
+ f"FROM+ApexClass+WHERE+NamespacePrefix+%3D+{namespace_param}"
+ "+AND+%28Name+LIKE+%27%25_TEST%27%29"
)
Expand All @@ -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)],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Deprecate match_querystring argument in Response and CallbackResponse.
    Use responses.matchers.query_param_matcher or responses.matchers.query_string_matcher

json=expected_response,
)

def _get_mock_test_query_results(self, methodnames, outcomes, messages):
Expand Down Expand Up @@ -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/"),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switch to query_string_matcher

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": [
Expand All @@ -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": [
{
Expand All @@ -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)
)
Expand All @@ -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"):
Expand Down
4 changes: 1 addition & 3 deletions cumulusci/tasks/bulkdata/tests/test_snowfakery.py
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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:

Expand Down Expand Up @@ -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},
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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",
},
)
Expand Down
3 changes: 2 additions & 1 deletion cumulusci/tasks/github/tests/test_release.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

import pytest
import responses
from responses.matchers import json_params_matcher

from cumulusci.core.config import ServiceConfig, TaskConfig
from cumulusci.core.exceptions import GithubException, TaskOptionsError
Expand Down Expand Up @@ -352,7 +353,7 @@ def test_run_task__with_beta_2gp(self):
url=self.repo_api_url + "/releases",
json=self._get_expected_release("release"),
match=[
responses.json_params_matcher(
json_params_matcher(
{
"tag_name": "beta/1.1",
"name": "1.1",
Expand Down
Loading