Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TDL-14475 added unsupported feature and unittests #47

Merged

Conversation

namrata270998
Copy link
Contributor

Description of change

  • added unsupported inclusion property and a description explaining in the catalog

Manual QA steps

  • Added unit test case to check that the unsupported inclusion property is properly created.
  • Added unit test case to check the description is properly mapped.
  • Updated the integration test to verify unsupported fields

Risks

Rollback steps

  • revert this branch

@namrata270998 namrata270998 changed the base branch from master to TDL-15623-Crest-Master November 1, 2021 07:50
@namrata270998 namrata270998 changed the title added unsupported feature and unittests TDL-14475 added unsupported feature and unittests Nov 8, 2021
# in the columns dict. The `prior_column_skipped` would be true when it is the first column of the two
# consecutive empty headers column if true: update the incusion property to `unsupported`
for column in columns:
if column.get('columnSkipped') and not column.get('prior_column_skipped', None):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By default it will return None, do we need to pass None?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By default it will return None, do we need to pass None?

removed None as not needed

@@ -0,0 +1,192 @@
from tap_google_sheets.client import GoogleClient
import unittest
from unittest.case import TestCase
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can remove the unnecessary import of TestCase as the test case uses unittest.TestCase.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

import unittest
from unittest.case import TestCase
from unittest import mock
from tap_google_sheets import LOGGER, schema
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can remove unused import of LOGGER

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

}
sheet_json_schema, columns = schema.get_sheet_schema_columns(sheet)
self.assertEqual(sheet_json_schema, expected_schema) # test the schema is as expected
self.assertEqual(columns, expected_columns) # tst if the columns is as expected
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
self.assertEqual(columns, expected_columns) # tst if the columns is as expected
self.assertEqual(columns, expected_columns) # test if the columns is as expected

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Contributor

@KrisPersonal KrisPersonal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need code walk through of this change

@namrata270998
Copy link
Contributor Author

Need code walk through of this change

Provided code walkthrough

Copy link
Contributor

@kspeer825 kspeer825 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved with comment.

# BUG_TDL-14475 | https://jira.talendforge.org/browse/TDL-14475
failing_streams = {'sadsheet-column-skip-bug', 'Item Master'} # BUG_TDL-14475
if stream not in failing_streams: # BUG_TDL-14475
# The card TDL-14475 was only about adding unsupported
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this ticket. The original writeup for these bugs did not properly cover the two types of issues that we found here. The original assertion is correct in that both streams should be marked as unsupported, but for different reasons. By removing the bug mark and altering the assertion comment the bug is being covering up for the Item Master stream. In the future please raise discrepancies between tickets and tests so that issues do not get missed. I will card this out separately.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure @kspeer825 , we will take care from the next time. Also, we have started looking into the card that you have created.

@prijendev prijendev merged commit d76ae96 into TDL-15623-Crest-Master Dec 13, 2021
@prijendev prijendev mentioned this pull request Dec 13, 2021
KrisPersonal pushed a commit that referenced this pull request Dec 13, 2021
* type of emailaddress corrected (#53)

* Tdl 16079 check best practices (#51)

* Initial commit for best practice update

* updated setup.py and start_date test

* Updated test cases

* Updated start_date test case

* Updated start_date test case

* Updated comment

* Revert back test case changes

* Added new line

* Tdl 14376 pagination failure (#50)

* Initial commit for pagination failer

* Fixed pagination test cases

* Added comments

* Added detail comment into the code

* Removed unnecessary comment

* Removed unnecessary assertion

* Removed extra comment

* added comment for bug (#49)

* TDL-14475 added unsupported feature and unittests (#47)

* added unsupported feature and unittests

* added code comments

* fixed indent

* fixed indentation

* resolved a bug of writing md when 2 consecutive empty headers

* updated the logic for consecutive empty headers

* rsolved comments

* added test case for consecutive empty headers

* added comments

* resolved circleci errors

* resolved comments

Co-authored-by: namrata270998 <namrata.brahmbhatt@crestdatasystems.com>
Co-authored-by: prijendev <prijen.khokhani@crestdatasys.com>

* TDL-14397-Add skipped log when first row is empty (#46)

* added logger message and unittests

* added code comments

* changed the logger message and logic

* resolved comments

Co-authored-by: namrata270998 <namrata.brahmbhatt@crestdatasystems.com>
Co-authored-by: prijendev <prijen.khokhani@crestdatasys.com>

* TDL-16054 added code comments (#52)

* TDL-16054 added code comments

* rsolved comments

Co-authored-by: namrata270998 <namrata.brahmbhatt@crestdatasystems.com>
Co-authored-by: prijendev <prijen.khokhani@crestdatasys.com>

* Tdl 16280 implement request timeout (#54)

* TDL-16280 added request timeout

* TDL-16280: Added factor 3 to add more wait time between 2 calls

* TDL-16280: Updated Connection error as it wasn't defined.

* added backoff for access token

* updated readme

* updated request timeout and added jitter

* added comment for jitter

* added code coverage

* added testcase for connection error

* addd request timeout in config example

* updated the json example

* removed the client initialization outside with

Co-authored-by: dbshah1212 <dhruvin.shah@crestdatasys.com>
Co-authored-by: prijendev <prijen.khokhani@crestdatasys.com>

Co-authored-by: namrata270998 <75604662+namrata270998@users.noreply.github.com>
Co-authored-by: namrata270998 <namrata.brahmbhatt@crestdatasystems.com>
Co-authored-by: dbshah1212 <dhruvin.shah@crestdatasys.com>
@namrata270998 namrata270998 deleted the TDL-14475-make-unsupported-field-for-no-header branch June 2, 2022 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants