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.

Sign up for GitHub

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

Jump to bottom

Update integration tests #120

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Update integration tests #120

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a9dffc8
Update Pin and Board models with new properties
gilderlane Oct 4, 2023
f69bf6f
Update Targeting spec for ad group
gilderlane Oct 4, 2023
6bde6a3
update integrration tests to use DEFAULT_AD_GROUP_ID
gilderlane Oct 4, 2023
4e26981
Update integration tests for conversion_events due to changes on the …
gilderlane Oct 4, 2023
c5e0eb6
Fix lint problems
gilderlane Oct 4, 2023
b908ee8
Update Pinterest Generated Client version in the requirements
gilderlane Oct 5, 2023
382e1a4
Add logging to delete_pin
gilderlane Oct 5, 2023
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
6 changes: 3 additions & 3 deletions integration_tests/ads/test_ad_groups.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
from integration_tests.base_test import BaseTestCase
from integration_tests.config import DEFAULT_AD_ACCOUNT_ID

from openapi_generated.pinterest_client.model.targeting_spec import TargetingSpec

from pinterest.ads.ad_groups import AdGroup


Expand Down Expand Up @@ -63,9 +65,7 @@ def test_update_success(self):
)

new_name = "SDK_AD_GROUP_NEW_NAME"
new_spec = {
"GENDER": ["male"]
}
new_spec = TargetingSpec(gender=["male"])
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't be using models directly like this from the generated client. we need a copy of its implementation in the SDK and then use that if needed. Was there an issue with just using what we had earlier?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, we started to get ApiTypeError: Invalid type for variable 'targeting_spec' for what we had for new_spec previously


