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

Built-in unique test to not be explicitly added to "primary_key_test_macros" #497

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

PatFitzner
Copy link

@PatFitzner PatFitzner commented Sep 19, 2024

In order to avoid name collisions on unique test definition overrides, the line that explicitly adds the builtin dbt.test_unique test to the "primary_key_test_macros" list has been replaced with a conditional block that will only add it if none of the tests already present in the "primary_key_test_macros" list would produce a is_test_unique column in the int_all_graph_resources model, which is required downstream by int_model_test_summary.

This is a:

  • bug fix PR with no breaking changes

Link to Issue

Closes 496

Description & motivation

Line 10 of models/marts/core/int_all_graph_resources.sql

{%- do test_macro_list.append("dbt.test_unique") -%}

Is supposed to ensure that the column is_test_unique is generated, but breaks the execution of the package when the default test dbt.test_unique has been overridden in a project.

Checklist

  • I have verified that these changes work locally on the following warehouses (Note: it's okay if you do not have access to all warehouses, this helps us understand what has been covered)
    • BigQuery
    • Postgres
  • I have updated the README.md (if applicable)
  • I have added tests & descriptions to my models (and macros if applicable)

@PatFitzner
Copy link
Author

I feel like CircleCI is out to get me. I've pushed the same commit 3 or 4 times with amended comments to trigger a new CI execution, and the failures are not always the same, as well as unrelated to the changes of this PR (non-exsiting tables, failed freshness / non-documented model tests ...)
Could you please let me know what I may be missing here @graciegoheen @dave-connors-3 @b-per ?

Thanks <3

@PatFitzner PatFitzner changed the title Bbuilt-in unique test to not be explicitly added to "primary_key_test_macros" Built-in unique test to not be explicitly added to "primary_key_test_macros" Sep 19, 2024
@b-per
Copy link
Collaborator

b-per commented Sep 26, 2024

Thanks @PatFitzner
Yes, CI is a bit flaky with some adapters unfortunately but we will have a look.

I think that the original piece of code on test_unique was added because we allow people to say that a model is PK-tested if it has a unique test and has a not null constraint on it.

I think that your approach is correct but I will need to do a few more checks.

@b-per
Copy link
Collaborator

b-per commented Nov 27, 2024

Hi @PatFitzner

Sorry for the delay here.

I finally spent the time looking at this issue and your PR looks all good! Thanks for the contribution. As this is a bug we might release it before the v1 that we are planning to release soon.

@PatFitzner
Copy link
Author

Hi @PatFitzner

Sorry for the delay here.

I finally spent the time looking at this issue and your PR looks all good! Thanks for the contribution. As this is a bug we might release it before the v1 that we are planning to release soon.

Thank you, Benoit!

Looking forward to v1

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.

Duplicate tests found when overriding default dbt.test_unique
2 participants