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 355/migrate simple seed #5013

Closed
wants to merge 13 commits into from
Closed

Conversation

VersusFacit
Copy link
Contributor

@VersusFacit VersusFacit commented Apr 8, 2022

Migrates most (soon all) of 005 simple seed.

Description

Using my best judgement of the test framework, I've gotten these tests working locally! There are two main TODOs where I'm sure there is a better way to accomplish things than the ideas I was able to come up with. I'll mark with inline comments. Please do suggest things so I can get them ship-shape.

Updated:

I've figured out how to get all the tests working per the issues stated above.

Checklist

  • I have signed the CLA
  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • I have added information about my change to be included in the CHANGELOG.

@VersusFacit VersusFacit self-assigned this Apr 8, 2022
@VersusFacit VersusFacit requested a review from a team as a code owner April 8, 2022 11:56
@cla-bot cla-bot bot added the cla:yes label Apr 8, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Apr 8, 2022

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.

check_table_does_exist(project.adapter, "seed_enabled")
check_table_does_not_exist(project.adapter, "seed_disabled")
check_table_does_exist(project.adapter, "seed_tricky")
# TODO: 1/3 is there a better way to enforce cleanup
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this is required cleanup. I'm guessing there is a better way to reset the database after every test_ when using the def seeds fixture in a test class.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can have your setup fixture do it by doing the setup, yield something and do the drop table after yield. Examples

Copy link
Contributor

Choose a reason for hiding this comment

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

We do now have project.drop_test_schema(). I haven't tested it for this use case, but it should work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to get the project.drop_test_schema() to work but no dice. Digging into the code didn't reveal any answers. Opted for this instead:

182     @pytest.fixture(scope="function")
183     def clear_test_schema(self, project):
184         yield
185         project.run_sql(f"drop schema if exists {project.test_schema} cascade")
186
187     def test_postgres_simple_seed_with_disabled(self, clear_test_schema, project):

@VersusFacit VersusFacit marked this pull request as draft April 8, 2022 12:02
Copy link
Contributor

@ChenyuLInx ChenyuLInx left a comment

Choose a reason for hiding this comment

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

This looks great!!!

tests/functional/simple_seed/test_seed.py Outdated Show resolved Hide resolved
check_table_does_exist(project.adapter, "seed_enabled")
check_table_does_not_exist(project.adapter, "seed_disabled")
check_table_does_exist(project.adapter, "seed_tricky")
# TODO: 1/3 is there a better way to enforce cleanup
Copy link
Contributor

Choose a reason for hiding this comment

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

You can have your setup fixture do it by doing the setup, yield something and do the drop table after yield. Examples

tests/functional/simple_seed/test_seed.py Outdated Show resolved Hide resolved
tests/functional/simple_seed/test_seed.py Outdated Show resolved Hide resolved
# check_relations_equal(project.adapter, ["seed_actual", "seed_expected"])


class TestSimpleSeedEnabledViaConfig(object):
Copy link
Contributor

Choose a reason for hiding this comment

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

@gshank @VersusFacit How do you guys feel about moving this to selection(graph selection?) tests? since this is mainly testing the selection functionality

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could weigh in better if I understood the primacy of the selection feature. Do we consider it a subfeature of seed or do we consider seed a command which mixes in the selection behavior. I feel like that reasoning best informs whether it should be moved to a selection test or not.

tests/functional/simple_seed/test_seed.py Outdated Show resolved Hide resolved
Copy link
Contributor

@gshank gshank left a comment

Choose a reason for hiding this comment

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

Looks good, but needs a few minor changes.

tests/functional/simple_seed/test_seed.py Outdated Show resolved Hide resolved
tests/functional/simple_seed/test_seed.py Outdated Show resolved Hide resolved
)


class TestSeedConfigFullRefreshOn(SeedTestBase):
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a reminder that if these tests are moved into the adapter zone, they'll all have to have names that don't start with 'Test' and a simple class wrapper that does start with 'Test'. Doesn't have to be done now though.

