Tdl 14376 pagination failure

[prijendev]

Description of change

• Updated one condition to break out of pagination loop.
  • If a whole blank page found, then stop looping.
• Fixed pagination test case.

Note : Unittest case was not possible for this PR. We have replaced just one condition and method which having this condition is too large and using lot of other methods. There is sync method with sync.py module. That's why to mock the other methods was not possible due to same name of method and module. So, skipped it.

Manual QA steps

• Verify that pagination does not break if last 2 records of any page is empty.

Risks

Rollback steps

• revert this branch

[savan-chovatiya]

Add a detailed comment about the data present in this sheet.

[prijendev]

Added

[karanpanchal-crest]

@prijendev Can you please explain what the earlier behavior row_num < to_row: meant?

[prijendev]

Here, to_row is initialized with a minimum of 200(max page size) or max_row. Then, it continues to add 200 until max_row. Initially, from_row is assigned by 2, and from the next page, it is assigned by to_row+1.(201 in second page). row_num is the addition of from_row and total records get in response. The above condition checks that if row_num is less than to_row or not based on which it set is_last_row true. But API does not return the last empty rows in response.

For example, rows 199 and 200 are empty, and a total 400 rows are there in the sheet. So, in 1st iteration

to_row = 200
from_row = 2
row_num = 2 + 197 = 199(1st row contain header value)
So, the above condition becomes true and breaks the loop.

[KrisPersonal]

1. Please add comments in the code as to why if row_num < to_row is replaced with sheet_data_rows.
2. If for example, if a sheet contains only 200 rows and let us say 99 and 100 are empty, then it will continue to process the remaining rows because of this condition being removed and by adding sheet_data_rows empty condition it will check whether the whole sheet is empty. Is that correct?

[prijendev]

1. Added detail comments in the code.
2. sheet_data_rows will check for an empty page, not a whole empty sheet.

[kspeer825]

We are striving to writeup defects found by QA in a way that makes reproducing issues as simple as uncommenting/commenting lines marked with BUG. Unless there is an explicit TODO left in the test, or a bullet in the DoD of the card there shouldn't be any missing test cases. I believe that is the case here and the test additions can be removed. If you find this is not the case please raise this to us so we can adjust our bug reporting process.

[kspeer825]

There should not have needed to be any test changes here besides adding the failing sheet back to the test. I don't think these test additions are adding any test coverage. The sheets have been setup in a way that relies on a specific column with incrementing values used to compare against the sdc column. See fake_pk_list above.

[prijendev]

Removed extra assertion. The current test is written in such a way that it was respecting only Pagination stream.

tap-google-sheets/tests/test_google_sheets_pagination.py

Lines 80 to 87 in 25136fc

	# verify the data for the "Pagination" stream is free of any duplicates or breaks by checking
	# our fake pk value ('id')
	# THIS ASSERTION CAN BE MADE BECAUSE WE SETUP DATA IN A SPECIFIC WAY. DONT COPY THIS
	self.assertEqual(list(range(1, 239)), fake_pk_list)
	# verify the data for the "Pagination" stream is free of any duplicates or breaks by checking
	# the actual primary key values (__sdc_row)
	self.assertEqual(list(range(2, 240)), actual_pk_list)

If we add back sadsheet-pagination then we have to write assertion according to this sheet also.

[kspeer825]

That comment is misleading, the test is setup to apply to both sheets

tap-google-sheets/tests/test_google_sheets_pagination.py

Line 38 in 25136fc

testable_streams = {"Pagination", "sadsheet-pagination"}

Additionally there is a large comment at the top of this test that should be removed

tap-google-sheets/tests/test_google_sheets_pagination.py

Lines 9 to 13 in 25136fc

	# BUG_TDL-14376 | https://jira.talendforge.org/browse/TDL-14376
	# Expectation: Tap will pick up next page (200 rows) iff there is a non-null value on that page
	# We observed a BUG where the tap does not paginate properly on sheets where the last two rows in a batch
	# are empty values. The tap does not capture anything on the subsequent pages when this happens.
	#

[prijendev]

Removed large comment at the top of this test. In the sadsheet-pagination sheet, there is a total of 238 rows and in that sheet row, no 199 and 200 are empty rows whereas in the Pagination sheet there are total of 239 rows with no empty row.
So, as two rows are empty in sadsheet-pagination, we need to write the separate assertion in which we are excluding rows no 199 and 200.

[kspeer825]

Responded to comments