Implement all_fields test using new framework-tdl-7499 #159
Conversation
Merge branch 'tdl-7499' of github.com:singer-io/tap-salesforce into tdl-7499
…s based on the api type
|
Re-started commit work flow from start as last test failed due to quota to see if tests will pass. |
| @@ -844,6 +852,14 @@ def rest_only_streams(self): | |||
| 'UndecidedEventRelation', | |||
| } | |||
|
|
|||
| def expected_streams(self): | |||
There was a problem hiding this comment.
Some of these self references may have to change to cls references for some of the methods according to recent changes in sfbase.
…assertion for the custom field name
| def test_non_custom_fields( self ): | ||
| for stream in self.streams_to_selected_fields(): | ||
| expected_non_custom_fields = self.streams_to_selected_fields().get(stream, set() ) | ||
| replicated_non_custom_fields = self.actual_fields.get(stream, set() ).difference(self.expected_automatic_fields(stream)) |
There was a problem hiding this comment.
We don't want to subtract automatic fields here because the automatic fields are non custom fields.
There was a problem hiding this comment.
The reason to subtract automatic fields - if the replicated fields contain only automatic fields, my assertion for Not none in the next line will not verify if fields other than automatic fields are replicated or not.
There was a problem hiding this comment.
The automatic fields are non custom fields. The test is currently making a sub-set of non_custom fields. Do we need to track both groups? All non custom fields and then all non custom fields that aren't automatic? I'm not sure we need to do this. We could have two assertions if you want to check both cases:
self.assertIsNotNone(replicated_non_custom_fields, msg)
self.assertIsNotNone(replicated_non_custom_fields.difference(self.expected_automatic_fields(stream)), msg)
There was a problem hiding this comment.
I think this is the test that is still failing. I suggest not removing automatic fields from replicated_non_custom_fields. Then we can add two more assertions
- The first assertion to make sure that the count of non custom fields is greater than the count of automatic fields to make sure that we are replicating more than just the automatic fields.
- To make sure the number of non custom fields is equal to the number of replicated fields to make sure we didn't miss any.
| def test_non_custom_fields( self ): | ||
| for stream in self.streams_to_selected_fields(): | ||
| expected_non_custom_fields = self.streams_to_selected_fields().get(stream, set() ) | ||
| replicated_non_custom_fields = self.actual_fields.get(stream, set() ).difference(self.expected_automatic_fields(stream)) |
There was a problem hiding this comment.
The automatic fields are non custom fields. The test is currently making a sub-set of non_custom fields. Do we need to track both groups? All non custom fields and then all non custom fields that aren't automatic? I'm not sure we need to do this. We could have two assertions if you want to check both cases:
self.assertIsNotNone(replicated_non_custom_fields, msg)
self.assertIsNotNone(replicated_non_custom_fields.difference(self.expected_automatic_fields(stream)), msg)
tests/sfbase.py
Outdated
| non_custom += 1 | ||
| else: | ||
| custom += 1 | ||
| return ( custom, non_custom ) |
There was a problem hiding this comment.
| return ( custom, non_custom ) | |
| return (custom, non_custom) |
|
|
||
| def test_custom_fields( self ): | ||
| for stream in self.streams_to_selected_fields(): | ||
| expected_custom_fields = self.streams_to_selected_fields().get(stream, set() ) |
There was a problem hiding this comment.
| expected_custom_fields = self.streams_to_selected_fields().get(stream, set() ) | |
| expected_custom_fields = self.streams_to_selected_fields().get(stream, set()) |
| replicated_custom_fields = self.actual_fields.get(stream, set() ).difference(self.expected_automatic_fields(stream)) | ||
|
|
||
| #Verify at least one custom field is replicated | ||
| self.assertIsNotNone( replicated_custom_fields, msg = f"Replication didn't return any custom fields for stream {stream}" ) |
There was a problem hiding this comment.
| self.assertIsNotNone( replicated_custom_fields, msg = f"Replication didn't return any custom fields for stream {stream}" ) | |
| self.assertIsNotNone(replicated_custom_fields, msg = f"Replication didn't return any custom fields for stream {stream}") |
|
|
||
| #Verify only custom fields are replicated by checking the field name | ||
| num_custom, num_non_custom = self.count_custom_non_custom_fields(replicated_custom_fields) | ||
| self.assertEqual( num_non_custom, 0, "Replicated some fields that are not custom fields for stream {stream}" ) |
There was a problem hiding this comment.
| self.assertEqual( num_non_custom, 0, "Replicated some fields that are not custom fields for stream {stream}" ) | |
| self.assertEqual(num_non_custom, 0, | |
| "Replicated some fields that are not custom fields for stream {stream}") |
| def streams_to_selected_fields(): | ||
| return SFBaseTest.non_custom_fields | ||
|
|
||
| def test_non_custom_fields( self ): |
There was a problem hiding this comment.
| def test_non_custom_fields( self ): | |
| def test_non_custom_fields(self): |
|
|
||
| def test_non_custom_fields( self ): | ||
| for stream in self.streams_to_selected_fields(): | ||
| expected_non_custom_fields = self.streams_to_selected_fields().get(stream, set() ) |
There was a problem hiding this comment.
| expected_non_custom_fields = self.streams_to_selected_fields().get(stream, set() ) | |
| expected_non_custom_fields = self.streams_to_selected_fields().get(stream, set()) |
| replicated_non_custom_fields = self.actual_fields.get(stream, set() ).difference(self.expected_automatic_fields(stream)) | ||
|
|
||
| #Verify at least one non-custom field is replicated | ||
| self.assertIsNotNone( replicated_non_custom_fields, msg = f"Replication didn't return any non-custom fields for stream {stream}" ) |
There was a problem hiding this comment.
| self.assertIsNotNone( replicated_non_custom_fields, msg = f"Replication didn't return any non-custom fields for stream {stream}" ) | |
| self.assertIsNotNone(replicated_non_custom_fields, msg = f"Replication didn't return any non-custom fields for stream {stream}") |
| replicated_non_custom_fields = self.actual_fields.get(stream, set() ).difference(self.expected_automatic_fields(stream)) | ||
|
|
||
| #Verify at least one non-custom field is replicated | ||
| self.assertIsNotNone( replicated_non_custom_fields, msg = f"Replication didn't return any non-custom fields for stream {stream}" ) |
There was a problem hiding this comment.
| self.assertIsNotNone( replicated_non_custom_fields, msg = f"Replication didn't return any non-custom fields for stream {stream}" ) | |
| self.assertIsNotNone(replicated_non_custom_fields, msg = f"Replication didn't return any non-custom fields for stream {stream}") |
|
|
||
| #Verify ustom fields are not replicated by checking the field name | ||
| num_custom, num_non_custom = self.count_custom_non_custom_fields(replicated_non_custom_fields) | ||
| self.assertEqual(num_custom, 0, "Replicated some fields that are custom fields for stream {stream}" ) |
There was a problem hiding this comment.
| self.assertEqual(num_custom, 0, "Replicated some fields that are custom fields for stream {stream}" ) | |
| self.assertEqual(num_custom, 0, "Replicated some fields that are custom fields for stream {stream}") |
tests/sfbase.py
Outdated
| from datetime import timedelta | ||
| from datetime import datetime as dt | ||
|
|
||
| from operator import itemgetter |
tests/sfbase.py
Outdated
| """ List all the custom_fields for each stream""" | ||
| custom_fields = {} | ||
| for stream in self.streams_to_test(): | ||
| with self.subTest(stream=stream): |
There was a problem hiding this comment.
I don't think there's any assertions in this function, so we can remove this subTest line
There was a problem hiding this comment.
Sure, will do that
tests/sfbase.py
Outdated
| """ List all the non_custom_fields for each stream""" | ||
| non_custom_fields = {} | ||
| for stream in self.streams_to_test(): | ||
| with self.subTest(stream=stream): |
There was a problem hiding this comment.
I don't think there's any assertions in this function, so we can remove this subTest line
tests/sfbase.py
Outdated
| non_custom_fields[stream] = {key for key in schema['properties'].keys() if not key.endswith("__c") | ||
| and schema['properties'][key]['inclusion']!="unsupported"} |
There was a problem hiding this comment.
- There's two spaces between
andandschema- There should be one
- There's no spaces around
!=- There should be one on each side
Optional: I personally think if there's an if in a comprehension, it should get its own line and every and after that too. To me, it helps separate the filtering from the looping
non_custom_fields[stream] = {key
for key in schema['properties'].keys()
if not key.endswith("__c")
and schema['properties'][key]['inclusion'] != "unsupported"}| logging=f"verify all fields are replicated for stream {stream}") | ||
|
|
||
| #Verify that only custome fields are replicated besides automatic fields | ||
| num_custom, num_non_custom = self.count_custom_non_custom_fields(replicated_custom_fields) |
| def streams_to_test(self): | ||
| if self.partitioned_streams: | ||
| return self.partitioned_streams | ||
| return self.partition_streams(self.get_streams_with_data()) |
There was a problem hiding this comment.
This function is repeated 4 times and should be moved to sfbase
There was a problem hiding this comment.
All these tests are implement in the new tap-tester framework and the All_fields test needs this method to be present in the test as it is a abstract method in tap-tester all_fields test , if I understood it right. Do let me know if thats not the case.
https://github.com/stitchdata/tap-tester/blob/b280a171ce2ecc565d54f96bbef7cac9fe4a61ec/tap_tester/base_suite_tests/all_fields_test.py#L49
There was a problem hiding this comment.
I think if SFBaseTest implements it then SFNonCustomFieldsTest doesn't have to
There was a problem hiding this comment.
Let me try to write up a quick test
| msg = f"All non_custom fields are not no replicated for stream {stream}") | ||
|
|
||
| #verify that automatic fields are also replicated along with non_custom_fields | ||
| self.assertTrue(self.expected_automatic_fields(stream).issubset(replicated_non_custom_fields, |
There was a problem hiding this comment.
| self.assertTrue(self.expected_automatic_fields(stream).issubset(replicated_non_custom_fields, | |
| self.assertTrue(self.expected_automatic_fields(stream).issubset(replicated_non_custom_fields), |
This missing paren is causing CI to fail
tests/test_salesforce_sync_canary.py
Outdated
| # Run sync | ||
| menagerie.set_state(conn_id, {}) | ||
| _ = self.run_and_verify_sync(conn_id) | ||
| record_count_by_stream = self.run_and_verify_sync_mode(conn_id) | ||
| actual_streams_with_data ={stream for stream in record_count_by_stream.keys() |
There was a problem hiding this comment.
Just so you know, looping over a dictionary gives you the .keys() by default
| return non_custom_fields | ||
|
|
||
| def test_non_custom_fields(self): | ||
| for stream in self.streams_to_selected_fields(): |
There was a problem hiding this comment.
Should this be wrapped in a subTest so that in one run you can see all of the failing streams?
|
|
||
| #Verify only custom fields are replicated by checking the field name | ||
| num_custom, num_non_custom = self.count_custom_non_custom_fields(replicated_custom_fields) | ||
| self.assertEqual(num_non_custom, 0, "Replicated some fields that are not custom fields for stream {stream}") |
There was a problem hiding this comment.
I'd like to add a second assertion that the num_custom == replicated_custom_fields + automatic fields. This will make sure we replicated them all and didn't miss any rather than making sure we replicated at least 1 with the is not none check.
Description of change
https://jira.talendforge.org/browse/TDL-7499
Implement all_fields_test using the new tap-tester framework.
Tested with streams that also has custom fields
Also modified tap-tester/base_suite_tests/base_case.py : get_all_streams_and_fields to exclude unsupported fields
QA steps
Risks
Rollback steps