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

use test materialization for schema/generic tests #3286

Merged
merged 3 commits into from
Apr 27, 2021

Conversation

kwigley
Copy link
Contributor

@kwigley kwigley commented Apr 22, 2021

resolves #3192

Description

  • convert schema/generic tests to use test jijna tag
  • update schema/generic tests to return rows that include failures instead of a count of failures (this still needs some love, but will help support Storing test results in the database #2593)

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 updated the CHANGELOG.md and added information about my change to the "dbt next" section.

@kwigley kwigley requested review from gshank, jtcohen6 and iknox-fa April 22, 2021 15:18
@kwigley kwigley self-assigned this Apr 22, 2021
@cla-bot cla-bot bot added the cla:yes label Apr 22, 2021
@kwigley kwigley force-pushed the feature/schema-test-materialization branch from b368d60 to ac8cd78 Compare April 22, 2021 15:26
Copy link
Contributor

@jtcohen6 jtcohen6 left a comment

Choose a reason for hiding this comment

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

This is looking really good!

One thing we can think about before / as part of #2593: What data should each generic test query return? For the not_null test, the entire row (*) makes sense, but I could imagine the unique and accepted_values test including the count of each duplicated/unaccepted value, too. That does not need to block merging this PR, just something on my mind.

Comment on lines 15 to 16
select 'Expected {{ expected }}, but got {{ actual }}' as validation_error
where {{ success }} = 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Nicely done! I like this a lot

Copy link
Contributor Author

@kwigley kwigley Apr 22, 2021

Choose a reason for hiding this comment

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

The where hack actually doesn't work on bigquery :(

Query without FROM clause cannot have a WHERE clause at [..]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is definitely a perfect example of a where test config.. any thoughts what to do for the time being or for this instance specifically?

Copy link
Contributor

Choose a reason for hiding this comment

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

Heh, ok, try this:

select 'Expected {{ expected }}, but got {{ actual }}' as validation_error
from (select true)
where {{ success }} = 0

The where test config should work a little differently—that would be filtering/rewriting the actual {{ model }} reference (as a subquery). There would definitely be a from clause.

@@ -15,4 +15,4 @@
select 1 as error
{% endif %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this just need the same treatment as above?

select '{{ table.type }} does not match expected value {{ type }}'
where '{{ table.type }}' != '{{ type }}'

@@ -65,5 +64,5 @@
{% endif %}
{% endfor %}
{% do log('bad columns: ' ~ bad_columns, info=True) %}
select {{ bad_columns | length }} as pass_fail
{% endmacro %}
select * from unnest(string_to_array('{{ bad_columns|join(',') }}', ','))
Copy link
Contributor

Choose a reason for hiding this comment

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

fancy! :)

This works well on Postgres. If we needed to run it on any database, I think we could do:

{% for bad_column in bad_columns %}
  select '{{ bad_column }}' as bad_column
  {{ 'union all' if not loop.last }}
{% endfor %}

Copy link
Contributor Author

@kwigley kwigley Apr 22, 2021

Choose a reason for hiding this comment

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

I totally agree! The piece here that I'm struggling to handle correctly is if there are no bad columns (bad_columns == []) In this case, no sql will be rendered.

Copy link
Contributor

@jtcohen6 jtcohen6 Apr 22, 2021

Choose a reason for hiding this comment

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

Ohh right! So this is a bit jankier, but perhaps something like:

{% for bad_column in bad_columns %}
  select '{{ bad_column }}' as bad_column union all
{% endfor %}
  select * from (select 1 limit 0)

If we only run this test on postgres, I like your answer better :) All the more reason for adapter-specific implementations of generic tests...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🙏

@@ -20,7 +21,7 @@ def models(self):
def test_postgres_column_types(self):
self.run_and_test()


@pytest.mark.skip("Temporarily skipping while implementing the handling of test results.")
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@kwigley kwigley force-pushed the feature/schema-test-materialization branch from 15fa927 to 1d7b4c0 Compare April 22, 2021 20:24
Copy link
Contributor

@iknox-fa iknox-fa left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@jtcohen6 jtcohen6 left a comment

Choose a reason for hiding this comment

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

+78 −422

LGr8TM!

@kwigley kwigley merged commit 8c55e74 into develop Apr 27, 2021
@kwigley kwigley deleted the feature/schema-test-materialization branch April 27, 2021 12:05
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.

Use test materialization when executing ✨generic✨ tests
4 participants