tests/functional/simple_seed/test_seed.py Outdated Show resolved Hide resolved
check_table_does_exist(project.adapter, "seed_enabled")
check_table_does_not_exist(project.adapter, "seed_disabled")
check_table_does_exist(project.adapter, "seed_tricky")
# TODO: 1/3 is there a better way to enforce cleanup
Copy link
Contributor

Choose a reason for hiding this comment

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

We do now have project.drop_test_schema(). I haven't tested it for this use case, but it should work.

tests/functional/simple_seed/test_seed.py Outdated Show resolved Hide resolved
@VersusFacit VersusFacit marked this pull request as ready for review April 9, 2022 11:15
@VersusFacit VersusFacit requested a review from a team April 9, 2022 11:15
@VersusFacit VersusFacit requested a review from a team as a code owner April 9, 2022 11:15
Copy link
Contributor

@gshank gshank left a comment

Choose a reason for hiding this comment

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

I have no idea what's up with the postgres error on Windows:
[DEBUG] configured_file: 22:21:45.398957 [debug] [MainThread]: On _test: /* {"app": "dbt", "dbt_version": "1.1.0b1", "profile_name": "test", "target_name": "default", "connection_name": "_test"} /
with diff_count as (
SELECT
1 as id,
COUNT(
) as num_missing FROM (
(SELECT "birthday", "email", "first_name", "ip_address", "seed_id" FROM "dbt"."test16495429020601287135_test_seed"."seed_expected" EXCEPT
SELECT "birthday", "email", "first_name", "ip_address", "seed_id" FROM "dbt"."test16495429020601287135_test_seed"."seed_bom")
UNION ALL
(SELECT "birthday", "email", "first_name", "ip_address", "seed_id" FROM "dbt"."test16495429020601287135_test_seed"."seed_bom" EXCEPT
SELECT "birthday", "email", "first_name", "ip_address", "seed_id" FROM "dbt"."test16495429020601287135_test_seed"."seed_expected")
) as a
), table_a as (
SELECT COUNT() as num_rows FROM "dbt"."test16495429020601287135_test_seed"."seed_expected"
), table_b as (
SELECT COUNT(
) as num_rows FROM "dbt"."test16495429020601287135_test_seed"."seed_bom"
), row_count_diff as (
select
1 as id,
table_a.num_rows - table_b.num_rows as difference
from table_a, table_b
)
select
row_count_diff.difference as row_count_difference,
diff_count.num_missing as num_mismatched
from row_count_diff
join diff_count using (id)
[DEBUG] configured_file: 22:21:45.398957 [debug] [MainThread]: Postgres adapter: Postgres error: column "seed_id" does not exist
LINE 7: ... "birthday", "email", "first_name", "ip_address", "seed_id" ...
^
HINT: There is a column named "seed_id" in table "SELECT 1", but it cannot be referenced from this part of the query.

@gshank
Copy link
Contributor

gshank commented Apr 12, 2022

It looks like the current failure is in pytest-csv and is due to codepage 1252. So it could be that that pytest plugin isn't compatible with unicode csv files. But I have no idea how the previous error got turned into this error...

Comment on lines +233 to +236
shutil.copyfile(
project.test_data_dir / Path("seed_bom.csv"),
project.project_root / Path("seeds") / Path("seed_bom.csv"),
)
Copy link
Contributor

@ChenyuLInx ChenyuLInx Apr 13, 2022

Choose a reason for hiding this comment

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

@gshank this is currently being used to copy the csv file over to project seed dir. I wonder which part actually uses pytest_csv

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like pytest-csv is for creating test reports in csv format. So I have no idea how it got involved, but I saw it in the error log.

@VersusFacit
Copy link
Contributor Author

At last! On remembering some scattered notes about unicode I studied some 2ish months ago, I got the idea that the encoding for the unicode file might be cp1252 on Windows. Using the system codepoints and manually enforcing the encoding resolves this (Gerda suggested this yesterday I see and I wish I had seen it sooner!)

The insight came from comparing what the default behavior of open was on Windows vs. Unix-systems. I need to move some more things around and this PR is a mess. I'll reopen shortly for a much cleaner experience. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants