Update integration tests #120
Conversation
| new_spec = { | ||
| "GENDER": ["male"] | ||
| } | ||
| new_spec = TargetingSpec(gender=["male"]) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Yes, we started to get ApiTypeError: Invalid type for variable 'targeting_spec' for what we had for new_spec previously
| ad_group_id=self.ad_group_utils.get_ad_group_id(), | ||
| creative_type="REGULAR", | ||
| ad_group_id=DEFAULT_AD_GROUP_ID, | ||
| creative_type="IDEA", |
There was a problem hiding this comment.
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.
| @@ -8,6 +8,8 @@ | |||
| from pinterest.client import PinterestSDKClient | |||
| from pinterest.ads.conversion_events import Conversion | |||
|
|
|||
| from openapi_generated.pinterest_client.exceptions import ApiException | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @@ -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", | |||
There was a problem hiding this comment.
Please change this to REGULAR for reasons explained above. Unless there is an issue which you can explain here.
There was a problem hiding this comment.
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
| @@ -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", | |||
There was a problem hiding this comment.
Please change this to REGULAR for reasons explained above.
| 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) |
There was a problem hiding this comment.
Can you add some sort of logging here? Or a print statement or something? Something that indicates
- Default pin was tried to be deleted
- Or, it didn't actually delete the pin that was trying to be deleted
| @@ -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 | |||
There was a problem hiding this comment.
Change this for reasons explained above.
| 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 |
Update required Pinterest-Generated-Client version
Update Pin and Board models with new properties
Update integration tests