ad_group.update_fields(
name=new_name,
Expand Down
10 changes: 5 additions & 5 deletions integration_tests/ads/test_ads.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
from openapi_generated.pinterest_client.model.entity_status import EntityStatus

from integration_tests.base_test import BaseTestCase
from integration_tests.config import DEFAULT_PIN_ID, DEFAULT_AD_ACCOUNT_ID
from integration_tests.config import DEFAULT_PIN_ID, DEFAULT_AD_ACCOUNT_ID, DEFAULT_AD_GROUP_ID


class TestCreateAd(BaseTestCase):
Expand All @@ -23,8 +23,8 @@ def test_create_ad_success(self):
"""
ad = Ad.create(
ad_account_id=DEFAULT_AD_ACCOUNT_ID,
ad_group_id=self.ad_group_utils.get_ad_group_id(),
creative_type="REGULAR",
ad_group_id=DEFAULT_AD_GROUP_ID,
creative_type="IDEA",
Copy link
Member

Choose a reason for hiding this comment

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

REGULAR format pins are more common and will always stay. I think we should stick to creating an ad using that creative type instead of IDEA as it is a new creative type and we may see changes there.

pin_id=DEFAULT_PIN_ID,
name="Test_create_ad",
status="ACTIVE",
Expand All @@ -43,7 +43,7 @@ def test_create_ad_failure_without_creative_type(self):
"""
ad_arguments = dict(
ad_account_id=DEFAULT_AD_ACCOUNT_ID,
ad_group_id=self.ad_group_utils.get_ad_group_id(),
ad_group_id=DEFAULT_AD_GROUP_ID,
pin_id=DEFAULT_PIN_ID,
name="Test_create_ad",
status="ACTIVE",
Expand All @@ -59,7 +59,7 @@ def test_create_ad_failure_with_incorrect_creative_type(self):
"""
ad_arguments = dict(
ad_account_id=DEFAULT_AD_ACCOUNT_ID,
ad_group_id=self.ad_group_utils.get_ad_group_id(),
ad_group_id=DEFAULT_AD_GROUP_ID,
creative_type="NOT",
pin_id=DEFAULT_PIN_ID,
name="Test_create_ad",
Expand Down
23 changes: 9 additions & 14 deletions integration_tests/ads/test_conversion_events.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
from pinterest.client import PinterestSDKClient
from pinterest.ads.conversion_events import Conversion

from openapi_generated.pinterest_client.exceptions import ApiException
Copy link
Member

Choose a reason for hiding this comment

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

Why are we importing this? I do not see any uses of it in the file. If we need one, there is an SDKException class defined, please use that instead.

Copy link
Author

Choose a reason for hiding this comment

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

The ApiException is the one raised when wrong params format is passed, which is the case tested in test_send_conversion_fail(). This is the reason why I test if its raised the failure test and therefore import it.


class TestSendConversionEvent(BaseTestCase):
"""
Test send Conversion Event
Expand Down Expand Up @@ -79,17 +81,10 @@ def test_send_conversion_fail(self):
for _ in range(NUMBER_OF_CONVERSION_EVENTS)
]

response = Conversion.send_conversion_events(
client = client,
ad_account_id = DEFAULT_AD_ACCOUNT_ID,
conversion_events = conversion_events,
test = True,
)

assert response
assert response.num_events_received == 2
assert response.num_events_processed == 0
assert len(response.events) == 2

assert 'hashed format' in response.events[0].error_message
assert 'hashed format' in response.events[0].error_message
Comment on lines -89 to -95
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we keep asserts?

with self.assertRaises(ApiException):
Conversion.send_conversion_events(
client = client,
ad_account_id = DEFAULT_AD_ACCOUNT_ID,
conversion_events = conversion_events,
test = True,
)
3 changes: 3 additions & 0 deletions integration_tests/config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,6 @@
DEFAULT_BOARD_SECTION_ID = os.environ.get('DEFAULT_BOARD_SECTION_ID', "")
OWNER_USER_ID = os.environ.get('OWNER_USER_ID', "")
DEFAULT_AD_ACCOUNT_ID = os.environ.get('DEFAULT_AD_ACCOUNT_ID', "")
DEFAULT_AD_ID = os.environ.get('DEFAULT_AD_ID', "")
DEFAULT_AD_GROUP_ID = os.environ.get('DEFAULT_AD_GROUP_ID', "")
DEFAULT_CAMPAIGN_ID = os.environ.get('DEFAULT_CAMPAIGN_ID', "")
Empty file added integration_tests/organic/___init__.py
View file
Open in desktop
Empty file.
4 changes: 2 additions & 2 deletions integration_tests/utils/ads_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,7 @@ def __init__(self, client=None):
self.ad = Ad.create(
ad_account_id=DEFAULT_AD_ACCOUNT_ID,
ad_group_id=getattr(self.ad_group, "_id"),
creative_type="REGULAR",
creative_type="IDEA",
Copy link
Member

Choose a reason for hiding this comment

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

Please change this to REGULAR for reasons explained above. Unless there is an issue which you can explain here.

Copy link
Author

@gilderlane gilderlane Oct 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The default test pin associated to the ad created in the integration tests is an IDEA pin. It was created via the UI, and has only one image, but only IDEA ads are elegible for it

pin_id=DEFAULT_PIN_ID,
name="Test_create_ad",
status="ACTIVE",
Expand All @@ -232,7 +232,7 @@ def get_default_params(self):
return dict(
ad_account_id=DEFAULT_AD_ACCOUNT_ID,
ad_group_id=getattr(self.ad_group, "_id"),
creative_type="REGULAR",
creative_type="IDEA",
Copy link
Member

Choose a reason for hiding this comment

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

Please change this to REGULAR for reasons explained above.

pin_id=DEFAULT_PIN_ID,
name="Test_create_ad",
status="ACTIVE",
Expand Down
7 changes: 6 additions & 1 deletion integration_tests/utils/organic_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
Provide helper and utility functions for Organic Endpoints Integration Testing
"""
import random
import logging

from pinterest.client import PinterestSDKClient

Expand All @@ -10,6 +11,7 @@

from integration_tests.config import DEFAULT_BOARD_ID, DEFAULT_PIN_ID

log = logging.getLogger(__name__)

def _merge_default_params_with_params(default_params, params):
if not params:
Expand Down Expand Up @@ -79,4 +81,7 @@ def create_new_pin(self, **kwargs):
return Pin.create(**_merge_default_params_with_params(self.get_default_params(), kwargs))

def delete_pin(self, pin_id):
return Pin.delete(pin_id=pin_id, client=self.test_client)
if pin_id != DEFAULT_PIN_ID: # Make sure default pin is not being deleted
return Pin.delete(pin_id=pin_id, client=self.test_client)
Comment on lines +84 to +85
Copy link
Member

Choose a reason for hiding this comment

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

Can you add some sort of logging here? Or a print statement or something? Something that indicates

  1. Default pin was tried to be deleted
  2. Or, it didn't actually delete the pin that was trying to be deleted

Copy link
Author

Choose a reason for hiding this comment

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

Added

else:
log.warning(f"Default pin {DEFAULT_PIN_ID} was attempted to be deleted.")
37 changes: 37 additions & 0 deletions pinterest/organic/boards.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,14 @@ def __init__(
client (PinterestSDKClient, optional): PinterestSDKClient Object. Uses the default client, if not provided.
"""
self._id = None
self._created_at = None
self._board_pins_modified_at = None
self._name = None
self._collaborator_count = None
self._pin_count = None
self._follower_count = None
self._media = None
self._owner = None
self._description = None
self._owner = None
self._privacy = None
Expand All @@ -227,11 +234,41 @@ def id(self) -> str:
# pylint: disable=missing-function-docstring
return self._id

@property
def created_at(self) -> str:
# pylint: disable=missing-function-docstring
return self._created_at

@property
def board_pins_modified_at(self) -> str:
# pylint: disable=missing-function-docstring
return self._board_pins_modified_at

@property
def name(self) -> str:
# pylint: disable=missing-function-docstring
return self._name

@property
def collaborator_count(self) -> int:
# pylint: disable=missing-function-docstring
return self._collaborator_count

@property
def pin_count(self) -> int:
# pylint: disable=missing-function-docstring
return self._pin_count

@property
def follower_count(self) -> int:
# pylint: disable=missing-function-docstring
return self._follower_count

@property
def media(self):
# pylint: disable=missing-function-docstring
return self._media

@property
def description(self) -> str:
# pylint: disable=missing-function-docstring
Expand Down
30 changes: 30 additions & 0 deletions pinterest/organic/pins.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,11 @@ def __init__(
self._ad_account_id = None

self._id = None
self._is_owner = None
self._is_standard = None
self._note = None
self._has_been_promoted = None
self._creative_type = None
self._created_at = None
self._link = None
self._title = None
Expand Down Expand Up @@ -75,6 +80,31 @@ def id(self) -> str:
# pylint: disable=missing-function-docstring
return self._id

@property
def is_owner(self) -> str:
# pylint: disable=missing-function-docstring
return self._is_owner

@property
def is_standard(self) -> str:
# pylint: disable=missing-function-docstring
return self._is_standard

@property
def note(self) -> str:
# pylint: disable=missing-function-docstring
return self._note

@property
def has_been_promoted(self) -> str:
# pylint: disable=missing-function-docstring
return self._has_been_promoted

@property
def creative_type(self) -> str:
# pylint: disable=missing-function-docstring
return self._creative_type

@property
def created_at(self) -> str:
# pylint: disable=missing-function-docstring
Expand Down
2 changes: 1 addition & 1 deletion requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
Pinterest-Generated-Client==0.1.7
Pinterest-Generated-Client==0.1.8
python-dateutil==2.8.2
six==1.16.0
urllib3==1.26.12
Expand Down
4 changes: 2 additions & 2 deletions setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,10 @@ def _get_prod_version():
"python-dateutil",
"python-dotenv==0.20.0",
"six==1.16.0",
"Pinterest-Generated-Client==0.1.7"
"Pinterest-Generated-Client==0.1.8"
]

long_description = (Path(__file__).parent / "README.md").read_text()
long_description = (Path(__file__).parent / "README.md").read_text(encoding='UTF-8')
package_root = os.path.abspath(os.path.dirname(__file__))

__version__ = None
Expand Down
5 changes: 2 additions & 3 deletions tests/src/pinterest/ads/test_ad_groups.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
from openapi_generated.pinterest_client.model.ad_group_response import AdGroupResponse
from openapi_generated.pinterest_client.model.ad_group_array_response import AdGroupArrayResponse
from openapi_generated.pinterest_client.model.ad_group_array_response_element import AdGroupArrayResponseElement
from openapi_generated.pinterest_client.model.targeting_spec import TargetingSpec
Copy link
Member

Choose a reason for hiding this comment

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

Change this for reasons explained above.


from pinterest.ads.ad_groups import AdGroup
from pinterest.ads.ads import Ad
Expand Down Expand Up @@ -101,9 +102,7 @@ def test_update_ad_group(self, get_mock, update_mock):
"""
update_mock.__name__ = "ad_groups_update"
new_name = "SDK_AD_GROUP_NEW_NAME"
new_spec = {
"GENDER": ["male"]
}
new_spec = TargetingSpec(gender=["male"])

get_mock.return_value = AdGroupResponse(
id=self.test_ad_group_id,
Expand Down