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

[Bug] parse-time validation when unit testing incremental model #9593

Closed
2 tasks done
Tracked by #8283
dbeatty10 opened this issue Feb 16, 2024 · 4 comments · Fixed by #9769
Closed
2 tasks done
Tracked by #8283

[Bug] parse-time validation when unit testing incremental model #9593

dbeatty10 opened this issue Feb 16, 2024 · 4 comments · Fixed by #9769
Assignees
Labels
bug Something isn't working dbt tests Issues related to built-in dbt testing functionality High Severity bug with significant impact that should be resolved in a reasonable timeframe incremental Incremental modeling with dbt pre-release Bug not yet in a stable release unit tests Issues related to built-in dbt unit testing functionality
Milestone

Comments

@dbeatty10
Copy link
Contributor

dbeatty10 commented Feb 16, 2024

Is this a new bug in dbt-core?

  • I believe this is a new bug in dbt-core
  • I have searched the existing issues, and I could not find an existing issue for this bug

Current Behavior

Suppose a model includes the is_incremental() macro.

If a unit test is added for that model, then an override like the following is required or else an error will occur:

    overrides:
      macros:
        is_incremental: true

Acceptance Criteria

We should have a parse time error when:

  • unit testing an incremental model but not providing an override for is_incremental
  • unit testing an incremental model overriding is_incremental: true but NOT providing an input for this

Steps To Reproduce

models/my_model.sql

-- is_incremental: {{ is_incremental() }}
select
    1 as id

models/_unit_tests.yml

unit_tests:

  - name: demo_unit_test_overrides

    given: []

    model: my_model

    expect:
      format: csv
      rows: |
        id
        1
dbt run -s models/my_model.sql
dbt build -s models/my_model.sql

💥 See error log below.

In order to work, the following needs to be added within the unit test YAML above:

    overrides:
      macros:
        is_incremental: true

Relevant log output

21:41:03    Compilation Error in unit_test my_model__demo_unit_test_overrides (models/_unit_tests.yml)
  'None' has no attribute 'database'

Environment

- OS:
- Python:
- dbt: 1.8 beta

Which database adapter are you using with dbt?

No response

Additional Context

No response

@dbeatty10 dbeatty10 added bug Something isn't working triage dbt tests Issues related to built-in dbt testing functionality pre-release Bug not yet in a stable release and removed triage labels Feb 16, 2024
@graciegoheen
Copy link
Contributor

Curious what @MichelleArk @jtcohen6 you think here...

I actually think it's a good thing that we require you to be explicit about whether this is a unit test for the incremental model in "incremental" mode or in "full refresh" mode. I'm not convinced we should set a default here (is that a spicy take?). However, if we don't set a default, we should:

  • update our docs on unit testing incremental models
  • update the error message returned to be explicit "For incremental models you must set is_incremental to true or false in your unit test overrides" (or something similar) - right now, the error message is unreadable

@MichelleArk
Copy link
Contributor

This particular error is probably happening because this was not overwritten, and the 'real' is_incremental macro was attempting to access this.database: https://github.com/dbt-labs/dbt-adapters/blob/main/dbt/include/global_project/macros/materializations/models/incremental/is_incremental.sql#L7

I'm not convinced we should set a default here (is that a spicy take?).

I'm not convinced either. It feels like either setting (is_incremental: true or is_incremental: false) could be perceived as surprising behaviour and we'd get issue reports either way.

update the error message returned to be explicit "For incremental models you must set is_incremental to true or false in your unit test overrides" (or something similar)

I like the idea of parse-time validation here! 👍


@graciegoheen graciegoheen changed the title [Bug] Unit test requires explicit override when the model uses the is_incremental() macro [Bug] Better error message when unit testing incremental model Feb 20, 2024
@graciegoheen graciegoheen changed the title [Bug] Better error message when unit testing incremental model [Bug] parse-time validation when unit testing incremental model Feb 21, 2024
@graciegoheen
Copy link
Contributor

Sounds good! Updated the issue name and acceptance criteria accordingly

@graciegoheen graciegoheen added this to the v1.8 milestone Feb 21, 2024
@dbeatty10 dbeatty10 added the incremental Incremental modeling with dbt label Feb 21, 2024
@graciegoheen graciegoheen added the High Severity bug with significant impact that should be resolved in a reasonable timeframe label Feb 22, 2024
@MichelleArk
Copy link
Contributor

From refinement:

  • Can determine whether the model being tested relies on the is_incremental macro based on its depends_on.macros (rather than checking for materialized=incremental, which is less precise)
  • can also validate that is_incremental is being overriden with a boolean value

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working dbt tests Issues related to built-in dbt testing functionality High Severity bug with significant impact that should be resolved in a reasonable timeframe incremental Incremental modeling with dbt pre-release Bug not yet in a stable release unit tests Issues related to built-in dbt unit testing functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants