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

[CT-1959]: moving simple_seed tests to adapter zone #6859

Merged
merged 9 commits into from
Feb 7, 2023

Conversation

nssalian
Copy link
Contributor

@nssalian nssalian commented Feb 3, 2023

resolves #

Description

Moving the simple seed test code over to the adapter zone to help convert tests in adapters.

Checklist

@nssalian nssalian requested review from a team as code owners February 3, 2023 22:29
@cla-bot cla-bot bot added the cla:yes label Feb 3, 2023
@nssalian nssalian added Skip Changelog Skips GHA to check for changelog file and removed cla:yes labels Feb 3, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Feb 3, 2023

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

@cla-bot cla-bot bot added the cla:yes label Feb 3, 2023
@nssalian nssalian removed the Skip Changelog Skips GHA to check for changelog file label Feb 3, 2023
@nssalian nssalian requested a review from VersusFacit February 3, 2023 23:07

# manual copy because seed has a special and tricky-to-include unicode character at 0
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to preserve these as actual .csv files to validate this? I'm wondering if it would be easier to have a core-only test that looks at some of these edge cases (BOM, unicode etc.) and an actually "simple" seed test in the adapter zone that just validates loading csv files

Copy link
Contributor Author

@nssalian nssalian Feb 4, 2023

Choose a reason for hiding this comment

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

I think that would be a better approach. I'll modify the code.
Update: rethinking this - the adapter zone could include a csv that checks for these cases rather than having the tests split across the two paths. No duplication.

Copy link
Contributor

@VersusFacit VersusFacit left a comment

Choose a reason for hiding this comment

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

A good rework of this test. Now, I remember how hard this was when I did it because of the BOM unprintable character -- it stumped everyone on the team until I chanced upon the hidden character one day.

Can we comment referencing that on which test case that is, just so people understand we have a hidden character there? That feels very important for future readers who may run into unprintable character issues.

Otherwise, LGTM

@nssalian nssalian merged commit 4c63b63 into main Feb 7, 2023
@nssalian nssalian deleted the ns/ct-1959-simple-seed-adapter-zone branch February 7, 2023 03:51
github-actions bot pushed a commit that referenced this pull request Mar 7, 2023
* Formatting

* Changelog entry

* Rename to BaseSimpleSeedColumnOverride

* Better error handling

* Update test to include the BOM test

* Cleanup and formating

* Unused import remove

* nit line

* Pr comments

(cherry picked from commit 4c63b63)
colin-rogers-dbt pushed a commit that referenced this pull request Mar 7, 2023
* Formatting

* Changelog entry

* Rename to BaseSimpleSeedColumnOverride

* Better error handling

* Update test to include the BOM test

* Cleanup and formating

* Unused import remove

* nit line

* Pr comments

(cherry picked from commit 4c63b63)

Co-authored-by: Neelesh Salian <nssalian@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants