🐛 Source Facebook Marketing: SAT ignored fields in TestFullRefresh.test_sequential_reads #6463

vitaliizazmic · 2021-09-27T15:51:43Z

What

Sometimes records of AdsInsights streams don't contain cost_per_estimated_ad_recallers. It causes test failing.

How

Metric cost_per_estimated_ad_recallers can be calculated based on spend and estimated_ad_recallers. Estimate metric if not presented in records.

Pre-merge Checklist

…llers for AdsInsights streams if not presented in records

vitaliizazmic · 2021-09-27T15:52:51Z

/test connector=source-facebook-marketing

🕑 source-facebook-marketing https://github.com/airbytehq/airbyte/actions/runs/1279018052
❌ source-facebook-marketing https://github.com/airbytehq/airbyte/actions/runs/1279018052
🐛

CLAassistant · 2021-09-27T17:23:02Z

All committers have signed the CLA.

…test

…sh_test_failed # Conflicts: # airbyte-integrations/connectors/source-facebook-marketing/acceptance-test-config.yml

vitaliizazmic · 2021-10-05T06:51:28Z

/test connector=source-facebook-marketing

🕑 source-facebook-marketing https://github.com/airbytehq/airbyte/actions/runs/1306706075
❌ source-facebook-marketing https://github.com/airbytehq/airbyte/actions/runs/1306706075
🐛 https://gradle.com/s/th4ljflrcymqk

avida · 2021-10-05T10:01:36Z

airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/conftest.py

@@ -164,3 +164,9 @@ def detailed_logger() -> Logger:
    logger.log_json_list = lambda l: logger.info(json.dumps(list(l), indent=1))
    logger.handlers = [fh]
    return logger
+
+
+@pytest.fixture(name="ignored_fields")


Why do you need fixture for this? Fixture galore makes code vary hard to read, just use input .

avida

Please bump SAT version, add docs and change input format to more generic one, like here #6433

avida · 2021-10-05T10:03:52Z

airbyte-integrations/connectors/source-facebook-marketing/acceptance-test-config.yml

-      configured_catalog_path: "integration_tests/configured_catalog_without_insights.json"
+      configured_catalog_path: "integration_tests/configured_catalog.json"
+      ignored_fields:
+        "ads_insights_age_and_gender": [ "cost_per_estimated_ad_recallers", ]


This config is specific to your case, please make it more general, like described in this issue #6433

keu · 2021-10-05T15:24:15Z

airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/utils/common.py

@@ -59,6 +60,18 @@ def filter_output(records: Iterable[AirbyteMessage], type_) -> List[AirbyteMessa
    return list(filter(lambda x: x.type == type_, records))


+def remove_ignored_fields(output: List[AirbyteMessage], ignored_fields: Mapping[str, List[str]]) -> List[Mapping[str, Any]]:


this function does too much and doesn't reflect its name.
how about have additional argument in serialize function:

def serialize(value, exclude_fields=None): ...

then your code will be:

serializer = partial(serialize, exclude_fields= ignored_fields) output_diff = set(map(serializer, records_1)) - set(map(serializer, records_2))

keu

see comments

vitaliizazmic · 2021-10-18T20:49:26Z

/test connector=source-facebook-marketing

🕑 source-facebook-marketing https://github.com/airbytehq/airbyte/actions/runs/1356437640
❌ source-facebook-marketing https://github.com/airbytehq/airbyte/actions/runs/1356437640
🐛 https://gradle.com/s/yxbp7tqcmw67c

…sh_test_failed # Conflicts: # airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_full_refresh.py # airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/utils/compare.py # airbyte-integrations/bases/source-acceptance-test/unit_tests/test_utils.py # docs/connector-development/testing-connectors/source-acceptance-tests-reference.md

vitaliizazmic · 2021-11-01T14:35:42Z

/test connector=bases/source-acceptance-test

🕑 bases/source-acceptance-test https://github.com/airbytehq/airbyte/actions/runs/1408333483
✅ bases/source-acceptance-test https://github.com/airbytehq/airbyte/actions/runs/1408333483
No Python unittests run

vitaliizazmic · 2021-11-01T14:44:40Z

/publish connector=bases/source-acceptance-test

🕑 bases/source-acceptance-test https://github.com/airbytehq/airbyte/actions/runs/1408364598
✅ bases/source-acceptance-test https://github.com/airbytehq/airbyte/actions/runs/1408364598

vitaliizazmic · 2021-11-01T15:06:59Z

/publish connector=bases/source-acceptance-test

🕑 bases/source-acceptance-test https://github.com/airbytehq/airbyte/actions/runs/1408449270
✅ bases/source-acceptance-test https://github.com/airbytehq/airbyte/actions/runs/1408449270

Source Facebook Marketing #5190 - estimate cost_per_estimated_ad_reca…

1e7b591

…llers for AdsInsights streams if not presented in records

vitaliizazmic self-assigned this Sep 27, 2021

github-actions bot added the area/connectors Connector related issues label Sep 27, 2021

jrhizor temporarily deployed to more-secrets September 27, 2021 15:55 Inactive

vitaliizazmic added 4 commits October 4, 2021 21:59

Source Facebook Marketing #5190 - add ignored fields to full refresh …

e5352ac

…test

Source Facebook Marketing #5190 - annotations

0c922c8

Merge branch 'master' into vitalii/5190_facebook_marketing_full_refre…

b27aaf9

…sh_test_failed # Conflicts: # airbyte-integrations/connectors/source-facebook-marketing/acceptance-test-config.yml

Source Facebook Marketing #5190 - reformat

7462140

vitaliizazmic temporarily deployed to more-secrets October 5, 2021 06:46 Inactive

vitaliizazmic changed the title ~~🐛 Source Facebook Marketing: estimate cost_per_estimated_ad_recallers~~ 🐛 Source Facebook Marketing: SAT ignored fields in TestFullRefresh.test_sequential_reads Oct 5, 2021

jrhizor temporarily deployed to more-secrets October 5, 2021 06:54 Inactive

vitaliizazmic requested review from keu, eliziario and gaart October 5, 2021 07:00

eliziario approved these changes Oct 5, 2021

View reviewed changes

avida reviewed Oct 5, 2021

View reviewed changes

avida suggested changes Oct 5, 2021

View reviewed changes

keu reviewed Oct 5, 2021

View reviewed changes

keu suggested changes Oct 5, 2021

View reviewed changes

vitaliizazmic linked an issue Oct 11, 2021 that may be closed by this pull request

Facebook Conversion Data Missing #5190

Closed

vitaliizazmic added 2 commits October 11, 2021 21:07

SAT #5190 - delete remove_ignored_fields

8c8d9dd

Source Facebook Marketing #5190 - use dpath util for excluding fields

9b76ac3

vitaliizazmic temporarily deployed to more-secrets October 18, 2021 20:50 Inactive

jrhizor temporarily deployed to more-secrets October 18, 2021 20:51 Inactive

vitaliizazmic requested review from avida and keu October 18, 2021 21:00

avida mentioned this pull request Oct 28, 2021

Facebook pages: generate Page token #7440

Merged

Source Facebook Marketing #5190 - fixing according to PR

5b3840c

vitaliizazmic temporarily deployed to more-secrets October 28, 2021 13:57 Inactive

Source Facebook Marketing #5190 - support ignored fields by stream

9cecea7

vitaliizazmic temporarily deployed to more-secrets October 29, 2021 11:01 Inactive

Source Facebook Marketing #5190 - update docs

55875b4

vitaliizazmic requested a review from sherifnada October 29, 2021 11:45

github-actions bot added the area/documentation Improvements or additions to documentation label Oct 29, 2021

vitaliizazmic temporarily deployed to more-secrets October 29, 2021 11:46 Inactive

vitaliizazmic added 2 commits November 1, 2021 16:15

Source Facebook Marketing #5190 - merge conflicts

8fa7d60

vitaliizazmic temporarily deployed to more-secrets November 1, 2021 14:37 Inactive

jrhizor temporarily deployed to more-secrets November 1, 2021 14:38 Inactive

Source Facebook Marketing #5190 - bump SAT version

6a66d7b

vitaliizazmic temporarily deployed to more-secrets November 1, 2021 14:45 Inactive

jrhizor temporarily deployed to more-secrets November 1, 2021 14:47 Inactive

vitaliizazmic temporarily deployed to more-secrets November 1, 2021 14:53 Inactive

vitaliizazmic added 2 commits November 1, 2021 17:05

Source Facebook Marketing #5190 - fixing unit tests

49df07c

Source Facebook Marketing #5190 - bump

4fb858f

vitaliizazmic temporarily deployed to more-secrets November 1, 2021 15:08 Inactive

jrhizor temporarily deployed to more-secrets November 1, 2021 15:09 Inactive

vitaliizazmic temporarily deployed to more-secrets November 1, 2021 15:15 Inactive

vitaliizazmic merged commit 1d922ca into master Nov 1, 2021

vitaliizazmic deleted the vitalii/5190_facebook_marketing_full_refresh_test_failed branch November 1, 2021 15:45

eliziario mentioned this pull request Dec 2, 2021

SAT: add option to ignore specific fields when checking expected records #6433

Closed

karinakuz added connectors/sources-api connectors/source/facebook-marketing labels Jan 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Source Facebook Marketing: SAT ignored fields in TestFullRefresh.test_sequential_reads #6463

🐛 Source Facebook Marketing: SAT ignored fields in TestFullRefresh.test_sequential_reads #6463

vitaliizazmic commented Sep 27, 2021

vitaliizazmic commented Sep 27, 2021 •

edited by github-actions bot

Loading

CLAassistant commented Sep 27, 2021 •

edited

Loading

vitaliizazmic commented Oct 5, 2021 •

edited by github-actions bot

Loading

avida Oct 5, 2021

avida left a comment

avida Oct 5, 2021

keu Oct 5, 2021

keu left a comment

vitaliizazmic commented Oct 18, 2021 •

edited by github-actions bot

Loading

vitaliizazmic commented Nov 1, 2021 •

edited by github-actions bot

Loading

vitaliizazmic commented Nov 1, 2021 •

edited by github-actions bot

Loading

vitaliizazmic commented Nov 1, 2021 •

edited by github-actions bot

Loading

		@@ -59,6 +60,18 @@ def filter_output(records: Iterable[AirbyteMessage], type_) -> List[AirbyteMessa
		return list(filter(lambda x: x.type == type_, records))


		def remove_ignored_fields(output: List[AirbyteMessage], ignored_fields: Mapping[str, List[str]]) -> List[Mapping[str, Any]]:

🐛 Source Facebook Marketing: SAT ignored fields in TestFullRefresh.test_sequential_reads #6463

🐛 Source Facebook Marketing: SAT ignored fields in TestFullRefresh.test_sequential_reads #6463

Conversation

vitaliizazmic commented Sep 27, 2021

What

How

Pre-merge Checklist

vitaliizazmic commented Sep 27, 2021 • edited by github-actions bot Loading

CLAassistant commented Sep 27, 2021 • edited Loading

vitaliizazmic commented Oct 5, 2021 • edited by github-actions bot Loading

avida Oct 5, 2021

Choose a reason for hiding this comment

avida left a comment

Choose a reason for hiding this comment

avida Oct 5, 2021

Choose a reason for hiding this comment

keu Oct 5, 2021

Choose a reason for hiding this comment

keu left a comment

Choose a reason for hiding this comment

vitaliizazmic commented Oct 18, 2021 • edited by github-actions bot Loading

vitaliizazmic commented Nov 1, 2021 • edited by github-actions bot Loading

vitaliizazmic commented Nov 1, 2021 • edited by github-actions bot Loading

vitaliizazmic commented Nov 1, 2021 • edited by github-actions bot Loading

vitaliizazmic commented Sep 27, 2021 •

edited by github-actions bot

Loading

CLAassistant commented Sep 27, 2021 •

edited

Loading

vitaliizazmic commented Oct 5, 2021 •

edited by github-actions bot

Loading

vitaliizazmic commented Oct 18, 2021 •

edited by github-actions bot

Loading

vitaliizazmic commented Nov 1, 2021 •

edited by github-actions bot

Loading

vitaliizazmic commented Nov 1, 2021 •

edited by github-actions bot

Loading

vitaliizazmic commented Nov 1, 2021 •

edited by github-actions bot

Loading