✨ Source Facebook Marketing: remove testing deleted records from integration test, move it to mock server tests #37095
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
...integrations/connectors/source-facebook-marketing/unit_tests/integration/response_builder.py
Outdated
Show resolved
Hide resolved
...grations/connectors/source-facebook-marketing/unit_tests/integration/test_include_deleted.py
Show resolved
Hide resolved
...grations/connectors/source-facebook-marketing/unit_tests/resource/http/response/ad_sets.json
Show resolved
Hide resolved
| ) | ||
| filters = ( | ||
| '[{"field":"ad.effective_status","operator":"IN","value":["ACTIVE","ARCHIVED"]},' | ||
| '{"field":"ad.updated_time","operator":"GREATER_THAN","value":1672531200}]' |
There was a problem hiding this comment.
Where does this value comes from? It seems like it should be relevant but without a variable, it is hard to know the link between this and where it comes from
There was a problem hiding this comment.
added comment for filters
| output = self._read(config().with_ad_statuses(self.statuses), "ads") | ||
| assert len(output.records) == 1 | ||
| account_state = output.most_recent_state.dict()["stream_state"][self.account_id] | ||
| assert self.filter_statuses_flag in account_state, f"State should include `filter_statuses` flag" |
There was a problem hiding this comment.
The error message is clear but without context about the "why", it is hard to understand why this is asserted here. Can we add more info on that?
There was a problem hiding this comment.
updated error message with reason why we need filter_statuses
| get_account_response(), | ||
| ) | ||
| filters = ( | ||
| '[{"field":"ad.effective_status","operator":"IN","value":["ACTIVE","ARCHIVED"]},' |
There was a problem hiding this comment.
Should we use self. statuses here to clarify the link between the config and the API call? The same comment applies for the other tests
There was a problem hiding this comment.
updated with self.statuses
| assert len(output.records) == 1 | ||
| account_state = output.most_recent_state.dict()["stream_state"][self.account_id] | ||
| assert self.filter_statuses_flag in account_state, f"State should include `filter_statuses` flag" | ||
| assert account_state == {"filter_statuses": ["ACTIVE", "ARCHIVED"], "updated_time": "2023-03-21T22:41:46-0700"} |
There was a problem hiding this comment.
Should we use self. statuses here?
There was a problem hiding this comment.
updated with self.statuses
Test logic is the same, we check for deleted records are in response and states have filter statuses. Also parent stream are covered in unit tests already, so we don't need to add test for stream slices etc. Pagination is a part of fb lib, I don't think we should test it. |
darynaishchenko
deleted the
daryna/source-facebook-marketing/failing-int-test
branch
…gration test, move it to mock server tests (#37095)
resolved: https://github.com/airbytehq/airbyte-internal-issues/issues/6552
What:
removed integration tests that were based on live data and added new test suit for deleted records to mock server tests (unit_tests/integration)